Chore: refactor Comparison out of QueryPlanSerde #2028
Conversation
- Create comparisons.scala following the pattern from math/array expressions. - Implements CometGreaterThan as proof of concept for issue apache#2019.
CuteChuanChuan
force-pushed
the
raymond/refactor-expressions-in-QueryPlanSerde
branch
from
56ddd51 to
202556d
Compare
- Add ComparisonBase trait with reusable helper methods - Move GreaterThan, GreaterThanOrEqual, LessThan, LessThanOrEqual to CometComparison* - Move IsNull, IsNotNull, IsNaN to CometIs* classes - Move In expression to CometIn class - Update exprSerdeMap to use new comparison classes - Remove corresponding case statements from exprToProtoInternal
CuteChuanChuan
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2028 +/- ##
============================================
+ Coverage 56.12% 58.83% +2.70%
- Complexity 976 1253 +277
============================================
Files 119 138 +19
Lines 11743 13198 +1455
Branches 2251 2364 +113
============================================
+ Hits 6591 7765 +1174
- Misses 4012 4201 +189
- Partials 1140 1232 +92 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @andygrove , Could you please take a look at this PR and provide some guidance when you have a chance? I've refactored the comparison expressions (GreaterThan, LessThan, IsNull, In, etc.) following the pattern discussed in #2019. In the meantime, I'll continue working on refactoring the remaining expressions from the list. Thanks! |
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
Thanks for the first contribution @CuteChuanChuan. First feedback provided below.
- Remove duplicated utility functions (createUnaryExpr, createBinaryExpr, optExprWithInfo) - Remove empty ComparisonBase trait
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
Thanks for the contribution @CuteChuanChuan. I think that this PR can be approved once all feedback has been addressed.
- Move in function logic into CometIn.convert
|
@mbutrovich could you re-review? |
| (builder, binaryExpr) => builder.setBitwiseAnd(binaryExpr)) | ||
|
|
||
| case In(value, list) => | ||
| in(expr, value, list, inputs, binding, negate = false) |
There was a problem hiding this comment.
I understand now why you brought that in function over to comparisons.scala. It was used here, and in InSet and Not(In).
I think we should handle those two expressions in this PR, and bring in over to comparisons.scala so the logic isn't duplicated in two places.
- Extract shared in function to ComparisonUtils for reuse by In, InSet, and Not(In) expressions - Move NotIn and InSet to comparisons.scala - Add CometInSet to exprSerdeMap and keep Not(In) pattern matching for nested structure
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
LTGM, thanks for your patience on this one @CuteChuanChuan!
4 participants
Which issue does this PR close?
Part of #2019
Rationale for this change
See #2019 for rationale.
What changes are included in this PR?
Moved comparison expressions to separate file (comparison.scala)
How are these changes tested?
There are no functional changes, so it relies on existing tests.
Details: