-
Notifications
You must be signed in to change notification settings - Fork 28.1k
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-8227][SQL]Add function unhex #7113
Conversation
Merged build triggered. |
Merged build started. |
Test build #36100 has started for PR 7113 at commit |
Test build #36100 has finished for PR 7113 at commit
|
Merged build finished. Test FAILed. |
Merged build triggered. |
Merged build started. |
Test build #36109 has started for PR 7113 at commit |
Test build #36109 has finished for PR 7113 at commit
|
Merged build finished. Test FAILed. |
Merged build triggered. |
Merged build started. |
Test build #36111 has started for PR 7113 at commit |
Test build #36111 has finished for PR 7113 at commit
|
Merged build finished. Test FAILed. |
if (num == null) { | ||
null | ||
} else { | ||
unhex(num.asInstanceOf[UTF8String].toString) |
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.
Converting UTF8String to String is slow, please use getBytes
Merged build triggered. |
Merged build started. |
Test build #36223 has started for PR 7113 at commit |
Test build #36223 has finished for PR 7113 at commit
|
Merged build finished. Test FAILed. |
reset this please |
case class UnHex(child: Expression) | ||
extends UnaryExpression with AutoCastInputTypes with Serializable { | ||
|
||
override def expectedChildTypes: Seq[DataType] = Seq(StringType) |
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.
@davies this could be changed to ByteType
, couldn't it? This avoids a cast, if someone calls this with a byte array.
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.
hex() return StringType, so unhex() should take StringType, not BinaryType.
Merged build triggered. |
Merged build started. |
Test build #36262 has started for PR 7113 at commit |
Test build #36262 has finished for PR 7113 at commit
|
Merged build finished. Test PASSed. |
} | ||
|
||
private def toDigit(b: Byte): Byte = { | ||
val digit = hexDigits(b) |
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 should be a val
of UnHex.
Merged build triggered. |
Merged build started. |
Test build #36320 has started for PR 7113 at commit |
Test build #36320 has finished for PR 7113 at commit
|
Merged build finished. Test PASSed. |
LGTM, merging this into master, thanks! |
* Performs the inverse operation of HEX. | ||
* Resulting characters are returned as a byte array. | ||
*/ | ||
case class UnHex(child: Expression) extends UnaryExpression with Serializable { |
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 should just implement ExpectsInputTypes, and use BinaryType as the only type.
There will be a rule in #7175 to do implicit type casting from null type to expected type(s).
cc @chenghao-intel @adrian-wang