You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "mihailoale-db (via GitHub)" <gi...@apache.org> on 2025/04/15 10:32:26 UTC
[PR] [SPARK-51805] [SQL] Get with null argument should return null instead of throwing cast error [spark]
mihailoale-db opened a new pull request, #50590:
URL: https://github.com/apache/spark/pull/50590
### What changes were proposed in this pull request?
Following query throw `Cannot cast NullType to Arraytype`:
```
SELECT get(null, 0);
```
instead of returning `null`. I propose that we fix that.
### Why are the changes needed?
To correct behavior of `get` function.
### Does this PR introduce _any_ user-facing change?
Query that were failing is now passing.
### How was this patch tested?
Added test.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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
Re: [PR] [SPARK-51805] [SQL] Get function with improper argument should throw proper exception instead of an internal one [spark]
Posted by "mihailotim-db (via GitHub)" <gi...@apache.org>.
mihailotim-db commented on code in PR #50590:
URL: https://github.com/apache/spark/pull/50590#discussion_r2055940352
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala:
##########
@@ -314,6 +315,30 @@ case class GetArrayItem(
})
}
+ override def checkInputDataTypes(): TypeCheckResult = {
+ (left.dataType, right.dataType) match {
+ case (_: ArrayType, e2) if !e2.isInstanceOf[IntegralType] =>
+ DataTypeMismatch(
+ errorSubClass = "UNEXPECTED_INPUT_TYPE",
+ messageParameters = Map(
+ "paramIndex" -> ordinalNumber(1),
+ "requiredType" -> toSQLType(IntegerType),
+ "inputSql" -> toSQLExpr(right),
+ "inputType" -> toSQLType(right.dataType))
+ )
+ case (e1, _) if (!e1.isInstanceOf[ArrayType]) =>
Review Comment:
```suggestion
case (e1, _) if !e1.isInstanceOf[ArrayType] =>
```
--
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
Re: [PR] [SPARK-51805] [SQL] Get function with improper argument should throw proper exception instead of an internal one [spark]
Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.
mihailoale-db commented on PR #50590:
URL: https://github.com/apache/spark/pull/50590#issuecomment-2827159155
@MaxGekk PTAL when you have time. 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
Re: [PR] [SPARK-51805] [SQL] Get with null argument should return null instead of throwing cast error [spark]
Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.
mihailoale-db commented on PR #50590:
URL: https://github.com/apache/spark/pull/50590#issuecomment-2810055127
After offline discussion with @srielau we agreed that throwing is the way to go to make it on par with `element_at` function.
--
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
Re: [PR] [SPARK-51805] [SQL] Get function with improper argument should throw proper exception instead of an internal one [spark]
Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.
mihailoale-db commented on code in PR #50590:
URL: https://github.com/apache/spark/pull/50590#discussion_r2055968831
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala:
##########
@@ -314,6 +315,30 @@ case class GetArrayItem(
})
}
+ override def checkInputDataTypes(): TypeCheckResult = {
+ (left.dataType, right.dataType) match {
+ case (_: ArrayType, e2) if !e2.isInstanceOf[IntegralType] =>
+ DataTypeMismatch(
+ errorSubClass = "UNEXPECTED_INPUT_TYPE",
+ messageParameters = Map(
+ "paramIndex" -> ordinalNumber(1),
+ "requiredType" -> toSQLType(IntegerType),
+ "inputSql" -> toSQLExpr(right),
+ "inputType" -> toSQLType(right.dataType))
+ )
+ case (e1, _) if (!e1.isInstanceOf[ArrayType]) =>
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
Re: [PR] [SPARK-51805] [SQL] Get function with improper argument should throw proper exception instead of an internal one [spark]
Posted by "mihailotim-db (via GitHub)" <gi...@apache.org>.
mihailotim-db commented on code in PR #50590:
URL: https://github.com/apache/spark/pull/50590#discussion_r2055953354
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala:
##########
@@ -314,6 +315,30 @@ case class GetArrayItem(
})
}
+ override def checkInputDataTypes(): TypeCheckResult = {
+ (left.dataType, right.dataType) match {
+ case (_: ArrayType, e2) if !e2.isInstanceOf[IntegralType] =>
+ DataTypeMismatch(
+ errorSubClass = "UNEXPECTED_INPUT_TYPE",
+ messageParameters = Map(
+ "paramIndex" -> ordinalNumber(1),
+ "requiredType" -> toSQLType(IntegerType),
+ "inputSql" -> toSQLExpr(right),
+ "inputType" -> toSQLType(right.dataType))
+ )
+ case (e1, _) if (!e1.isInstanceOf[ArrayType]) =>
Review Comment:
Also can we have `MapTypeHere`?
--
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
Re: [PR] [SPARK-51805] [SQL] Get function with improper argument should throw proper exception instead of an internal one [spark]
Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.
mihailoale-db commented on code in PR #50590:
URL: https://github.com/apache/spark/pull/50590#discussion_r2060463599
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala:
##########
@@ -314,6 +315,30 @@ case class GetArrayItem(
})
}
+ override def checkInputDataTypes(): TypeCheckResult = {
+ (left.dataType, right.dataType) match {
+ case (_: ArrayType, e2) if !e2.isInstanceOf[IntegralType] =>
Review Comment:
I would keep it for safety (so we don't throw internal error if we somehow fail to apply type coercion rule). @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
Re: [PR] [SPARK-51805] [SQL] Get with null argument should return null instead of throwing cast error [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #50590:
URL: https://github.com/apache/spark/pull/50590#discussion_r2044636714
##########
sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala:
##########
@@ -4934,6 +4934,11 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
checkAnswer(df, Row("a"))
}
+
+ test("SPARK-51805: Get with null argument should return null instead of throwing cast error") {
Review Comment:
Would you mind to move this test to more specific place, for example `array.sql`
##########
sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala:
##########
@@ -4934,6 +4934,11 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
checkAnswer(df, Row("a"))
}
+
+ test("SPARK-51805: Get with null argument should return null instead of throwing cast error") {
+ val df = sql("SELECT get(null, 0)")
Review Comment:
Could you check the second argument as `null`, 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
Re: [PR] [SPARK-51805] [SQL] Get function with improper argument should throw proper exception instead of an internal one [spark]
Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.
mihailoale-db commented on code in PR #50590:
URL: https://github.com/apache/spark/pull/50590#discussion_r2060432450
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala:
##########
@@ -314,6 +315,30 @@ case class GetArrayItem(
})
}
+ override def checkInputDataTypes(): TypeCheckResult = {
+ (left.dataType, right.dataType) match {
+ case (_: ArrayType, e2) if !e2.isInstanceOf[IntegralType] =>
Review Comment:
Yes, they have existed before (please check `array.sql`). But on the other hand `select get(array(1),null)` passes because `GetArrayItem` extends `ExpectsInputTypes` and because of that `NullType` is casted to `IntegerType`. Should we remove this check as it is noop?
--
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
Re: [PR] [SPARK-51805] [SQL] Get function with improper argument should throw proper exception instead of an internal one [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #50590:
URL: https://github.com/apache/spark/pull/50590#discussion_r2055996285
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameAggregateSuite.scala:
##########
@@ -1397,7 +1397,7 @@ class DataFrameAggregateSuite extends QueryTest
"paramIndex" -> "second",
"inputSql" -> "\"a\"",
"inputType" -> "\"STRING\"",
- "requiredType" -> "\"INTEGRAL\""),
+ "requiredType" -> "\"INT\""),
Review Comment:
Why the requirement become more strict?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala:
##########
@@ -314,6 +315,30 @@ case class GetArrayItem(
})
}
+ override def checkInputDataTypes(): TypeCheckResult = {
+ (left.dataType, right.dataType) match {
+ case (_: ArrayType, e2) if !e2.isInstanceOf[IntegralType] =>
+ DataTypeMismatch(
+ errorSubClass = "UNEXPECTED_INPUT_TYPE",
+ messageParameters = Map(
+ "paramIndex" -> ordinalNumber(1),
+ "requiredType" -> toSQLType(IntegerType),
Review Comment:
Why it is `IntegerType` if you check for `IntegralType` above?
--
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
Re: [PR] [SPARK-51805] [SQL] Get function with improper argument should throw proper exception instead of an internal one [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #50590: [SPARK-51805] [SQL] Get function with improper argument should throw proper exception instead of an internal one
URL: https://github.com/apache/spark/pull/50590
--
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
Re: [PR] [SPARK-51805] [SQL] Get function with improper argument should throw proper exception instead of an internal one [spark]
Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.
mihailoale-db commented on PR #50590:
URL: https://github.com/apache/spark/pull/50590#issuecomment-2823762161
@MaxGekk PTAL when you have time. 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
Re: [PR] [SPARK-51805] [SQL] Get with null argument should return null instead of throwing cast error [spark]
Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.
mihailoale-db commented on code in PR #50590:
URL: https://github.com/apache/spark/pull/50590#discussion_r2045061554
##########
sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala:
##########
@@ -4934,6 +4934,11 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
checkAnswer(df, Row("a"))
}
+
+ test("SPARK-51805: Get with null argument should return null instead of throwing cast error") {
+ val df = sql("SELECT get(null, 0)")
Review Comment:
Already covered (working on all the versions).
##########
sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala:
##########
@@ -4934,6 +4934,11 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
checkAnswer(df, Row("a"))
}
+
+ test("SPARK-51805: Get with null argument should return null instead of throwing cast error") {
Review Comment:
Makes sense.
--
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
Re: [PR] [SPARK-51805] [SQL] Get function with improper argument should throw proper exception instead of an internal one [spark]
Posted by "mihailotim-db (via GitHub)" <gi...@apache.org>.
mihailotim-db commented on code in PR #50590:
URL: https://github.com/apache/spark/pull/50590#discussion_r2055967354
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala:
##########
@@ -314,6 +315,30 @@ case class GetArrayItem(
})
}
+ override def checkInputDataTypes(): TypeCheckResult = {
+ (left.dataType, right.dataType) match {
+ case (_: ArrayType, e2) if !e2.isInstanceOf[IntegralType] =>
+ DataTypeMismatch(
+ errorSubClass = "UNEXPECTED_INPUT_TYPE",
+ messageParameters = Map(
+ "paramIndex" -> ordinalNumber(1),
+ "requiredType" -> toSQLType(IntegerType),
+ "inputSql" -> toSQLExpr(right),
+ "inputType" -> toSQLType(right.dataType))
+ )
+ case (e1, _) if (!e1.isInstanceOf[ArrayType]) =>
Review Comment:
In that case we shouldn't suggest it in error message here: https://github.com/apache/spark/pull/50590/files#diff-85360f5a564bdbada5a1687b7bc1dbfe1df6a1470897db13fb31f3bda9ccfc27R334
--
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
Re: [PR] [SPARK-51805] [SQL] Get function with improper argument should throw proper exception instead of an internal one [spark]
Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.
mihailoale-db commented on code in PR #50590:
URL: https://github.com/apache/spark/pull/50590#discussion_r2055964501
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala:
##########
@@ -314,6 +315,30 @@ case class GetArrayItem(
})
}
+ override def checkInputDataTypes(): TypeCheckResult = {
+ (left.dataType, right.dataType) match {
+ case (_: ArrayType, e2) if !e2.isInstanceOf[IntegralType] =>
+ DataTypeMismatch(
+ errorSubClass = "UNEXPECTED_INPUT_TYPE",
+ messageParameters = Map(
+ "paramIndex" -> ordinalNumber(1),
+ "requiredType" -> toSQLType(IntegerType),
+ "inputSql" -> toSQLExpr(right),
+ "inputType" -> toSQLType(right.dataType))
+ )
+ case (e1, _) if (!e1.isInstanceOf[ArrayType]) =>
Review Comment:
It is `ARRAY` expression only (per 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
Re: [PR] [SPARK-51805] [SQL] Get function with improper argument should throw proper exception instead of an internal one [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50590:
URL: https://github.com/apache/spark/pull/50590#discussion_r2060321814
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala:
##########
@@ -314,6 +315,30 @@ case class GetArrayItem(
})
}
+ override def checkInputDataTypes(): TypeCheckResult = {
+ (left.dataType, right.dataType) match {
+ case (_: ArrayType, e2) if !e2.isInstanceOf[IntegralType] =>
Review Comment:
do we have a test to examine this branch?
--
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
Re: [PR] [SPARK-51805] [SQL] Get function with improper argument should throw proper exception instead of an internal one [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #50590:
URL: https://github.com/apache/spark/pull/50590#issuecomment-2830878038
thanks, merging to master/4.0!
--
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