-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-8209[SQL]Add function conv #6872
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
|
Test build #35099 has finished for PR 6872 at commit
|
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 think you should rewrite this part in a more functional way.
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.
And be private[sql] object
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.
Sorry, I mean trait.
|
Test build #35101 has finished for PR 6872 at commit
|
|
Test build #35215 has finished for PR 6872 at commit
|
|
retest this please |
|
Test build #35216 has finished for PR 6872 at commit
|
|
Test build #35223 has finished for PR 6872 at commit
|
|
Test build #35227 has finished for PR 6872 at commit
|
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.
Ternary is a very rare case, and probably most of the code in this class are needed to be overrided for different TernaryExpressions, so how about move the logic into the Conv?
|
Test build #35644 has finished for PR 6872 at commit
|
|
retest this please. |
|
Test build #35648 has finished for PR 6872 at commit
|
|
retest this please. |
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.
Probably more user cases of fromBase and the toBase will be represented as the real value, than the column names? What do you think?
@rxin we actually have some cases like this, do you think if we need to support both constant string values and column names for cases like this?
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.
yes i agree. i think we can always use real value here
|
Test build #37563 has finished for PR 6872 at commit
|
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.
we need a lot more test cases here. please think hard about corner cases.
|
@zhichao-li this is really close. Let's add some test cases and then merge it. |
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.
can you add documentation for Conv itself?
|
LGTM |
|
Test build #37607 has finished for PR 6872 at commit
|
|
Thanks. I've merged this. |
cc @chenghao-intel @adrian-wang