-
Notifications
You must be signed in to change notification settings - Fork 217
chore: Introduce exprHandlers
map in QueryPlanSerde
#1903
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
Conversation
@@ -127,7 +127,7 @@ The following Spark expressions are currently available. Any known compatibility | |||
| Log10 | | | |||
| Pow | | | |||
| Round | | | |||
| Signum | Signum does not differentiate between `0.0` and `-0.0` | |
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.
This was fixed in #1889
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1903 +/- ##
============================================
+ Coverage 56.12% 59.05% +2.93%
- Complexity 976 1140 +164
============================================
Files 119 130 +11
Lines 11743 12826 +1083
Branches 2251 2388 +137
============================================
+ Hits 6591 7575 +984
- Misses 4012 4033 +21
- Partials 1140 1218 +78 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@Kontinuation you may be interested in this since it is a step towards making Comet more extensible |
/** | ||
* Mapping of Spark expression class to Comet expression handler. | ||
*/ | ||
private val exprHandlers: Map[Class[_], CometExpressionSerde] = Map( |
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.
Thats neat
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.
Prob we can call exprMappers
or sparkToCometExprMapper
?
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.
I renamed to exprSerdeMap
since the map holds CometExpressionSerde
instances. I am open to other suggestions though,
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.
lgtm thanks @andygrove
I assume this is not the end of it and we would be enhancing this as we go? |
Yes, this is just a first step.
Yes, this will help with generating documentation from the source code rather than manually updating our docs for supported expressions. |
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.
Nice change @andygrove!
Which issue does this PR close?
N/A
Rationale for this change
We recently started to make QueryPlanSerde more maintainable by moving expression serde logic into individual classes or objects per expression using a new
CometExpressionSerde
trait. This was meant as a step towards having a registry of supported expressions to reduce some of the boilerplate code and to enable automatic generation of documentation for supported expressions.What changes are included in this PR?
Remove custom matching of some expressions and use a registry approach.
How are these changes tested?