-
Notifications
You must be signed in to change notification settings - Fork 28.4k
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
[SPARK-8238][SPARK-8239][SPARK-8242][SPARK-8243][SPARK-8268][SQL]Add ascii/base64/unbase64/encode/decode functions #6843
Conversation
Test build #34986 has finished for PR 6843 at commit
|
@adrian-wang @zhichao-li can you review the code for me? |
Test build #34990 has finished for PR 6843 at commit
|
} | ||
} | ||
|
||
override def toString: String = s"ascii($child)" |
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.
Upper case these all?
Test build #35132 has finished for PR 6843 at commit
|
@rxin, any more comments on this? |
} | ||
} | ||
|
||
override def toString: String = s"ASCII($child)" |
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 don't think this is necessary since it is a case class?
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.
btw i don't think we should be making everything uppercase..
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 the registered function name will make more sense, than giving the user a class name. What do you think?
Test build #35522 has finished for PR 6843 at commit
|
a5e89b6
to
da6a788
Compare
Test build #36330 has finished for PR 6843 at commit
|
da6a788
to
947a88a
Compare
Test build #36341 has finished for PR 6843 at commit
|
947a88a
to
9d6f9f4
Compare
Test build #36500 has finished for PR 6843 at commit
|
Test build #36501 has finished for PR 6843 at commit
|
* (one of 'US-ASCII', 'ISO-8859-1', 'UTF-8', 'UTF-16BE', 'UTF-16LE', 'UTF-16'). | ||
* If either argument is null, the result will also be null. (As of Hive 0.12.0.). | ||
*/ | ||
case class Decode(bin: Expression, charset: Expression) extends Expression with ExpectsInputTypes { |
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 make this extend BinaryExpression? You can just define def bin = left, and def charset = right.
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.
Actually that's my intention, as I think the parameters is asymmetric semantically. Not sure if you are thinking the code impovement like #7157?
/** | ||
* Decodes the first argument into a String using the provided character set | ||
* (one of 'US-ASCII', 'ISO-8859-1', 'UTF-8', 'UTF-16BE', 'UTF-16LE', 'UTF-16'). | ||
* If either argument is null, the result will also be null. (As of Hive 0.12.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.
remove "As of Hive 0.12.0"
I'm going to merge this first. Please submit a cleanup pr. |
Add
ascii
,base64
,unbase64
,encode
anddecode
expressions.