You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/07/21 07:35:34 UTC
[GitHub] [spark] dgd-contributor opened a new pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
dgd-contributor opened a new pull request #33459:
URL: https://github.com/apache/spark/pull/33459
… more than 64 invalid characters and return wrong value on overflow
### What changes were proposed in this pull request?
1/ conv() have inconsistency in behavior where the returned value is different above the 64 char threshold.
```
scala> spark.sql("select conv(repeat('?', 64), 10, 16)").show
+---------------------------+
|conv(repeat(?, 64), 10, 16)|
+---------------------------+
| 0|
+---------------------------+
scala> spark.sql("select conv(repeat('?', 65), 10, 16)").show // which should be 0
+---------------------------+
|conv(repeat(?, 65), 10, 16)|
+---------------------------+
| FFFFFFFFFFFFFFFF|
+---------------------------+
scala> spark.sql("select conv(repeat('?', 65), 10, -16)").show // which should be 0
+----------------------------+
|conv(repeat(?, 65), 10, -16)|
+----------------------------+
| -1|
+----------------------------+
scala> spark.sql("select conv(repeat('?', 64), 10, -16)").show
+----------------------------+
|conv(repeat(?, 64), 10, -16)|
+----------------------------+
| 0|
+----------------------------+
```
2/ conv should return result equal to -1 in base toBase
```
scala> spark.sql(select conv('aaaaaaa0aaaaaaa0a', 16, 10)).show // which should be 18446744073709551615
+-------------------------------+
|conv(aaaaaaa0aaaaaaa0a, 16, 10)|
+-------------------------------+
| 12297828695278266890|
+-------------------------------+
```
### Why are the changes needed?
Bug fix
### Does this PR introduce _any_ user-facing change?
change in result of conv() function
### How was this patch tested?
add test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884471194
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/141411/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884640268
Kubernetes integration test unable to build dist.
exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45978/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r677401451
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,12 +49,14 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
+ val bound = java.lang.Long.divideUnsigned(-1 - radix, radix)
var i = fromPos
while (i < value.length && value(i) >= 0) {
- if (v >= bound) {
Review comment:
ok now I got it, thanks for the explanation! I think it's better to simplify the code which makes it easier to write code comments. The 2 conditions of overflow are very different.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884763265
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/141459/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-886025167
@cloud-fan hey, can you review this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-888264196
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46286/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-888425879
**[Test build #141773 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141773/testReport)** for PR 33459 at commit [`3de094e`](https://github.com/apache/spark/commit/3de094edf8c7b619a6a88862b278162909cd88ac).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r678202783
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,11 +49,22 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
Review comment:
Done!
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,11 +49,22 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
+ val bound = java.lang.Long.divideUnsigned(-1 - radix, radix)
var i = fromPos
while (i < value.length && value(i) >= 0) {
+ // if v < 0, which mean its bit presentation is start with 1, so v * radix will cause
Review comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884345849
**[Test build #141430 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141430/testReport)** for PR 33459 at commit [`69c61bb`](https://github.com/apache/spark/commit/69c61bb3deb1de8de23714f0b5c5535e62a816da).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r674458518
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/MathFunctionsSuite.scala
##########
@@ -218,6 +218,20 @@ class MathFunctionsSuite extends QueryTest with SharedSparkSession {
checkAnswer(df.select(conv('num, 16, -10)), Row("-1"))
}
+ test("SPARK-36229 inconsistently behaviour where returned value is above the 64 char threshold") {
+ val df = Seq(("?"*64), ("?"*65), ("a"*4 + "?"*60), ("a"*4 + "?"*61)).toDF("num")
Review comment:
nit:
```suggestion
val df = Seq(("?" * 64), ("?" * 65), ("a" * 4 + "?" * 60), ("a" * 4 + "?" * 61)).toDF("num")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-885159591
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-888446733
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/141773/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884425121
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/45948/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-883976255
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r676684793
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -89,6 +91,10 @@ object NumberConverter {
var i = fromPos
while (i < value.length) {
value(i) = Character.digit(value(i), radix).asInstanceOf[Byte]
+ // if invalid characters are found, it no need to convert the suffix starting there
+ if (value(i) < 0) {
Review comment:
No we don't need to do that, Character.digit(value(i), radix).asInstanceOf[Byte] will return -1 if the character is invalid
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r676539297
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -89,6 +91,10 @@ object NumberConverter {
var i = fromPos
while (i < value.length) {
value(i) = Character.digit(value(i), radix).asInstanceOf[Byte]
+ // if invalid characters are found, it no need to convert the suffix starting there
+ if (value(i) < 0) {
Review comment:
do we need to do `value(i) = -1` here? Otherwise the doc is wrong: `..., convert it to -1 and ...`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r678078124
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -112,19 +127,19 @@ object NumberConverter {
var (negative, first) = if (n(0) == '-') (true, 1) else (false, 0)
// Copy the digits in the right side of the array
- val temp = new Array[Byte](64)
+ val temp = new Array[Byte](Math.max(n.length, 64))
var v: Long = -1
- if ((n.length == 65 && negative) || n.length <= 64) {
Review comment:
now what happens if n.length > 65?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r676691828
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,12 +49,14 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
+ val bound = java.lang.Long.divideUnsigned(-1 - radix, radix)
Review comment:
note that when treating Long value as unsigned long value, as negative value will start with bit 1, negative value will be greater than positive values.
the presentation of unsigned long type is from 0000000000000000000000000000000000000000000000000000000000000000 (equal to 0 in long type)
to
1111111111111111111111111111111111111111111111111111111111111111 (equal to -1 in long type)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r677664285
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,12 +49,14 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
+ val bound = java.lang.Long.divideUnsigned(-1 - radix, radix)
var i = fromPos
while (i < value.length && value(i) >= 0) {
- if (v >= bound) {
Review comment:
Yea that would be better. But for this fix that needs to backport, I think changing less code and adding more code comments is safer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r676686936
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,12 +49,14 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
+ val bound = java.lang.Long.divideUnsigned(-1 - radix, radix)
Review comment:
bound is the max value which can multiply by radix without overflow when we treat Long value as Unsigned Long value
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884263667
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884571766
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/141430/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r678192485
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -112,19 +127,19 @@ object NumberConverter {
var (negative, first) = if (n(0) == '-') (true, 1) else (false, 0)
// Copy the digits in the right side of the array
- val temp = new Array[Byte](64)
+ val temp = new Array[Byte](Math.max(n.length, 64))
var v: Long = -1
- if ((n.length == 65 && negative) || n.length <= 64) {
Review comment:
if n.length > 65, there are 2 cases can happen
case 1:
input string have format like: `valid Input String + invalid Character + something` and the valid input string can converted without overflow. it will ignore the suffix starts from the invalid character
example: `conv( ("a" * 4 + "?" * 60), 16, 10)` will have result as 43690 (as in new test case)
case 2:
input string is a valid input string => it will over flow and return max unsigned long value converted to toBase
example: `conv( "aaaaaaa0aaaaaaa0aaaaaaa0aaaaaaa0aaaaaaa0aaaaaaa0aaaaaaa0aaaaaaa0aaaaaaa0", 16, 10)` will result 18446744073709551615 (this is from test case from SPARK-33428)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884134466
Jenkins test this please
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884425121
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/45948/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-883976255
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r676634647
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,12 +49,14 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
+ val bound = java.lang.Long.divideUnsigned(-1 - radix, radix)
Review comment:
I'm not very familiar with this code. What does `bound` mean here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r677357893
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,12 +49,14 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
+ val bound = java.lang.Long.divideUnsigned(-1 - radix, radix)
var i = fromPos
while (i < value.length && value(i) >= 0) {
- if (v >= bound) {
Review comment:
we can simplify the code as
```
if (v < 0) return -1
if (v >= bound) {
...
}
```
but I think its logic is not as clear as the current code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-888233805
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r676688190
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,12 +49,14 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
+ val bound = java.lang.Long.divideUnsigned(-1 - radix, radix)
Review comment:
so if v is greater than bound, v*radix will cause overflow
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r678042820
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,12 +49,14 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
+ val bound = java.lang.Long.divideUnsigned(-1 - radix, radix)
var i = fromPos
while (i < value.length && value(i) >= 0) {
- if (v >= bound) {
Review comment:
Okay, I've refactor code for changing less original code and adding more comments. Can you re-review? @cloud-fan
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884137173
**[Test build #141411 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141411/testReport)** for PR 33459 at commit [`69c61bb`](https://github.com/apache/spark/commit/69c61bb3deb1de8de23714f0b5c5535e62a816da).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-888266508
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/141757/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r677354506
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,12 +49,14 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
+ val bound = java.lang.Long.divideUnsigned(-1 - radix, radix)
var i = fromPos
while (i < value.length && value(i) >= 0) {
- if (v >= bound) {
Review comment:
1. Because v*radix >= v*2 (as radix >= 2), so if v is negative, which mean its bit presentation is start with 1, so if we multiply it by 2, it will cause overflow.
2. v >= bound is sufficient to v*radix + radix will cause overflow. But we need to check that v*radix + value(i) is overflow or not and v*radix + value(i) <= v*radix + radix
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r678066825
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -89,6 +100,10 @@ object NumberConverter {
var i = fromPos
while (i < value.length) {
value(i) = Character.digit(value(i), radix).asInstanceOf[Byte]
+ // if invalid characters are found, it no need to convert the suffix starting there
+ if (value(i) < 0) {
Review comment:
maybe an assert is too much, bu shall we do `if (value(i) == -1)` here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r677483113
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -89,6 +91,10 @@ object NumberConverter {
var i = fromPos
while (i < value.length) {
value(i) = Character.digit(value(i), radix).asInstanceOf[Byte]
+ // if invalid characters are found, it no need to convert the suffix starting there
+ if (value(i) < 0) {
Review comment:
if you like, but I don't think it needed since Character.digit(value(i), radix) is a simple function and it guaranteed to return -1 if character is invalid in its docs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r676686936
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,12 +49,14 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
+ val bound = java.lang.Long.divideUnsigned(-1 - radix, radix)
Review comment:
bound is the max value which can multiply by radix and plus a value < radix without overflow when we treat Long value as Unsigned Long value, so if v is greater than bound, v*radix + value(i) will cause overflow
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r678063123
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,11 +49,22 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
Review comment:
```suggestion
// bound will always be positive since radix >= 2
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r677354506
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,12 +49,14 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
+ val bound = java.lang.Long.divideUnsigned(-1 - radix, radix)
var i = fromPos
while (i < value.length && value(i) >= 0) {
- if (v >= bound) {
Review comment:
1. Because `v*radix >= v*2` (as `radix >= 2`), so if v is negative, which mean its bit presentation is start with 1, so if we multiply it by 2, it will cause overflow.
2. `v >= bound` is sufficient to `v*radix + radix` will cause overflow. But we need to check that `v*radix + value(i)` is overflow or not and `v*radix + value(i) <= v*radix + radix`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r678064856
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,11 +49,22 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
+ val bound = java.lang.Long.divideUnsigned(-1 - radix, radix)
var i = fromPos
while (i < value.length && value(i) >= 0) {
+ // if v < 0, which mean its bit presentation is start with 1, so v * radix will cause
+ // overflow since radix is greater than 2
+ if (v < 0) {
+ return -1
+ }
+ // check if v greater than bound
+ // if v is greater than bound, v * radix + radix will cause overflow.
Review comment:
```suggestion
// if v is greater than bound, v * radix + radix may cause overflow.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884257268
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/45928/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r676686936
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,12 +49,14 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
+ val bound = java.lang.Long.divideUnsigned(-1 - radix, radix)
Review comment:
bound is the max value which can multiply by radix without overflow when we treat Long value as Unsigned Long value, so if v is greater than bound, v*radix will cause overflow
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,12 +49,14 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
+ val bound = java.lang.Long.divideUnsigned(-1 - radix, radix)
Review comment:
so if v is greater than bound, v*radix will cause overflow
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884763265
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/141459/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884345849
**[Test build #141430 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141430/testReport)** for PR 33459 at commit [`69c61bb`](https://github.com/apache/spark/commit/69c61bb3deb1de8de23714f0b5c5535e62a816da).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884761168
**[Test build #141459 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141459/testReport)** for PR 33459 at commit [`26496d0`](https://github.com/apache/spark/commit/26496d04be98a0b91e7e8d87934574c36a7dddb0).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-888079275
**[Test build #141757 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141757/testReport)** for PR 33459 at commit [`71b94ee`](https://github.com/apache/spark/commit/71b94ee6dc1a7c43cb2c697ff855f320f24b4a9b).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r676684793
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -89,6 +91,10 @@ object NumberConverter {
var i = fromPos
while (i < value.length) {
value(i) = Character.digit(value(i), radix).asInstanceOf[Byte]
+ // if invalid characters are found, it no need to convert the suffix starting there
+ if (value(i) < 0) {
Review comment:
Character.digit(value(i), radix).asInstanceOf[Byte] will return -1 if the character is invalid
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r674466717
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/MathFunctionsSuite.scala
##########
@@ -218,6 +218,20 @@ class MathFunctionsSuite extends QueryTest with SharedSparkSession {
checkAnswer(df.select(conv('num, 16, -10)), Row("-1"))
}
+ test("SPARK-36229 inconsistently behaviour where returned value is above the 64 char threshold") {
+ val df = Seq(("?"*64), ("?"*65), ("a"*4 + "?"*60), ("a"*4 + "?"*61)).toDF("num")
Review comment:
Updated! Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884257268
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/45928/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r677489071
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,12 +49,14 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
+ val bound = java.lang.Long.divideUnsigned(-1 - radix, radix)
var i = fromPos
while (i < value.length && value(i) >= 0) {
- if (v >= bound) {
Review comment:
I think the root of this misunderstanding problem is we treat a long value as an unsigned long value. I think we should introduce a new class of unsigned long value to provide the needed function such as comparing, check for overflow...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884641172
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/45978/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r678203059
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -89,6 +100,10 @@ object NumberConverter {
var i = fromPos
while (i < value.length) {
value(i) = Character.digit(value(i), radix).asInstanceOf[Byte]
+ // if invalid characters are found, it no need to convert the suffix starting there
+ if (value(i) < 0) {
Review comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-888266508
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/141757/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan closed pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #33459:
URL: https://github.com/apache/spark/pull/33459
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r678067753
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -112,19 +127,19 @@ object NumberConverter {
var (negative, first) = if (n(0) == '-') (true, 1) else (false, 0)
// Copy the digits in the right side of the array
- val temp = new Array[Byte](64)
+ val temp = new Array[Byte](Math.max(n.length, 64))
var v: Long = -1
- if ((n.length == 65 && negative) || n.length <= 64) {
- var i = 1
- while (i <= n.length - first) {
- temp(temp.length - i) = n(n.length - i)
- i += 1
- }
- char2byte(fromBase, temp.length - n.length + first, temp)
- // Do the conversion by going through a 64 bit integer
- v = encode(fromBase, temp.length - n.length + first, temp)
+ var i = 1
+ while (i <= n.length - first) {
Review comment:
not related to this PR, shall we use `System.arrayCopy`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884230370
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45928/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884471194
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/141411/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-885159591
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r678405214
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -112,19 +127,19 @@ object NumberConverter {
var (negative, first) = if (n(0) == '-') (true, 1) else (false, 0)
// Copy the digits in the right side of the array
- val temp = new Array[Byte](64)
+ val temp = new Array[Byte](Math.max(n.length, 64))
var v: Long = -1
- if ((n.length == 65 && negative) || n.length <= 64) {
Review comment:
yes, MySQL have the same behavior
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-888307413
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/46286/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884319587
ok to test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884137173
**[Test build #141411 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141411/testReport)** for PR 33459 at commit [`69c61bb`](https://github.com/apache/spark/commit/69c61bb3deb1de8de23714f0b5c5535e62a816da).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-888079275
**[Test build #141757 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141757/testReport)** for PR 33459 at commit [`71b94ee`](https://github.com/apache/spark/commit/71b94ee6dc1a7c43cb2c697ff855f320f24b4a9b).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r678064120
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,11 +49,22 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
Review comment:
We can also mention that `-1` is the largest unsigned long.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884425088
Kubernetes integration test unable to build dist.
exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45948/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884315208
> Can you check the result with MySQL? This function comes from MySQL and I think it's better to behave the same as MySQL here.
yes, I have check the result with MySQL and the goal of this pull request is to reproduce the behavior of conv in MySQL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884571766
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/141430/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884038113
@cloud-fan is test broken?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r676704901
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,12 +49,14 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
+ val bound = java.lang.Long.divideUnsigned(-1 - radix, radix)
var i = fromPos
while (i < value.length && value(i) >= 0) {
- if (v >= bound) {
+ // check if v greater than bound
+ if (v >= bound || v < 0) {
// Check for overflow
- if (java.lang.Long.divideUnsigned(-1 - value(i), radix) < v) {
+ if (java.lang.Long.divideUnsigned(-1 - value(i), radix) < v || v < 0) {
Review comment:
this condition is to check that v * radix + value(i) will be smaller than the max value in unsigned long type ( which is equal to -1 in long type). Because radix is >= 2, java.lang.Long.divideUnsigned(-1 - value(i), radix) will be a positive value (with its bit presentation start with 0), so we can compare it with v as java.lang.Long.divideUnsigned(-1 - value(i), radix) < v || v < 0. Note that if v < 0, its bit presentation is start with 1 and will be greater than a positive value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884213274
Can you check the result with MySQL? This function comes from MySQL and I think it's better to behave the same as MySQL here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884625492
**[Test build #141459 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141459/testReport)** for PR 33459 at commit [`26496d0`](https://github.com/apache/spark/commit/26496d04be98a0b91e7e8d87934574c36a7dddb0).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884641172
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/45978/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-888233805
**[Test build #141773 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141773/testReport)** for PR 33459 at commit [`3de094e`](https://github.com/apache/spark/commit/3de094edf8c7b619a6a88862b278162909cd88ac).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r674466634
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -89,6 +91,10 @@ object NumberConverter {
var i = fromPos
while (i < value.length) {
value(i) = Character.digit(value(i), radix).asInstanceOf[Byte]
+ // if invalid characters are found, it no need to convert the suffix starting there
Review comment:
Updated! Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r674146391
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -89,6 +91,10 @@ object NumberConverter {
var i = fromPos
while (i < value.length) {
value(i) = Character.digit(value(i), radix).asInstanceOf[Byte]
+ // if invalid characters are found, it no need to convert the suffix starting there
Review comment:
can we update the method doc as well? it currently says `Convert invalid characters to -1.`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-888124972
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46271/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-888164397
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/46271/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r676691828
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,12 +49,14 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
+ val bound = java.lang.Long.divideUnsigned(-1 - radix, radix)
Review comment:
note that when treat Long value as unsigned long value, as negative value will start with bit 1, negative value will be greater than positive values.
the presentation of unsigned long type is from 0000000000000000000000000000000000000000000000000000000000000000 (equal to 0 in long type)
to
1111111111111111111111111111111111111111111111111111111111111111 (equal to -1 in long type)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r678334360
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -112,19 +127,19 @@ object NumberConverter {
var (negative, first) = if (n(0) == '-') (true, 1) else (false, 0)
// Copy the digits in the right side of the array
- val temp = new Array[Byte](64)
+ val temp = new Array[Byte](Math.max(n.length, 64))
var v: Long = -1
- if ((n.length == 65 && negative) || n.length <= 64) {
Review comment:
For case 1, does MySQL have the same behavior?
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -112,19 +127,19 @@ object NumberConverter {
var (negative, first) = if (n(0) == '-') (true, 1) else (false, 0)
// Copy the digits in the right side of the array
- val temp = new Array[Byte](64)
+ val temp = new Array[Byte](Math.max(n.length, 64))
var v: Long = -1
- if ((n.length == 65 && negative) || n.length <= 64) {
Review comment:
For case 1, does MySQL have the same behavior?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884468624
**[Test build #141411 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141411/testReport)** for PR 33459 at commit [`69c61bb`](https://github.com/apache/spark/commit/69c61bb3deb1de8de23714f0b5c5535e62a816da).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-888164358
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46271/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-888300296
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46286/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884570265
**[Test build #141430 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141430/testReport)** for PR 33459 at commit [`69c61bb`](https://github.com/apache/spark/commit/69c61bb3deb1de8de23714f0b5c5535e62a816da).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-888164397
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/46271/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r678192485
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -112,19 +127,19 @@ object NumberConverter {
var (negative, first) = if (n(0) == '-') (true, 1) else (false, 0)
// Copy the digits in the right side of the array
- val temp = new Array[Byte](64)
+ val temp = new Array[Byte](Math.max(n.length, 64))
var v: Long = -1
- if ((n.length == 65 && negative) || n.length <= 64) {
Review comment:
if n.length > 65, there are 2 case can happen
case 1:
input string have format like: `valid Input String + invalid Character + something` and the valid input string can converted without overflow. it will ignore the suffix starts from the invalid character
example: `conv( ("a" * 4 + "?" * 60), 16, 10)` will have result as 43690 (as in new test case)
case 2:
input string is a valid input string => it will over flow and return max unsigned long value converted to toBase
example: `conv( "aaaaaaa0aaaaaaa0aaaaaaa0aaaaaaa0aaaaaaa0aaaaaaa0aaaaaaa0aaaaaaa0aaaaaaa0", 16, 10)` will result 18446744073709551615 (this is from test case from SPARK-33428)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r677340069
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,12 +49,14 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
+ val bound = java.lang.Long.divideUnsigned(-1 - radix, radix)
var i = fromPos
while (i < value.length && value(i) >= 0) {
- if (v >= bound) {
Review comment:
nit: seems we can simplify the code
```
if (v < 0) return -1
if (v >= bound) {
...
}
```
However, this block of code is really hard to understand and we probably need to add some code comments.
1. why `v < 0` means overflow?
2. why we need to check overflow again? Isn't `v >= bound` sufficient to prove overflow?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dgd-contributor commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
dgd-contributor commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r677489071
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,12 +49,14 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
+ val bound = java.lang.Long.divideUnsigned(-1 - radix, radix)
var i = fromPos
while (i < value.length && value(i) >= 0) {
- if (v >= bound) {
Review comment:
I think the root of this misunderstanding problem is we treat a long value as an unsigned long value. I think we should introduce a new class of unsigned long value to provide the needed function such as comparing, checking for overflow...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-888442318
thanks, merging to master/3.2!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-888307413
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/46286/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884263667
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r677340818
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -89,6 +91,10 @@ object NumberConverter {
var i = fromPos
while (i < value.length) {
value(i) = Character.digit(value(i), radix).asInstanceOf[Byte]
+ // if invalid characters are found, it no need to convert the suffix starting there
+ if (value(i) < 0) {
Review comment:
then shall we add `assert(value(i) == -1)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #33459:
URL: https://github.com/apache/spark/pull/33459#discussion_r678064410
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/NumberConverter.scala
##########
@@ -49,11 +49,22 @@ object NumberConverter {
*/
private def encode(radix: Int, fromPos: Int, value: Array[Byte]): Long = {
var v: Long = 0L
- val bound = java.lang.Long.divideUnsigned(-1 - radix, radix) // Possible overflow once
+ // bound will always positive since radix >= 2
+ val bound = java.lang.Long.divideUnsigned(-1 - radix, radix)
var i = fromPos
while (i < value.length && value(i) >= 0) {
+ // if v < 0, which mean its bit presentation is start with 1, so v * radix will cause
Review comment:
```suggestion
// if v < 0, which mean its bit presentation starts with 1, so v * radix will cause
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with more than 64 invalid characters and return wrong value on overflow
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884625492
**[Test build #141459 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141459/testReport)** for PR 33459 at commit [`26496d0`](https://github.com/apache/spark/commit/26496d04be98a0b91e7e8d87934574c36a7dddb0).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33459: [SPARK-36229][SQL] conv() inconsistently handles invalid strings with…
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33459:
URL: https://github.com/apache/spark/pull/33459#issuecomment-884196036
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45928/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org