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/12/10 12:02:08 UTC
[GitHub] [spark] Kwafoor opened a new pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Kwafoor opened a new pull request #34862:
URL: https://github.com/apache/spark/pull/34862
### What changes were proposed in this pull request?
For comparing StringType and IntegerType, the value of string exceed Int.MaxValue, then should thrown exception.
Other wise, the result is null.
For example:
SQL "select cast('2147483648' as int);",which return is null.
### Why are the changes needed?
User needs to know when an exception occurs.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added Unit 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] Kwafoor commented on a change in pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
Kwafoor commented on a change in pull request #34862:
URL: https://github.com/apache/spark/pull/34862#discussion_r767385081
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
##########
@@ -652,7 +652,13 @@ abstract class CastBase extends UnaryExpression with TimeZoneAwareExpression wit
buildCast[UTF8String](_, UTF8StringUtils.toIntExact)
case StringType =>
val result = new IntWrapper()
- buildCast[UTF8String](_, s => if (s.toInt(result)) result.value else null)
+ buildCast[UTF8String](_, s => {
Review comment:
I find this case in user change their HiveSQL to SparkSQL, user always filter column with comparing String and IntegerType.With ANSI this case is right, SparkSQL will thrown Exception, but user unwilling to change their code, so ANSI is often false.And in this case, it is actually return wrong result, I think SparkSQL should remind user where your sql is wrong.If user runs a big and complex query, without thrown exception it is hard to find where the sql is wrong.
--
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 #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-992101709
> when overflow happened SparkSQL return wrong results(null), but user didn't get any remind, I think it is a problem.
The problem has been there for many years, and many operations/functions are affected (math operations, cast, etc.). I don't think it's justified to change string and integer comparison only. ANSI mode is the way to go. If ANSI mode is too strict, we can relax it a bit to make it fit the real-world use cases better.
--
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] Kwafoor commented on pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
Kwafoor commented on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-992072302
> @Kwafoor the default behavior exists for many years. It is hard to change it now. That's also why we introducing the ANSI SQL mode as an alternative option. If users want to detect integer overflow, they can enable the `spark.sql.ansi.enabled`.
I agree with you, but I think SparkSQL should remind user your sql is wrong rather return null.
User may not check their results, it will lead a hug trouble.
ANSI with default true is also not a good idea, it is too strict with cast.
--
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 a change in pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #34862:
URL: https://github.com/apache/spark/pull/34862#discussion_r766840325
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
##########
@@ -652,7 +652,13 @@ abstract class CastBase extends UnaryExpression with TimeZoneAwareExpression wit
buildCast[UTF8String](_, UTF8StringUtils.toIntExact)
case StringType =>
val result = new IntWrapper()
- buildCast[UTF8String](_, s => if (s.toInt(result)) result.value else null)
+ buildCast[UTF8String](_, s => {
+ if (s.toInt(result)) {
+ result.value}
Review comment:
Brace is in the wrong place.
Do we have any reference for what the behavior should be in this case? I'm not sure if null or an exception is correct
--
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] Kwafoor commented on a change in pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
Kwafoor commented on a change in pull request #34862:
URL: https://github.com/apache/spark/pull/34862#discussion_r767382412
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
##########
@@ -652,7 +652,13 @@ abstract class CastBase extends UnaryExpression with TimeZoneAwareExpression wit
buildCast[UTF8String](_, UTF8StringUtils.toIntExact)
case StringType =>
val result = new IntWrapper()
- buildCast[UTF8String](_, s => if (s.toInt(result)) result.value else null)
+ buildCast[UTF8String](_, s => {
+ if (s.toInt(result)) {
+ result.value}
Review comment:
In sparkSQL
and
For in Hive
When user comparing String and IntegerType in sparkSQL with spark.sql.ansi.enabled=false, and the default value of spark.sql.ansi.enabled is false, user didn't get any remind but get a wrong result.
User runs a big and complex sql, it is very hard to find where the result is wrong,So they is hard to fix the wrong sql.
Actually I think may be there is some problem without any check to cast String to int when user comparing String and AtomicType.
And with spark.sql.ansi.enabled=true, SparkSQL disallowed compare String and DecimalType, user from hive to SparkSQL unwilling to change their code.
So I think It should remind User where your sql is not support in SparkSQL even without ansi mode, Because this case is exception, and user should to be reminded.
--
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 #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-992086112
If the value is expected to be large, is it not better to explicitly cast to a larger integer 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] Kwafoor commented on pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
Kwafoor commented on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-992185361
> > But I still think SparkSQL should remind user where you wrong.
>
> This doesn't answer the question of why you only fix string integer comparison here.
I think string integer comparison is common and easier to encounter,I haven't meet the other case.I have just tried, and also encounter problem(long, short, etc).
And I agree with you it's better to not introduce inconsistency to the system.
About other like math(+-*/), it's right about string and integer(SparkSQL cast string to double type).
May be I make a confusion title.I found the issue in comparing String and IntegerType, and the reason is string2int cast overflow.
So I fix issue by thrown exception in cast 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] wangyum commented on pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
wangyum commented on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-990974553
@Kwafoor Please enable `spark.sql.ansi.enabled` if you prefer thrown exception.
--
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 edited a comment on pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
cloud-fan edited a comment on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-992160298
> But I still think SparkSQL should remind user where you wrong.
This doesn't answer the question of why you only fix string integer comparison here.
> Or we have to suggest user better to use cast explicitly
This works for me. We should discourage relying on implicit cast.
--
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] Kwafoor commented on pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
Kwafoor commented on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-992069687
> @Kwafoor, Before we go further, I think it should be best to double check how ANSI defines the behaviour on the overflow of strings <> other types.
> @Kwafoor Please enable `spark.sql.ansi.enabled` if you prefer thrown exception.
With spark.sql.ansi.enabled enable it is right, but ANSI is too strict with cast, user often didn't enable.
Without thrown Exception, when overflow happened SparkSQL return wrong results(null), but user didn't get any remind, I think it is a problem.
Without thrown exception it is also too hard to find where the wrong sql is and fix it.
--
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] Kwafoor commented on a change in pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
Kwafoor commented on a change in pull request #34862:
URL: https://github.com/apache/spark/pull/34862#discussion_r767382412
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
##########
@@ -652,7 +652,13 @@ abstract class CastBase extends UnaryExpression with TimeZoneAwareExpression wit
buildCast[UTF8String](_, UTF8StringUtils.toIntExact)
case StringType =>
val result = new IntWrapper()
- buildCast[UTF8String](_, s => if (s.toInt(result)) result.value else null)
+ buildCast[UTF8String](_, s => {
+ if (s.toInt(result)) {
+ result.value}
Review comment:
In sparkSQL
and
For in Hive
When user comparing String and IntegerType in sparkSQL with spark.sql.ansi.enabled=false, and the default value of spark.sql.ansi.enabled is false, user didn't get any remind but get a wrong result.
User runs a big and complex sql, it is very hard to find where the result is wrong,So they is hard to fix the wrong sql.
Actually I think may be there is some problem without any check to cast String to int when user comparing String and AtomicType.
And with spark.sql.ansi.enabled=true, SparkSQL disallowed compare String and DecimalType, user from hive to SparkSQL unwilling to change their code.
So I think It should remind User where your sql is not support in SparkSQL even without ansi mode, Because this case is exception, and user should to be reminded.
--
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] Kwafoor removed a comment on pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
Kwafoor removed a comment on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-991845868
> ansi
spark.sql.ansi.enabled about this case is right, but user always didn't open ansi, because they want use hiveSQL in sparkSQL without SQL change.
I just want to remind user your sql have something wrong.
--
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 #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-992130176
> I agree with ansi, but in current place user didn't get any remind is a problem.
We can turn on ANSI by default someday, which fixes all the problems. I don't agree to randomly pick string integer comparison and make it ANSI. How about string bigint/float/double/decimal comparisons? How about string +-*/ numbers? How about cast string to int/bigint/float/double/decimal/date/timestamp?
--
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 #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-990915296
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] Kwafoor commented on pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
Kwafoor commented on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-991846753
> @Kwafoor Please enable `spark.sql.ansi.enabled` if you prefer thrown exception.
spark.sql.ansi.enabled about this case is right, but user always didn't open ansi, because they want use hiveSQL in sparkSQL without SQL change.
I just want to remind user your sql have something wrong.
--
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 pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-992002616
> spark.sql.ansi.enabled about this case is right, but user always didn't open ansi, because they want use hiveSQL in sparkSQL without SQL change.
> I just want to remind user your sql have something wrong.
the problem here is that it will break existing users app when they migrate to higher versions of spark. e.g.) it worked before but now it fails. So it's better to cover it with the ANSI configuration which users can explicitly enable.
cc @cloud-fan, @gengliangwang and @MaxGekk FYI
--
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] Kwafoor commented on pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
Kwafoor commented on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-992116757
> is it not better to explicitly cast to a larger integer type?
Explicitly cast string to a larger integer type is right, but user may implicit cast and he didn't find any exception( it is possible for a big query, and I have encountered).
If he doesn't check the results, a hug trouble will make.And even if he checked, it's so hard to find a such sql error(User may ignore it, for example hive for this cast type is right.)
So, I just want to remind user when ansi is not enabled and overflow happened, suggest them where your sql is not right.
Return null rather than Exception is too implicit, user always ignore it(If a big query with many filter, it is even more.).
--
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] Kwafoor commented on pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
Kwafoor commented on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-992127549
> > when overflow happened SparkSQL return wrong results(null), but user didn't get any remind, I think it is a problem.
>
> The problem has been there for many years, and many operations/functions are affected (math operations, cast, etc.). I don't think it's justified to change string and integer comparison only (and introduce inconsistency to the system). ANSI mode is the way to go. If ANSI mode is too strict, we can relax it a bit to make it fit the real-world use cases better.
I agree with ansi, but in current place user didn't get any remind is a problem.
I haven't think this could introduce inconsistency to the system yet.
User didn't get any exception but get a null result, it can truly cause confusion, user may ignore the sql error rather than check and find some problem.
--
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] gengliangwang commented on pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-992092077
> ANSI with default true is also not a good idea, it is too strict with cast.
I am actually trying to make the ANSI mode more lenient. Can you provide some scenarios that you think are too strict?
--
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] Kwafoor commented on pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
Kwafoor commented on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-992144811
> I don't agree to randomly pick string integer comparison and make it ANSI.
ANSI is a solution to fix all this problems.But I still think SparkSQL should remind user where you wrong.User from other platform accept ANSI need time to change their sql but they don't want change.So a remind to tell where your sql wrong rather null result is need.
Or we have to suggest user better to use cast explicitly and change their code to accept ANSI.
But return null is truly unobvious，it's hard to find the error.
--
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] Kwafoor commented on a change in pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
Kwafoor commented on a change in pull request #34862:
URL: https://github.com/apache/spark/pull/34862#discussion_r767382412
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
##########
@@ -652,7 +652,13 @@ abstract class CastBase extends UnaryExpression with TimeZoneAwareExpression wit
buildCast[UTF8String](_, UTF8StringUtils.toIntExact)
case StringType =>
val result = new IntWrapper()
- buildCast[UTF8String](_, s => if (s.toInt(result)) result.value else null)
+ buildCast[UTF8String](_, s => {
+ if (s.toInt(result)) {
+ result.value}
Review comment:
In sparkSQL
and
For in Hive
When user comparing String and IntegerType in sparkSQL with spark.sql.ansi.enabled=false, and the default value of spark.sql.ansi.enabled is false, user didn't get any remind but get a wrong result.
User runs a big and complex sql, it is very hard to find where the result is wrong,So they is hard to fix the wrong sql.
Actually I think may be there is some problem without any check to cast String to int when user comparing String and AtomicType.
And with spark.sql.ansi.enabled=true, SparkSQL disallowed compare String and DecimalType, user from hive to SparkSQL unwilling to change their code.
So I think It should remind User where your sql is not support in SparkSQL even without ansi mode, Because this case is exception, and user should to be reminded.
--
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 #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-992160298
> But I still think SparkSQL should remind user where you wrong.
This doesn't answer the question of why you only fix string integer comparison.
> Or we have to suggest user better to use cast explicitly
This works for me. We should discourage relying on implicit cast.
--
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] Kwafoor closed pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
Kwafoor closed pull request #34862:
URL: https://github.com/apache/spark/pull/34862
--
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] Kwafoor edited a comment on pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
Kwafoor edited a comment on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-991846753
> @Kwafoor Please enable `spark.sql.ansi.enabled` if you prefer thrown exception.
spark.sql.ansi.enabled about this case is right, but user always didn't open ansi, because they want use hiveSQL in sparkSQL without SQL change.
I just want to remind user your sql have something wrong.
--
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 #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #34862:
URL: https://github.com/apache/spark/pull/34862#discussion_r767351858
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
##########
@@ -652,7 +652,13 @@ abstract class CastBase extends UnaryExpression with TimeZoneAwareExpression wit
buildCast[UTF8String](_, UTF8StringUtils.toIntExact)
case StringType =>
val result = new IntWrapper()
- buildCast[UTF8String](_, s => if (s.toInt(result)) result.value else null)
+ buildCast[UTF8String](_, s => {
Review comment:
How about other types like short, etc?
--
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] gengliangwang commented on pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-992067435
@Kwafoor the default behavior exists for many years. It is hard to change it now.
That's also why we introducing the ANSI SQL mode as an alternative option. If users want to detect integer overflow, they can enable the `spark.sql.ansi.enabled`.
--
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 edited a comment on pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
cloud-fan edited a comment on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-992101709
> when overflow happened SparkSQL return wrong results(null), but user didn't get any remind, I think it is a problem.
The problem has been there for many years, and many operations/functions are affected (math operations, cast, etc.). I don't think it's justified to change string and integer comparison only (and introduce inconsistency to the system). ANSI mode is the way to go. If ANSI mode is too strict, we can relax it a bit to make it fit the real-world use cases better.
--
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 a change in pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #34862:
URL: https://github.com/apache/spark/pull/34862#discussion_r767281872
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
##########
@@ -652,7 +652,13 @@ abstract class CastBase extends UnaryExpression with TimeZoneAwareExpression wit
buildCast[UTF8String](_, UTF8StringUtils.toIntExact)
case StringType =>
val result = new IntWrapper()
- buildCast[UTF8String](_, s => if (s.toInt(result)) result.value else null)
+ buildCast[UTF8String](_, s => {
+ if (s.toInt(result)) {
+ result.value}
Review comment:
I'm still not clear what you are saying. What does Hive do, for example?
--
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] Kwafoor commented on a change in pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
Kwafoor commented on a change in pull request #34862:
URL: https://github.com/apache/spark/pull/34862#discussion_r767382412
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
##########
@@ -652,7 +652,13 @@ abstract class CastBase extends UnaryExpression with TimeZoneAwareExpression wit
buildCast[UTF8String](_, UTF8StringUtils.toIntExact)
case StringType =>
val result = new IntWrapper()
- buildCast[UTF8String](_, s => if (s.toInt(result)) result.value else null)
+ buildCast[UTF8String](_, s => {
+ if (s.toInt(result)) {
+ result.value}
Review comment:
In sparkSQL
and
For in Hive
When user comparing String and IntegerType in sparkSQL with spark.sql.ansi.enabled=false, and the default value of spark.sql.ansi.enabled is false, user didn't get any remind but get a wrong result.
User runs a big and complex sql, it is very hard to find where the result is wrong,So they is hard to fix the wrong sql.
Actually I think may be there is some problem without any check to cast String to int when user comparing String and AtomicType.
And with spark.sql.ansi.enabled=true, SparkSQL disallowed compare String and DecimalType, user from hive to SparkSQL unwilling to change their code.
So I think It should remind User where your sql is not support in SparkSQL even without ansi mode, Because this case is exception, and user should to be reminded.
--
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] Kwafoor commented on pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
Kwafoor commented on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-992135783
> > ANSI with default true is also not a good idea, it is too strict with cast.
>
> I am actually trying to make the ANSI mode more lenient. Can you provide some scenarios that you think are too strict?
I have test ANSI some simple case, and I will think it later.If I have some scenarios, I would like to share with you.
--
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] Kwafoor commented on pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
Kwafoor commented on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-991845868
> ansi
spark.sql.ansi.enabled about this case is right, but user always didn't open ansi, because they want use hiveSQL in sparkSQL without SQL change.
I just want to remind user your sql have something wrong.
--
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] Kwafoor commented on pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
Kwafoor commented on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-990926523
cc @wangyum @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] Kwafoor commented on a change in pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
Kwafoor commented on a change in pull request #34862:
URL: https://github.com/apache/spark/pull/34862#discussion_r767232024
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
##########
@@ -652,7 +652,13 @@ abstract class CastBase extends UnaryExpression with TimeZoneAwareExpression wit
buildCast[UTF8String](_, UTF8StringUtils.toIntExact)
case StringType =>
val result = new IntWrapper()
- buildCast[UTF8String](_, s => if (s.toInt(result)) result.value else null)
+ buildCast[UTF8String](_, s => {
+ if (s.toInt(result)) {
+ result.value}
Review comment:
This case is some users use HiveSQL in SparkSQL, when cast the value of string exceed Int.MaxValue to integer,SparkSQL return null.It will make big trouble, because use didn't get any exception, but the result is wrong.I think SparkSQL should thrown exception at least.
--
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] Kwafoor commented on a change in pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
Kwafoor commented on a change in pull request #34862:
URL: https://github.com/apache/spark/pull/34862#discussion_r767232024
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
##########
@@ -652,7 +652,13 @@ abstract class CastBase extends UnaryExpression with TimeZoneAwareExpression wit
buildCast[UTF8String](_, UTF8StringUtils.toIntExact)
case StringType =>
val result = new IntWrapper()
- buildCast[UTF8String](_, s => if (s.toInt(result)) result.value else null)
+ buildCast[UTF8String](_, s => {
+ if (s.toInt(result)) {
+ result.value}
Review comment:
This case is some users use HiveSQL in SparkSQL, when cast the value of string exceed Int.MaxValue to integer,SparkSQL return null.It will make big trouble, because user didn't get any exception, but the result is wrong.I think SparkSQL should thrown exception at least.
--
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 pull request #34862: [SPARK-30471][SQL]Fix issue when comparing String and IntegerType
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #34862:
URL: https://github.com/apache/spark/pull/34862#issuecomment-992002935
@Kwafoor, Before we go further, I think it should be best to double check how ANSI defines the behaviour on the overflow of strings <> other types.
--
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