Conversation
The LookupJoinMatcher needs to check if a condition is always true or false multiple times. This can be pre-computed to speed up the match checking This change reduces the time it takes to perform a for joining on a long key from ~ 36 ms/op to 23 ms/ op
jihoonson
left a comment
jihoonson
left a comment
There was a problem hiding this comment.
Cool, thank you for the PR. It seems like the benchmark class is not in master yet. Would you please link the benchmark class here?
| return equiConditions.isEmpty() && | ||
| nonEquiConditions.stream() | ||
| .allMatch(expr -> expr.isLiteral() && expr.eval(ExprUtils.nilBindings()).asBoolean()); | ||
| return equiConditions.isEmpty() && allTrueLiteralNonEquiConditions; |
There was a problem hiding this comment.
It seems like allTrueLiteralNonEquiConditions is only used here; how about caching isAlwaysTrue directly?
| { | ||
| return nonEquiConditions.stream() | ||
| .anyMatch(expr -> expr.isLiteral() && !expr.eval(ExprUtils.nilBindings()).asBoolean()); | ||
| return anyFalseLiteralNonEquiConditions; |
There was a problem hiding this comment.
Why not call this isAlwaysFalse? (It looks like it isn't used anywhere else, and it seems to me to be easier to understand the meaning of the field if it's named after what we want it to mean.)
There was a problem hiding this comment.
yeah. I wasn't sure if isAlwaysFalse could be more complex going forward. This is clearer - it took me a while to wrap my head around what isAlwaysFalse was trying to check. Added comments, and used isAlways...
@jihoonson here's the benchmark I was using. I will push it up to master once I clean it up a little more and add more tests |
4 participants
Description
The LookupJoinMatcher needs to check if a condition is always true or false
multiple times. This can be pre-computed to speed up the match checking
This change reduces the time it takes to perform a for joining on a long key
from ~ 36 ms/op to 23 ms/ op
This PR has: