You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "mihaibudiu (via GitHub)" <gi...@apache.org> on 2024/02/27 01:29:44 UTC
[PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
mihaibudiu opened a new pull request, #3706:
URL: https://github.com/apache/calcite/pull/3706
…
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3706:
URL: https://github.com/apache/calcite/pull/3706#issuecomment-1965646275
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3706) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3706&resolved=false&inNewCodePeriod=true)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3706&resolved=false&inNewCodePeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3706&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3706&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3706)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3706:
URL: https://github.com/apache/calcite/pull/3706#issuecomment-1968079267
The situation may be even more complicated if one considers cases where the compiler needs to insert implicit casts. I don't have a spark engine handy to test what Spark does, but if the array element types and the added element do not have the same type, Spark may choose to insert implicit casts in different ways than Calcite does.
Another fact I don't have a good handle on is the nullability of array elements: https://issues.apache.org/jira/browse/CALCITE-6275
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3706:
URL: https://github.com/apache/calcite/pull/3706#issuecomment-1969607750
> array_append needs to be capitalized in commit name/jira name/pr name.
I will update these when I squash the commits
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function ARRAY_APPEND with a NULL array argument crashes with NullPointerException [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3706:
URL: https://github.com/apache/calcite/pull/3706#issuecomment-1989019180
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3706) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3706&resolved=false&inNewCodePeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3706&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3706&resolved=false&inNewCodePeriod=true)
 [73.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3706&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3706&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3706)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on code in PR #3706:
URL: https://github.com/apache/calcite/pull/3706#discussion_r1503698681
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6268,6 +6268,18 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
f.checkScalar("rand_integer(2, 11)", 1, "INTEGER NOT NULL");
}
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6283">
+ * [CALCITE-6283] Function array_append with a NULL array argument crashes with
+ * NullPointerException</a>. */
+ @Test void testArrayNullFunc() {
+ final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.SPARK);
+ f.checkNull("array_append(null, 2)");
+ f.checkNull("array_prepend(null, 2)");
+ f.checkNull("array_remove(null, 2)");
+ f.checkNull("array_contains(null, 2)");
Review Comment:
What I mean is that spark3.4 version does not support array_append(null, 1), it will report an error. Currently, array_append and array_prepend are adapted for spark in calcite. If spark does not support array_append(null , 1), why do we need to return null? I am not sure whether higher versions of spark support it.
I don't know if my opinion is useful, if it is problematic, please ignore it. But I still want to express my opinion. This is open source, thank 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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function ARRAY_APPEND with a NULL array argument crashes with NullPointerException [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3706:
URL: https://github.com/apache/calcite/pull/3706#issuecomment-1989319546
Interpreting the +1 as a "go ahead and merge"
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3706:
URL: https://github.com/apache/calcite/pull/3706#issuecomment-1967467870
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3706) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3706&resolved=false&inNewCodePeriod=true)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3706&resolved=false&inNewCodePeriod=true)
 [84.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3706&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3706&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3706)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on PR #3706:
URL: https://github.com/apache/calcite/pull/3706#issuecomment-1965693972
@mihaibudiu I have also recently been fixing the bug of array_append, which seems to conflict with this PR.
https://github.com/apache/calcite/pull/3705
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function ARRAY_APPEND with a NULL array argument crashes with NullPointerException [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3706:
URL: https://github.com/apache/calcite/pull/3706#discussion_r1513771870
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6277,6 +6277,19 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
f.checkScalar("rand_integer(2, 11)", 1, "INTEGER NOT NULL");
}
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6283">
+ * [CALCITE-6283] Function array_append with a NULL array argument crashes with
+ * NullPointerException</a>. */
+ @Test void testArrayNullFunc() {
+ final String expected = "Illegal use of 'NULL'";
+ final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.SPARK);
+ f.checkFails("^array_append(null, 2)^", expected, false);
+ f.checkFails("^array_prepend(null, 2)^", expected, false);
+ f.checkFails("^array_remove(null, 2)^", expected, false);
+ f.checkFails("^array_contains(null, 2)^", expected, false);
+ f.checkFails("^array_position(null, 2)^", expected, false);
Review Comment:
since I got started I will add these too
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function ARRAY_APPEND with a NULL array argument crashes with NullPointerException [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu merged PR #3706:
URL: https://github.com/apache/calcite/pull/3706
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "chucheng92 (via GitHub)" <gi...@apache.org>.
chucheng92 commented on PR #3706:
URL: https://github.com/apache/calcite/pull/3706#issuecomment-1968075855
FYI: One background is that many spark functions related to calcite's array are contributed by one contributor. One basic strategy is to turn on NullPolicy, which means that NULL literals, including **NULL** and **cast(null as xxx)** are returned null. But in fact, spark will throw an exception in the former. We need to address the former case.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function ARRAY_APPEND with a NULL array argument crashes with NullPointerException [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3706:
URL: https://github.com/apache/calcite/pull/3706#issuecomment-1970355660
Let's merge [CALCITE-5893] first and I will rebase this one.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function ARRAY_APPEND with a NULL array argument crashes with NullPointerException [calcite]
Posted by "chucheng92 (via GitHub)" <gi...@apache.org>.
chucheng92 commented on code in PR #3706:
URL: https://github.com/apache/calcite/pull/3706#discussion_r1519077366
##########
core/src/main/java/org/apache/calcite/sql/type/ArrayElementOperandTypeChecker.java:
##########
@@ -34,61 +33,64 @@
public class ArrayElementOperandTypeChecker implements SqlOperandTypeChecker {
//~ Instance fields --------------------------------------------------------
- private final boolean allowNullCheck;
- private final boolean allowCast;
+ private final boolean arrayMayBeNull;
+ private final boolean elementMayBeNull;
//~ Constructors -----------------------------------------------------------
- public ArrayElementOperandTypeChecker() {
- this.allowNullCheck = false;
- this.allowCast = false;
- }
-
- public ArrayElementOperandTypeChecker(boolean allowNullCheck, boolean allowCast) {
- this.allowNullCheck = allowNullCheck;
- this.allowCast = allowCast;
+ public ArrayElementOperandTypeChecker(boolean arrayMayBeNull, boolean elementMayBeNull) {
+ this.arrayMayBeNull = arrayMayBeNull;
+ this.elementMayBeNull = elementMayBeNull;
}
//~ Methods ----------------------------------------------------------------
@Override public boolean checkOperandTypes(
SqlCallBinding callBinding,
boolean throwOnFailure) {
Review Comment:
If array is not first argument? such as array_xxx(1, array, 2). It seems that this typechecker only works in this situation.
But I think if this happens, we can add another typechecker or refactor, such as passing in index to indicate the index of the array parameter.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on code in PR #3706:
URL: https://github.com/apache/calcite/pull/3706#discussion_r1503561320
##########
core/src/main/java/org/apache/calcite/sql/type/ArrayElementOperandTypeChecker.java:
##########
@@ -45,23 +45,27 @@ public class ArrayElementOperandTypeChecker implements SqlOperandTypeChecker {
return false;
}
- RelDataType arrayComponentType =
- getComponentTypeOrThrow(SqlTypeUtil.deriveType(callBinding, op0));
+ RelDataType op0Type = SqlTypeUtil.deriveType(callBinding, op0);
Review Comment:
can this be final
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6268,6 +6268,18 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
f.checkScalar("rand_integer(2, 11)", 1, "INTEGER NOT NULL");
}
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6283">
+ * [CALCITE-6283] Function array_append with a NULL array argument crashes with
+ * NullPointerException</a>. */
+ @Test void testArrayNullFunc() {
+ final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.SPARK);
+ f.checkNull("array_append(null, 2)");
+ f.checkNull("array_prepend(null, 2)");
+ f.checkNull("array_remove(null, 2)");
+ f.checkNull("array_contains(null, 2)");
Review Comment:
Do we have tests for appending null elements
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on code in PR #3706:
URL: https://github.com/apache/calcite/pull/3706#discussion_r1503577545
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6268,6 +6268,18 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
f.checkScalar("rand_integer(2, 11)", 1, "INTEGER NOT NULL");
}
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6283">
+ * [CALCITE-6283] Function array_append with a NULL array argument crashes with
+ * NullPointerException</a>. */
+ @Test void testArrayNullFunc() {
+ final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.SPARK);
+ f.checkNull("array_append(null, 2)");
+ f.checkNull("array_prepend(null, 2)");
+ f.checkNull("array_remove(null, 2)");
+ f.checkNull("array_contains(null, 2)");
Review Comment:
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on code in PR #3706:
URL: https://github.com/apache/calcite/pull/3706#discussion_r1503587080
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6268,6 +6268,18 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
f.checkScalar("rand_integer(2, 11)", 1, "INTEGER NOT NULL");
}
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6283">
+ * [CALCITE-6283] Function array_append with a NULL array argument crashes with
+ * NullPointerException</a>. */
+ @Test void testArrayNullFunc() {
+ final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.SPARK);
+ f.checkNull("array_append(null, 2)");
+ f.checkNull("array_prepend(null, 2)");
+ f.checkNull("array_remove(null, 2)");
+ f.checkNull("array_contains(null, 2)");
Review Comment:
My problem is that array_append is to adapt to spark, but array_append(null, 2) of spark will report an error. Since it is to adapt to spark, why do I need to return null? Maybe higher spark versions have fixed it, do we need it? State this in the documentation
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function ARRAY_APPEND with a NULL array argument crashes with NullPointerException [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3706:
URL: https://github.com/apache/calcite/pull/3706#issuecomment-1986551839
In my last commit I have fixed array_min and array_max as well.
I have slightly modified the helper function introduced by @chucheng92 in https://github.com/apache/calcite/pull/3355 and used it for typechecking more functions.
I have also corrected a test for array_contains, according to the Databricks manual, that test was incorrect.
https://docs.databricks.com/en/sql/language-manual/functions/array_contains.html
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on code in PR #3706:
URL: https://github.com/apache/calcite/pull/3706#discussion_r1503587080
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6268,6 +6268,18 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
f.checkScalar("rand_integer(2, 11)", 1, "INTEGER NOT NULL");
}
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6283">
+ * [CALCITE-6283] Function array_append with a NULL array argument crashes with
+ * NullPointerException</a>. */
+ @Test void testArrayNullFunc() {
+ final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.SPARK);
+ f.checkNull("array_append(null, 2)");
+ f.checkNull("array_prepend(null, 2)");
+ f.checkNull("array_remove(null, 2)");
+ f.checkNull("array_contains(null, 2)");
Review Comment:
My problem is that array_append is to adapt to spark, but spark’s array_append(null, 2) will report an 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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "chucheng92 (via GitHub)" <gi...@apache.org>.
chucheng92 commented on PR #3706:
URL: https://github.com/apache/calcite/pull/3706#issuecomment-1968088353
> The situation may be even more complicated if one considers cases where the compiler needs to insert implicit casts. I don't have a spark engine handy to test what Spark does, but if the array element types and the added element do not have the same type, Spark may choose to insert implicit casts in different ways than Calcite does.
>
> Another fact I don't have a good handle on is the nullability of array elements: https://issues.apache.org/jira/browse/CALCITE-6275
make sense. I just read it wrong. Your PR only handles the situation when array is null.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on code in PR #3706:
URL: https://github.com/apache/calcite/pull/3706#discussion_r1504508362
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6268,6 +6268,18 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
f.checkScalar("rand_integer(2, 11)", 1, "INTEGER NOT NULL");
}
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6283">
+ * [CALCITE-6283] Function array_append with a NULL array argument crashes with
+ * NullPointerException</a>. */
+ @Test void testArrayNullFunc() {
+ final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.SPARK);
+ f.checkNull("array_append(null, 2)");
+ f.checkNull("array_prepend(null, 2)");
+ f.checkNull("array_remove(null, 2)");
+ f.checkNull("array_contains(null, 2)");
Review Comment:
Ah. Yes I agree with you. What do you think @mihaibudiu ?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function ARRAY_APPEND with a NULL array argument crashes with NullPointerException [calcite]
Posted by "chucheng92 (via GitHub)" <gi...@apache.org>.
chucheng92 commented on code in PR #3706:
URL: https://github.com/apache/calcite/pull/3706#discussion_r1519077366
##########
core/src/main/java/org/apache/calcite/sql/type/ArrayElementOperandTypeChecker.java:
##########
@@ -34,61 +33,64 @@
public class ArrayElementOperandTypeChecker implements SqlOperandTypeChecker {
//~ Instance fields --------------------------------------------------------
- private final boolean allowNullCheck;
- private final boolean allowCast;
+ private final boolean arrayMayBeNull;
+ private final boolean elementMayBeNull;
//~ Constructors -----------------------------------------------------------
- public ArrayElementOperandTypeChecker() {
- this.allowNullCheck = false;
- this.allowCast = false;
- }
-
- public ArrayElementOperandTypeChecker(boolean allowNullCheck, boolean allowCast) {
- this.allowNullCheck = allowNullCheck;
- this.allowCast = allowCast;
+ public ArrayElementOperandTypeChecker(boolean arrayMayBeNull, boolean elementMayBeNull) {
+ this.arrayMayBeNull = arrayMayBeNull;
+ this.elementMayBeNull = elementMayBeNull;
}
//~ Methods ----------------------------------------------------------------
@Override public boolean checkOperandTypes(
SqlCallBinding callBinding,
boolean throwOnFailure) {
Review Comment:
If array is not first argument? such as array_xxx(1, array, 2). It seems that this typechecker only works in this situation.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function ARRAY_APPEND with a NULL array argument crashes with NullPointerException [calcite]
Posted by "chucheng92 (via GitHub)" <gi...@apache.org>.
chucheng92 commented on PR #3706:
URL: https://github.com/apache/calcite/pull/3706#issuecomment-1987522169
overall looks good to me +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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on code in PR #3706:
URL: https://github.com/apache/calcite/pull/3706#discussion_r1503577184
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6268,6 +6268,18 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
f.checkScalar("rand_integer(2, 11)", 1, "INTEGER NOT NULL");
}
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6283">
+ * [CALCITE-6283] Function array_append with a NULL array argument crashes with
+ * NullPointerException</a>. */
+ @Test void testArrayNullFunc() {
+ final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.SPARK);
+ f.checkNull("array_append(null, 2)");
+ f.checkNull("array_prepend(null, 2)");
+ f.checkNull("array_remove(null, 2)");
+ f.checkNull("array_contains(null, 2)");
Review Comment:
I tested it in spark3.4. It seems that the first parameter of array_append is null. Why do we need to change it like 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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3706:
URL: https://github.com/apache/calcite/pull/3706#discussion_r1504806311
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6268,6 +6268,18 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
f.checkScalar("rand_integer(2, 11)", 1, "INTEGER NOT NULL");
}
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6283">
+ * [CALCITE-6283] Function array_append with a NULL array argument crashes with
+ * NullPointerException</a>. */
+ @Test void testArrayNullFunc() {
+ final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.SPARK);
+ f.checkNull("array_append(null, 2)");
+ f.checkNull("array_prepend(null, 2)");
+ f.checkNull("array_remove(null, 2)");
+ f.checkNull("array_contains(null, 2)");
Review Comment:
I have changed the type checker to reject NULL literals for arrays.
However, it seems to be that Spark is inconsistent, since it allows NULL values for arrays at runtime, but not at compile time. This is probably because Spark uses a different type checking/inference algorithm.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "chucheng92 (via GitHub)" <gi...@apache.org>.
chucheng92 commented on PR #3706:
URL: https://github.com/apache/calcite/pull/3706#issuecomment-1968085594
array_append needs to be capitalized in commit name/jira name/pr name.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function ARRAY_APPEND with a NULL array argument crashes with NullPointerException [calcite]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #3706:
URL: https://github.com/apache/calcite/pull/3706#discussion_r1512696170
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6277,6 +6277,19 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
f.checkScalar("rand_integer(2, 11)", 1, "INTEGER NOT NULL");
}
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6283">
+ * [CALCITE-6283] Function array_append with a NULL array argument crashes with
+ * NullPointerException</a>. */
+ @Test void testArrayNullFunc() {
+ final String expected = "Illegal use of 'NULL'";
+ final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.SPARK);
+ f.checkFails("^array_append(null, 2)^", expected, false);
+ f.checkFails("^array_prepend(null, 2)^", expected, false);
+ f.checkFails("^array_remove(null, 2)^", expected, false);
+ f.checkFails("^array_contains(null, 2)^", expected, false);
+ f.checkFails("^array_position(null, 2)^", expected, false);
Review Comment:
It seems there is a similar issue for `array_min/array_max` should we fix it within this issue or create a separate one?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on code in PR #3706:
URL: https://github.com/apache/calcite/pull/3706#discussion_r1505234803
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6268,6 +6268,18 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
f.checkScalar("rand_integer(2, 11)", 1, "INTEGER NOT NULL");
}
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6283">
+ * [CALCITE-6283] Function array_append with a NULL array argument crashes with
+ * NullPointerException</a>. */
+ @Test void testArrayNullFunc() {
+ final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.SPARK);
+ f.checkNull("array_append(null, 2)");
+ f.checkNull("array_prepend(null, 2)");
+ f.checkNull("array_remove(null, 2)");
+ f.checkNull("array_contains(null, 2)");
Review Comment:
> I have changed the type checker to reject NULL literals for arrays. However, it seems to be that Spark is inconsistent, since it allows NULL values for arrays at runtime, but not at compile time. This is probably because Spark uses a different type checking/inference algorithm.
spark sql, after generating the Unresolved Logical Plan, runs a series of rules to turn it into the Logical Plan, which solves some type conversion issues there, and the type handling of array_append(null,2) seems to be in that part
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3706:
URL: https://github.com/apache/calcite/pull/3706#issuecomment-1969596965
> make sense. I just read it wrong. Your PR only handles the situation when array arg is NULL. My point is that the element is also a NULL literal.
There are already tests for this case, e.g.: `testArrayAppendFunc`
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on code in PR #3706:
URL: https://github.com/apache/calcite/pull/3706#discussion_r1503579633
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6268,6 +6268,18 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
f.checkScalar("rand_integer(2, 11)", 1, "INTEGER NOT NULL");
}
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6283">
+ * [CALCITE-6283] Function array_append with a NULL array argument crashes with
+ * NullPointerException</a>. */
+ @Test void testArrayNullFunc() {
+ final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.SPARK);
+ f.checkNull("array_append(null, 2)");
+ f.checkNull("array_prepend(null, 2)");
+ f.checkNull("array_remove(null, 2)");
+ f.checkNull("array_contains(null, 2)");
Review Comment:
I don't understand your question
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on code in PR #3706:
URL: https://github.com/apache/calcite/pull/3706#discussion_r1503587080
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6268,6 +6268,18 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
f.checkScalar("rand_integer(2, 11)", 1, "INTEGER NOT NULL");
}
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6283">
+ * [CALCITE-6283] Function array_append with a NULL array argument crashes with
+ * NullPointerException</a>. */
+ @Test void testArrayNullFunc() {
+ final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.SPARK);
+ f.checkNull("array_append(null, 2)");
+ f.checkNull("array_prepend(null, 2)");
+ f.checkNull("array_remove(null, 2)");
+ f.checkNull("array_contains(null, 2)");
Review Comment:
My problem is that array_append is to adapt to spark, but array_append(null, 2) of spark will report an error. Since it is to adapt to spark, why do I need to return null? Maybe higher spark versions have fixed it, do we need it? State this in the documentation
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6268,6 +6268,18 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
f.checkScalar("rand_integer(2, 11)", 1, "INTEGER NOT NULL");
}
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6283">
+ * [CALCITE-6283] Function array_append with a NULL array argument crashes with
+ * NullPointerException</a>. */
+ @Test void testArrayNullFunc() {
+ final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.SPARK);
+ f.checkNull("array_append(null, 2)");
+ f.checkNull("array_prepend(null, 2)");
+ f.checkNull("array_remove(null, 2)");
+ f.checkNull("array_contains(null, 2)");
Review Comment:
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on code in PR #3706:
URL: https://github.com/apache/calcite/pull/3706#discussion_r1503698681
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6268,6 +6268,18 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
f.checkScalar("rand_integer(2, 11)", 1, "INTEGER NOT NULL");
}
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6283">
+ * [CALCITE-6283] Function array_append with a NULL array argument crashes with
+ * NullPointerException</a>. */
+ @Test void testArrayNullFunc() {
+ final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.SPARK);
+ f.checkNull("array_append(null, 2)");
+ f.checkNull("array_prepend(null, 2)");
+ f.checkNull("array_remove(null, 2)");
+ f.checkNull("array_contains(null, 2)");
Review Comment:
What I mean is that spark3.4 version does not support array_append(null, 1), it will report an error. Currently, array_append and array_prepend are adapted for spark in calcite. If spark does not support array_append(null , 1), why do we need to return null? I am not sure whether higher versions of spark support it.
I don't know if my opinion is useful, if it is problematic, please ignore it. thank 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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on code in PR #3706:
URL: https://github.com/apache/calcite/pull/3706#discussion_r1503567964
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6268,6 +6268,18 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
f.checkScalar("rand_integer(2, 11)", 1, "INTEGER NOT NULL");
}
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6283">
+ * [CALCITE-6283] Function array_append with a NULL array argument crashes with
+ * NullPointerException</a>. */
+ @Test void testArrayNullFunc() {
+ final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.SPARK);
+ f.checkNull("array_append(null, 2)");
+ f.checkNull("array_prepend(null, 2)");
+ f.checkNull("array_remove(null, 2)");
+ f.checkNull("array_contains(null, 2)");
Review Comment:
I meant a test where the element is null, not the array. But i found that we do so that's good.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on code in PR #3706:
URL: https://github.com/apache/calcite/pull/3706#discussion_r1503566534
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6268,6 +6268,18 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
f.checkScalar("rand_integer(2, 11)", 1, "INTEGER NOT NULL");
}
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6283">
+ * [CALCITE-6283] Function array_append with a NULL array argument crashes with
+ * NullPointerException</a>. */
+ @Test void testArrayNullFunc() {
+ final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.SPARK);
+ f.checkNull("array_append(null, 2)");
+ f.checkNull("array_prepend(null, 2)");
+ f.checkNull("array_remove(null, 2)");
+ f.checkNull("array_contains(null, 2)");
Review Comment:
@tanclary Yes, similar to:
f.checkNull("array_append(cast(null as integer array), 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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3706:
URL: https://github.com/apache/calcite/pull/3706#issuecomment-1969606855
> pls also see https://issues.apache.org/jira/browse/CALCITE-5893 to support array_except/array_intersect. they also should reject NULL literals.
>
> calcite spark:
>
> // return null select array_contains(array[1, 2], null); // return null select array_except(array[1, 2, 3], null) // return null select array_intersect(array[1,2,3], null)
>
> they should reject NULL.
That's great, we should merge that PR if it solves the problem. I will try to review 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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException [calcite]
Posted by "chucheng92 (via GitHub)" <gi...@apache.org>.
chucheng92 commented on PR #3706:
URL: https://github.com/apache/calcite/pull/3706#issuecomment-1968068958
pls also see https://issues.apache.org/jira/browse/CALCITE-5893 to support array_except/array_intersect. they also should reject NULL literals.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6283] Function ARRAY_APPEND with a NULL array argument crashes with NullPointerException [calcite]
Posted by "chucheng92 (via GitHub)" <gi...@apache.org>.
chucheng92 commented on PR #3706:
URL: https://github.com/apache/calcite/pull/3706#issuecomment-1970353706
> > make sense. I just read it wrong. Your PR only handles the situation when array arg is NULL. My point is that the element is also a NULL literal.
>
> There are already tests for this case, e.g.: `testArrayAppendFunc`
If the first array arg is NULL, then it is an exception for all these array functions. There is no doubt about this, but if the second arg is NULL, the semantics are different. `array_contains/array_except/array_intersect` is currently wrong. And you are right to mention `array_append/array_prepend` as it does insert null.
yes, I have fixed `array_contains/array_except/array_intersect` in that PR, and that PR will affect this PR.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org