[GitHub] xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add ASCII/CHR function for table/sql API
xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add
ASCII/CHR function for table/sql API
URL: https://github.com/apache/flink/pull/6432#discussion_r226506688
##
File path:
flink-libraries/flink-table/src/main/scala/org/apache/flink/table/runtime/functions/ScalarFunctions.scala
##
@@ -252,6 +252,17 @@ object ScalarFunctions {
regexp_extract(str, regex, 0)
}
+ /**
+* Returns a numeric value of the leftmost character of the string str.
+*/
+ def ascii(str: String): Integer = {
+if (str == null || str.equals("")) {
+ 0
Review comment:
Yes, the results are "coincidentally" to be null, but I still suggest to
explicitly return a `null` value instead of a normal result.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add ASCII/CHR function for table/sql API
xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add
ASCII/CHR function for table/sql API
URL: https://github.com/apache/flink/pull/6432#discussion_r226506284
##
File path:
flink-libraries/flink-table/src/main/scala/org/apache/flink/table/runtime/functions/ScalarFunctions.scala
##
@@ -252,6 +252,17 @@ object ScalarFunctions {
regexp_extract(str, regex, 0)
}
+ /**
+* Returns a numeric value of the leftmost character of the string str.
+*/
+ def ascii(str: String): Integer = {
+if (str == null || str.equals("")) {
+ 0
+} else {
+ str.charAt(0).toByte.toInt
Review comment:
Could you explain more about this behavior?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add ASCII/CHR function for table/sql API
xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add
ASCII/CHR function for table/sql API
URL: https://github.com/apache/flink/pull/6432#discussion_r226506162
##
File path:
flink-libraries/flink-table/src/test/scala/org/apache/flink/table/expressions/ScalarFunctionsTest.scala
##
@@ -889,6 +916,69 @@ class ScalarFunctionsTest extends ScalarTypesTestBase {
"")
}
+ @Test
+ def testChr(): Unit = {
+testAllApis(
+ 'f14.chr(),
+ "f14.chr()",
+ "CHR(f14)",
+ "null")
+
+testAllApis(
+ 'f34.chr(),
+ "f34.chr()",
+ "CHR(f34)",
+ "null")
+
+testAllApis(
+ 'f34.chr(),
+ "f34.chr()",
+ "CHR(f34)",
+ "null")
+
+testAllApis(
+ 'f36.chr(),
+ "f36.chr()",
+ "CHR(f36)",
+ "A")
+
+testAllApis(
+ 'f37.chr(),
+ "f37.chr()",
+ "CHR(f37)",
+ "a")
+
+testAllApis(
+ 'f38.chr(),
+ "f38.chr()",
+ "CHR(f38)",
+ "ÿ")
+
+testAllApis(
Review comment:
This is duplicated with a test case above.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add ASCII/CHR function for table/sql API
xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add
ASCII/CHR function for table/sql API
URL: https://github.com/apache/flink/pull/6432#discussion_r225311291
##
File path:
flink-libraries/flink-table/src/main/scala/org/apache/flink/table/runtime/functions/ScalarFunctions.scala
##
@@ -285,4 +296,15 @@ object ScalarFunctions {
*/
def repeat(base: String, n: Int): String = StringUtils.repeat(base, n)
+ /**
+* Returns a character corresponding to the input integer ASCII code.
Review comment:
The Java doc says the method takes an integer ASCII code, but the actual
parameter type is `Long`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add ASCII/CHR function for table/sql API
xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add
ASCII/CHR function for table/sql API
URL: https://github.com/apache/flink/pull/6432#discussion_r225324041
##
File path:
flink-libraries/flink-table/src/test/scala/org/apache/flink/table/expressions/utils/ScalarTypesTestBase.scala
##
@@ -65,6 +65,9 @@ class ScalarTypesTestBase extends ExpressionTestBase {
testData.setField(33, null)
testData.setField(34, 256)
testData.setField(35, "aGVsbG8gd29ybGQ=")
+testData.setField(36, 65)
+testData.setField(37, 97)
Review comment:
`f36` and `f37`, I think we can only keep one of them.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add ASCII/CHR function for table/sql API
xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add
ASCII/CHR function for table/sql API
URL: https://github.com/apache/flink/pull/6432#discussion_r225311291
##
File path:
flink-libraries/flink-table/src/main/scala/org/apache/flink/table/runtime/functions/ScalarFunctions.scala
##
@@ -285,4 +296,15 @@ object ScalarFunctions {
*/
def repeat(base: String, n: Int): String = StringUtils.repeat(base, n)
+ /**
+* Returns a character corresponding to the input integer ASCII code.
Review comment:
The Java doc says the method takes an integer ASCII code, but the actual
parameter type is `Long`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add ASCII/CHR function for table/sql API
xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add
ASCII/CHR function for table/sql API
URL: https://github.com/apache/flink/pull/6432#discussion_r225302431
##
File path:
flink-libraries/flink-table/src/main/scala/org/apache/flink/table/runtime/functions/ScalarFunctions.scala
##
@@ -252,6 +252,17 @@ object ScalarFunctions {
regexp_extract(str, regex, 0)
}
+ /**
+* Returns a numeric value of the leftmost character of the string str.
+*/
+ def ascii(str: String): Integer = {
+if (str == null || str.equals("")) {
+ 0
Review comment:
Should return `null` when `str == null`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add ASCII/CHR function for table/sql API
xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add
ASCII/CHR function for table/sql API
URL: https://github.com/apache/flink/pull/6432#discussion_r225319289
##
File path:
flink-libraries/flink-table/src/main/scala/org/apache/flink/table/runtime/functions/ScalarFunctions.scala
##
@@ -285,4 +296,15 @@ object ScalarFunctions {
*/
def repeat(base: String, n: Int): String = StringUtils.repeat(base, n)
+ /**
+* Returns a character corresponding to the input integer ASCII code.
+*/
+ def chr(ascii: Long): String = {
+if (ascii == null || ascii < 0 || ascii > 255) {
Review comment:
As far as I know, when the input exceeding the range (1 - 255), the behavior
of the `chr()` function differs in some databases. It's fine to return a `null`
value and maybe we should mention that in the documentation.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add ASCII/CHR function for table/sql API
xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add
ASCII/CHR function for table/sql API
URL: https://github.com/apache/flink/pull/6432#discussion_r225322607
##
File path:
flink-libraries/flink-table/src/test/scala/org/apache/flink/table/expressions/ScalarFunctionsTest.scala
##
@@ -889,6 +916,69 @@ class ScalarFunctionsTest extends ScalarTypesTestBase {
"")
}
+ @Test
+ def testChr(): Unit = {
+testAllApis(
+ 'f14.chr(),
+ "f14.chr()",
+ "CHR(f14)",
+ "null")
+
+testAllApis(
+ 'f34.chr(),
+ "f34.chr()",
+ "CHR(f34)",
+ "null")
+
+testAllApis(
Review comment:
Duplicated test case.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add ASCII/CHR function for table/sql API
xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add
ASCII/CHR function for table/sql API
URL: https://github.com/apache/flink/pull/6432#discussion_r225322741
##
File path:
flink-libraries/flink-table/src/test/scala/org/apache/flink/table/expressions/ScalarFunctionsTest.scala
##
@@ -889,6 +916,69 @@ class ScalarFunctionsTest extends ScalarTypesTestBase {
"")
}
+ @Test
+ def testChr(): Unit = {
+testAllApis(
+ 'f14.chr(),
+ "f14.chr()",
+ "CHR(f14)",
+ "null")
+
+testAllApis(
+ 'f34.chr(),
+ "f34.chr()",
+ "CHR(f34)",
+ "null")
+
+testAllApis(
+ 'f34.chr(),
+ "f34.chr()",
+ "CHR(f34)",
+ "null")
+
+testAllApis(
+ 'f36.chr(),
+ "f36.chr()",
+ "CHR(f36)",
+ "A")
+
+testAllApis(
+ 'f37.chr(),
+ "f37.chr()",
+ "CHR(f37)",
+ "a")
+
+testAllApis(
+ 'f38.chr(),
+ "f38.chr()",
+ "CHR(f38)",
+ "ÿ")
+
+testAllApis(
Review comment:
Duplicated
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add ASCII/CHR function for table/sql API
xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add
ASCII/CHR function for table/sql API
URL: https://github.com/apache/flink/pull/6432#discussion_r225307607
##
File path:
flink-libraries/flink-table/src/main/scala/org/apache/flink/table/runtime/functions/ScalarFunctions.scala
##
@@ -252,6 +252,17 @@ object ScalarFunctions {
regexp_extract(str, regex, 0)
}
+ /**
+* Returns a numeric value of the leftmost character of the string str.
+*/
+ def ascii(str: String): Integer = {
+if (str == null || str.equals("")) {
+ 0
+} else {
+ str.charAt(0).toByte.toInt
Review comment:
This may work for ASCII characters, but I'm not sure if it will return
"expected results" for non-ASCII characters. Or it doesn't matter?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add ASCII/CHR function for table/sql API
xccui commented on a change in pull request #6432: [FLINK-9970] [table] Add
ASCII/CHR function for table/sql API
URL: https://github.com/apache/flink/pull/6432#discussion_r225320391
##
File path: docs/dev/table/functions.md
##
@@ -2449,6 +2449,30 @@ REPEAT(string, integer)
+
+
+{% highlight text %}
+ASCII(string)
+{% endhighlight %}
+
+
+Returns a numeric value of the leftmost character of the
string.
Review comment:
Returns the ASCII code value of... (please also update the Javadoc
correspondingly)
As we've reached an agreement on adding the return type, it's better to
mention it here.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
