-
Notifications
You must be signed in to change notification settings - Fork 169
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
chore: Add CometEvalMode enum to replace string literals #539
Conversation
These changes were originally part of #509 but it looks like we won't be merging that one for some time |
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 pending ci
def unhexSerde(unhex: Unhex): (Expression, Expression) = { | ||
(unhex.child, Literal(unhex.failOnError)) | ||
} | ||
|
||
def evalMode(c: Cast): CometEvalMode.Value = | ||
CometEvalModeUtil.fromSparkEvalMode(c.evalMode) |
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.
nit: it seems to be that we can keep it as protected
instead of changing to public?
Also evalMode
can be protected
?
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.
That was an unintenional change. I will revert.
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.
@kazuyukitanimura I made these methods protected in all shim versions for consistency
* Add CometEvalMode enum * address feedback
Which issue does this PR close?
N/A
Rationale for this change
Code cleanup.
What changes are included in this PR?
CometEvalMode
How are these changes tested?
Existing tests