-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve DynamicWhereFilter performance #5293
Conversation
Lots of changes but tests passing.
…. Add abstract length() method.
Rwc index lookup via tuples
Hopefully better code.
… favor of sourceKeySource.
…r now, since we were relying on a specific implementation that's not dissimilar from ours.
Require.geq(partialKeySize, "partialKeySize", 2, "2"); | ||
|
||
final Object[] keysInDataIndexOrder = new Object[partialKeySize]; | ||
// TODO: decide if this is ridiculous or clever. Trying to avoid looping inside the lambda if possible and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a sheer "look at how much code there is" I kind of hate it. If benchmarks show it to be better, we can keep it, I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not offended, it's very ugly and probably pointless to attempt to outguess the JIT.
engine/table/src/main/java/io/deephaven/engine/table/impl/select/DynamicWhereFilter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I'll benchmark ASAP vs. main.
Require.geq(partialKeySize, "partialKeySize", 2, "2"); | ||
|
||
final Object[] keysInDataIndexOrder = new Object[partialKeySize]; | ||
// TODO: decide if this is ridiculous or clever. Trying to avoid looping inside the lambda if possible and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not offended, it's very ugly and probably pointless to attempt to outguess the JIT.
...src/main/java/io/deephaven/engine/table/impl/select/setinclusion/ByteSetInclusionKernel.java
Show resolved
Hide resolved
Rwc index lookup via tuples
Fixes #5268