[CALCITE-6283] Function ARRAY_APPEND with a NULL array argument crashes with NullPointerException #3706

mihaibudiu · 2024-02-27T01:29:42Z

No description provided.

tanclary · 2024-02-27T02:38:47Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

+    f.checkNull("array_append(null, 2)");
+    f.checkNull("array_prepend(null, 2)");
+    f.checkNull("array_remove(null, 2)");
+    f.checkNull("array_contains(null, 2)");


Do we have tests for appending null elements

@tanclary Yes, similar to:
f.checkNull("array_append(cast(null as integer array), 1)");

I meant a test where the element is null, not the array. But i found that we do so that's good.

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?

I don't understand your question

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.

Ah. Yes I agree with you. What do you think @mihaibudiu ?

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.

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

core/src/main/java/org/apache/calcite/sql/type/ArrayElementOperandTypeChecker.java

chucheng92 · 2024-02-28T02:20:57Z

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.

chucheng92 · 2024-02-28T02:29:22Z

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 directly. But in fact, spark will throw an exception in the former. We need to address the former case like this PR.

mihaibudiu · 2024-02-28T02:33:28Z

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

chucheng92 · 2024-02-28T02:40:17Z

array_append needs to be capitalized in commit name/jira name/pr name.

chucheng92 · 2024-02-28T02:43:44Z

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 arg is NULL.
My point is that the element is also a NULL literal.

mihaibudiu · 2024-02-28T18:33:22Z

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

mihaibudiu · 2024-02-28T18:39:43Z

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.

mihaibudiu · 2024-02-28T18:40:19Z

array_append needs to be capitalized in commit name/jira name/pr name.

I will update these when I squash the commits

chucheng92 · 2024-02-29T03:59:28Z

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 support inserting null.

yes, I have fixed array_contains/array_except/array_intersect in that PR, and that PR will affect this PR.

mihaibudiu · 2024-02-29T04:02:17Z

Let's merge [CALCITE-5893] first and I will rebase this one.

snuyanzin · 2024-03-05T11:53:32Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

+    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);


It seems there is a similar issue for array_min/array_max should we fix it within this issue or create a separate one?

since I got started I will add these too

mihaibudiu · 2024-03-08T23:13:30Z

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 #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

chucheng92 · 2024-03-11T02:34:21Z

core/src/main/java/org/apache/calcite/sql/type/ArrayElementOperandTypeChecker.java

-    this.allowCast = allowCast;
+  public ArrayElementOperandTypeChecker(boolean arrayMayBeNull, boolean elementMayBeNull) {
+    this.arrayMayBeNull = arrayMayBeNull;
+    this.elementMayBeNull = elementMayBeNull;
  }

  //~ Methods ----------------------------------------------------------------

  @Override public boolean checkOperandTypes(
      SqlCallBinding callBinding,
      boolean throwOnFailure) {


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.

chucheng92 · 2024-03-11T02:35:03Z

overall looks good to me +1

…es with NullPointerException Signed-off-by: Mihai Budiu <[email protected]>

sonarqubecloud · 2024-03-11T17:24:44Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
73.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

mihaibudiu · 2024-03-11T19:56:09Z

Interpreting the +1 as a "go ahead and merge"

tanclary reviewed Feb 27, 2024

View reviewed changes

mihaibudiu force-pushed the issue6283 branch from 76c8ab5 to c17a872 Compare February 27, 2024 19:09

mihaibudiu changed the title ~~[CALCITE-6283] Function array_append with a NULL array argument crashes with NullPointerException~~ [CALCITE-6283] Function ARRAY_APPEND with a NULL array argument crashes with NullPointerException Feb 28, 2024

snuyanzin reviewed Mar 5, 2024

View reviewed changes

mihaibudiu force-pushed the issue6283 branch from c17a872 to 884aad8 Compare March 8, 2024 23:10

chucheng92 reviewed Mar 11, 2024

View reviewed changes

[CALCITE-6283] Function ARRAY_APPEND with a NULL array argument crash…

d47abc6

…es with NullPointerException Signed-off-by: Mihai Budiu <[email protected]>

mihaibudiu force-pushed the issue6283 branch from 884aad8 to d47abc6 Compare March 11, 2024 16:50

mihaibudiu merged commit 90beb01 into apache:main Mar 11, 2024

mihaibudiu deleted the issue6283 branch March 11, 2024 19:56

[CALCITE-6283] Function ARRAY_APPEND with a NULL array argument crashes with NullPointerException #3706

[CALCITE-6283] Function ARRAY_APPEND with a NULL array argument crashes with NullPointerException #3706

Uh oh!

Conversation

mihaibudiu commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caicancai Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chucheng92 commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chucheng92 commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mihaibudiu commented Feb 28, 2024

Uh oh!

chucheng92 commented Feb 28, 2024

Uh oh!

chucheng92 commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mihaibudiu commented Feb 28, 2024

Uh oh!

mihaibudiu commented Feb 28, 2024

Uh oh!

mihaibudiu commented Feb 28, 2024

Uh oh!

chucheng92 commented Feb 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mihaibudiu commented Feb 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mihaibudiu commented Mar 8, 2024

Uh oh!

chucheng92 Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chucheng92 commented Mar 11, 2024

Uh oh!

sonarqubecloud bot commented Mar 11, 2024

Quality Gate passed

Uh oh!

mihaibudiu commented Mar 11, 2024

Uh oh!

Uh oh!

mihaibudiu commented Feb 27, 2024 •

edited

Loading

caicancai Feb 27, 2024 •

edited

Loading

chucheng92 commented Feb 28, 2024 •

edited

Loading

chucheng92 commented Feb 28, 2024 •

edited

Loading

chucheng92 commented Feb 28, 2024 •

edited

Loading

chucheng92 commented Feb 29, 2024 •

edited

Loading

chucheng92 Mar 11, 2024 •

edited

Loading