You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "chenhao-db (via GitHub)" <gi...@apache.org> on 2024/03/26 16:53:11 UTC
[PR] [SPARK-47569][SQL] Disallow comparing variant. [spark]
chenhao-db opened a new pull request, #45726:
URL: https://github.com/apache/spark/pull/45726
### What changes were proposed in this pull request?
It adds type-checking rules to disallow comparing variant values (including group by a variant column). We may support comparing variant values in the future, but since we don't have a proper comparison implementation at this point, they should be disallowed on the user surface.
### How was this patch tested?
Unit tests.
--
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-47569][SQL] Disallow comparing variant. [spark]
Posted by "gene-db (via GitHub)" <gi...@apache.org>.
gene-db commented on code in PR #45726:
URL: https://github.com/apache/spark/pull/45726#discussion_r1539782844
##########
sql/core/src/test/scala/org/apache/spark/sql/VariantSuite.scala:
##########
@@ -192,4 +192,22 @@ class VariantSuite extends QueryTest with SharedSparkSession {
}
}
}
+
+ test("group/order/join variant are disabled") {
Review Comment:
Thanks for the tests!
What about sort by? or window partition by, or window order by? Some examples:
```
select parse_json('') sort by 1
with t as (select 1 as a, parse_json('') as v)
select rank() over (partition by v order by a)
with t as (select 1 as a, parse_json('') as v)
select rank() over (partition by a order by v)
```
--
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-47569][SQL] Disallow comparing variant. [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #45726:
URL: https://github.com/apache/spark/pull/45726#issuecomment-2029323800
thanks, merging to master!
--
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-47569][SQL] Disallow comparing variant. [spark]
Posted by "chenhao-db (via GitHub)" <gi...@apache.org>.
chenhao-db commented on code in PR #45726:
URL: https://github.com/apache/spark/pull/45726#discussion_r1539892458
##########
sql/core/src/test/scala/org/apache/spark/sql/VariantSuite.scala:
##########
@@ -192,4 +192,22 @@ class VariantSuite extends QueryTest with SharedSparkSession {
}
}
}
+
+ test("group/order/join variant are disabled") {
Review Comment:
This is a very good point. Actually, the second statement (`partition by v`) will not fail as expected, and it is really an error in the type check for `Window`. Both `partitionSpec` and `orderSpec` must be orderable, but the current type check only includes `orderSpec`. If the `partitionSpec` contains a map type, the query will fail later in an optimizer rule with `INTERNAL_ERROR`. I will create another PR to fix the type check for `Window`.
--
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-47569][SQL] Disallow comparing variant. [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #45726: [SPARK-47569][SQL] Disallow comparing variant.
URL: https://github.com/apache/spark/pull/45726
--
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-47569][SQL] Disallow comparing variant. [spark]
Posted by "chenhao-db (via GitHub)" <gi...@apache.org>.
chenhao-db commented on code in PR #45726:
URL: https://github.com/apache/spark/pull/45726#discussion_r1539892458
##########
sql/core/src/test/scala/org/apache/spark/sql/VariantSuite.scala:
##########
@@ -192,4 +192,22 @@ class VariantSuite extends QueryTest with SharedSparkSession {
}
}
}
+
+ test("group/order/join variant are disabled") {
Review Comment:
This is a very good point. Actually, the third statement will not fail as expected, and it is really an error in the type check for `Window`. Both `partitionSpec` and `orderSpec` must be orderable, but the current type check only includes `orderSpec`. If the `partitionSpec` contains a map type, the query will fail later in an optimizer rule with `INTERNAL_ERROR`. I will create another PR to fix the type check for `Window`.
--
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-47569][SQL] Disallow comparing variant. [spark]
Posted by "chenhao-db (via GitHub)" <gi...@apache.org>.
chenhao-db commented on code in PR #45726:
URL: https://github.com/apache/spark/pull/45726#discussion_r1539959164
##########
sql/core/src/test/scala/org/apache/spark/sql/VariantSuite.scala:
##########
@@ -192,4 +192,22 @@ class VariantSuite extends QueryTest with SharedSparkSession {
}
}
}
+
+ test("group/order/join variant are disabled") {
Review Comment:
If we can merge https://github.com/apache/spark/pull/45730 before this PR, we can add the second statement to the tests.
--
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-47569][SQL] Disallow comparing variant. [spark]
Posted by "chenhao-db (via GitHub)" <gi...@apache.org>.
chenhao-db commented on PR #45726:
URL: https://github.com/apache/spark/pull/45726#issuecomment-2024158327
@cloud-fan Could you help review this PR? 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