-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
[FLINK-9853] [table] Add HEX support #6337
Conversation
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.
some little suggestion
@@ -400,6 +400,12 @@ trait ImplicitExpressionOperations { | |||
* numeric is null. E.g. "4" leads to "100", "12" leads to "1100". | |||
*/ | |||
def bin() = Bin(expr) | |||
/** |
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.
please insert a new blank line
@@ -182,4 +184,6 @@ object ScalarFunctions { | |||
|
|||
new String(data) | |||
} | |||
|
|||
def hex(x: String): String = Hex.encodeHexString(x.getBytes) |
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.
add the doc for the API looks better to me
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.
thanks for review, @yanghua , fixed
+1, from my side |
Thanks for your PR. +1 to merge. |
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.
Thanks for the contribution @xueyumusic.
I am a bit confuse regarding the documentation. Seems like both "20"
and 20L
are valid input to the HEX function. It's worth to point out the difference IMO, especially they return different results: e.g.
10.hex()
gives "a"
and "10".hex()
gives "3130"
+1 to merge once documentation is fixed.
docs/dev/table/sql.md
Outdated
{% endhighlight %} | ||
</td> | ||
<td> | ||
<p>Returns a string representation of an integer numeric value or a string in hex format. Returns null if numeric is null. E.g. "20" leads to "14", "100" leads to "64", "hello,world" leads to "68656c6c6f2c776f726c64".</p> |
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.
maybe consider using backtick for the number literals? e.g. "20"
--> `20`
.
Otherwise its confusing whether we should pass in stringify integer or just integer values.
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.
Thanks @walterddr , I clarify the doc adding explicit explain for numeric
or for string
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.
great change and explanations. actually my point was to have "20"
--> `20`
with specifically the backtick. since backtick quoted strings gets format differently in Flink doc page and it will be immediately clear that this is a numeral literal.
docs/dev/table/tableApi.md
Outdated
{% endhighlight %} | ||
</td> | ||
<td> | ||
<p>Returns a string representation of an integer numeric value or a string in hex format. Returns null if numeric is null. E.g. "20" leads to "14", "100" leads to "64", "hello,world" leads to "68656c6c6f2c776f726c64".</p> |
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.
same here.
@@ -406,6 +406,13 @@ trait ImplicitExpressionOperations { | |||
*/ | |||
def bin() = Bin(expr) | |||
|
|||
/** | |||
* Returns a string representation of an integer numeric value in hex format. Returns null if | |||
* numeric is null. E.g. "20" leads to "14", "100" leads to "64", "hello,world" leads to |
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.
just remove the "
if it is numeric
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.
Thank you @xueyumusic. I added one comment about the result string.
override private[flink] def resultType: TypeInformation[_] = BasicTypeInfo.STRING_TYPE_INFO | ||
|
||
override private[flink] def validateInput(): ValidationResult = child.resultType match { | ||
case _: IntegerTypeInfo[_] => |
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.
Rely on TypeCheckUtils.isIntegerFamily
and TypeCheckUtils#isString
instead.
'f2.hex(), | ||
"f2.hex()", | ||
"HEX(f2)", | ||
"2a") |
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.
MySQL returns upper-case letters. Should we do this as well?
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, we should do it as well. Thanks @twalthr , I updated the code
Thank you for the quick update @xueyumusic. I will merge this. |
@xueyumusic for future PRs use |
This closes apache#6337. (cherry picked from commit ab1d1df)
What is the purpose of the change
This PR propose to add HEX in table api and sql, the syntax like mysql, which could take int or string arguments. For a integer argument N, it returns a hexadecimal string representation of the value of N. For a string argument str, it returns a hexadecimal string representation of str where each byte of each character in str is converted to two hexadecimal digits.
Syntax:
HEX(100) = 64
HEX('This is a test String.') = '546869732069732061207465737420537472696e672e'
Brief change log
Verifying this change
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (no)Documentation