Skip to content

[CALCITE-6454] Implement array comparison operators#3852

Merged
mihaibudiu merged 1 commit intoapache:mainfrom
Bit-Quill:calcite-6454
Jul 15, 2024
Merged

[CALCITE-6454] Implement array comparison operators#3852
mihaibudiu merged 1 commit intoapache:mainfrom
Bit-Quill:calcite-6454

Conversation

@normanj-bitquill
Copy link
Contributor

  • <, <=, >, >= now work for arrays and rows
  • Can also sort arrays and rows
  • Comparison is performed along corresponding indexes
  • Longer arrays are considered greater
  • null is considered greater than anything
  • Cannot change whether nulls are first or last

Copy link
Contributor

@mihaibudiu mihaibudiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the natural question is whether this works for other recursive types, such as int array array...

@normanj-bitquill
Copy link
Contributor Author

the natural question is whether this works for other recursive types, such as int array array...

@mihaibudiu I'm not sure. This PR currently handles arrays and rows. The arrays and rows can contain scalar values as well as arrays and rows.

Do you have any examples of simple queries that could generate other types? I can generate an array in a query with SELECT array[1, 2]; and a row with SELECT row(1, 2);

@NobiGo
Copy link
Contributor

NobiGo commented Jul 12, 2024

@normanj-bitquill

  1. I find different data source has different behaviour about NULL value:
spark-sql (default)> select array(1,2)>array(1,null);
true
Time taken: 0.192 seconds, Fetched 1 row(s)
spark-sql (default)> select array(1,2)<array(1,null);
false
Time taken: 0.127 seconds, Fetched 1 row(s)

But this maybe another issue to resolve it.

  1. Please add the code annotation before the test to clarify the test. Other good to me.

@mihaibudiu
Copy link
Contributor

Do you have any examples of simple queries that could generate other types? I can generate an array in a query with SELECT array[1, 2]; and a row with SELECT row(1, 2);

There are some examples in SqlOperatorTest.java, for instance nested arrays:

f.checkScalar("array_append(array[array[1, 2]], array[3, 4])", "[[1, 2], [3, 4]]",

@normanj-bitquill
Copy link
Contributor Author

@mihaibudiu I have fixed an issue with comparing rows and added tests with nesting (arrays in arrays, rows in arrays, etc).

@normanj-bitquill
Copy link
Contributor Author

normanj-bitquill commented Jul 12, 2024

@normanj-bitquill

  1. I find different data source has different behaviour about NULL value:
spark-sql (default)> select array(1,2)>array(1,null);
true
Time taken: 0.192 seconds, Fetched 1 row(s)
spark-sql (default)> select array(1,2)<array(1,null);
false
Time taken: 0.127 seconds, Fetched 1 row(s)

But this maybe another issue to resolve it.

  1. Please add the code annotation before the test to clarify the test. Other good to me.

@NobiGo

  1. I don't see any obvious way to support specifying the null order. It might help to consider how it should be configured. When building a Spark engine using Calcite, what would the ideal way of specifying whether NULLs should be first or last in array comparisons?

  2. I'm not sure what you mean. I added a comment before the tests to say what they are for.

@mihaibudiu mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Jul 13, 2024
* <, <=, >, >= now work for arrays and rows
* Can also sort arrays and rows
* Comparison is performed along corresponding indexes
* Longer arrays are considered greater
* null is considered greater than anything
* Cannot change whether nulls are first or last
@sonarqubecloud
Copy link

@mihaibudiu mihaibudiu merged commit 1cdf864 into apache:main Jul 15, 2024
@normanj-bitquill normanj-bitquill deleted the calcite-6454 branch August 27, 2024 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants