[FLINK-31102][table] Add ARRAY_REMOVE function. #22144

liuyongvs · 2023-03-10T03:43:39Z

What is the purpose of the change
This is an implementation of ARRAY_REMOVE
Brief change log
ARRAY_REMOVE for Table API and SQL
```
Syntax:
array_remove(array, needle)

Arguments:
array: An ARRAY to be handled.

Returns:
An ARRAY. If array is NULL, the result is NULL. 
Examples:

SELECT array_remove(array[1, 2, 3, null, 3], 3); 
-- [1,2,null]
```
See also
spark https://spark.apache.org/docs/latest/api/sql/index.html#array_remove

presto https://prestodb.io/docs/current/functions/array.html

postgresql https://www.postgresql.org/docs/12/functions-array.html#ARRAY-FUNCTIONS-TABLE
Verifying this change
This change added tests in CollectionFunctionsITCase
Does this pull request potentially affect one of the following parts:
Dependencies (does it add or upgrade a dependency): ( no)
The public API, i.e., is any changed class annotated with @public(Evolving): (yes )
The serializers: (no)
The runtime per-record code paths (performance sensitive): ( no)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: ( no)
The S3 file system connector: ( no)
Documentation
Does this pull request introduce a new feature? (yes)
If yes, how is the feature documented? (docs)

flinkbot · 2023-03-10T03:47:22Z

CI report:

9b66276 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

liuyongvs · 2023-03-10T03:54:00Z

hi @snuyanzin @twalthr after talking in #21947
Let me make a quick summary

do not need support array_size, which can be replaced by CARDINALITY. so drop the commit
the select array_remove(array[1, 2], cast(null as int)); throw exception, which is caused by array_contains. and it is fixed and merge in 10dce7c
the java equals may cause bug in array_distinct, should use ExpressionEvaluator, suggested by @twalthr which is fixed and merge in 6797d6f
the last problem found in [FLINK-31377][table] Fix array_contains ArrayData.ElementGetter shoul… #22143 (comment), will also need to be solved

so it is the time to implment other array functions. so i submit a new PR, could you help to review

snuyanzin · 2023-03-10T08:10:41Z

thanks for the contribution
can you please fix description?
e.g.

I didn't get how ARRAY_SIZEis related to this PR
Syntax:
array_remove(array)

is it possible to invoke array_remove for one arg? What is the expected output?
SELECT array_remove(array(1, 2, 3, null, 3), 3);

it will not work in Flink since it is not Flink syntax

liuyongvs · 2023-03-10T08:22:34Z

@snuyanzin
1.it is here after talking in #21947, which you reviewed before.
2. is it possible to invoke array_remove for one arg -> no possible. sorry. it is my misspelling and i have fixed it 。
3. it is also misspelling, array[1, 2, 3, null, 3], i have fixed it

liuyongvs · 2023-03-13T13:09:21Z

hi @snuyanzin could you have time to help review it

snuyanzin · 2023-03-13T21:13:24Z

...ntime/src/main/java/org/apache/flink/table/runtime/functions/scalar/ArrayRemoveFunction.java

+                if ((element == null && needle != null)
+                        || (element != null && needle == null)
+                        || (element != null
+                                && needle != null
+                                && !(boolean) equalityHandle.invoke(element, needle))) {


This could be simplified e.g. check for the negative case and continue

have done @snuyanzin

snuyanzin · 2023-03-13T21:19:15Z

...lanner/src/test/java/org/apache/flink/table/planner/functions/CollectionFunctionsITCase.java

+                        .testResult(
+                                $("f3").arrayRemove(row(true, LocalDate.of(1990, 10, 14))),
+                                "ARRAY_REMOVE(f3, (TRUE, DATE '1990-10-14'))",
+                                new Row[] {Row.of(true, LocalDate.of(2022, 4, 20)), null},
+                                DataTypes.ARRAY(
+                                                DataTypes.ROW(
+                                                        DataTypes.BOOLEAN(), DataTypes.DATE()))
+                                        .nullable())
+                        .testResult(
+                                $("f3").arrayRemove(row(false, LocalDate.of(1990, 10, 14))),
+                                "ARRAY_REMOVE(f3, (FALSE, DATE '1990-10-14'))",
+                                new Row[] {
+                                    Row.of(true, LocalDate.of(2022, 4, 20)),
+                                    Row.of(true, LocalDate.of(1990, 10, 14)),
+                                    null
+                                },
+                                DataTypes.ARRAY(
+                                                DataTypes.ROW(
+                                                        DataTypes.BOOLEAN(), DataTypes.DATE()))
+                                        .nullable())


Under the hood there are a lot of Flink jobs executed.
From one side testing is important but tests should be well chosen.
It seems e.g. one of this could be removed since they are very similar.
I guess up to 4-5 test cases should be enough.
WDYT?

i agree with you @snuyanzin
These examples cover a variety of cases, and the rest of the functions don't have to cover as many cases as possible, but some of the early functions still need to cover as many cases as possible, so that users can see how to use them and find bugs.

have done @snuyanzin

liuyongvs · 2023-03-14T13:17:07Z

@flinkbot run azure

liuyongvs · 2023-03-15T06:19:27Z

hi @snuyanzin could you help review it again?

docs/data/sql_functions.yml

flink-python/pyflink/table/expression.py

.../flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/BaseExpressions.java

liuyongvs · 2023-03-17T02:13:18Z

@snuyanzin rebase to fix the conflicts and reviews from you.

liuyongvs · 2023-03-20T01:57:48Z

hi @snuyanzin could you help review it again?

snuyanzin · 2023-03-20T22:52:41Z

...lanner/src/test/java/org/apache/flink/table/planner/functions/CollectionFunctionsITCase.java

+                                $("f0").arrayRemove(
+                                                lit(null, DataTypes.SMALLINT())
+                                                        .cast(DataTypes.INT())),
+                                "ARRAY_REMOVE(f0, cast(NULL AS INT))",


Suggested change

"ARRAY_REMOVE(f0, cast(NULL AS INT))",

"ARRAY_REMOVE(f0, CAST(NULL AS INT))",

All SQL keywords should be in uppercase

snuyanzin · 2023-03-20T22:53:05Z

...lanner/src/test/java/org/apache/flink/table/planner/functions/CollectionFunctionsITCase.java

+                                $("f2").arrayRemove(
+                                                lit(null, DataTypes.STRING())
+                                                        .cast(DataTypes.STRING())),
+                                "ARRAY_REMOVE(f2, cast(NULL AS VARCHAR))",


Suggested change

"ARRAY_REMOVE(f2, cast(NULL AS VARCHAR))",

"ARRAY_REMOVE(f2, CAST(NULL AS VARCHAR))",

All SQL keywords should be in uppercase

ok, fixed, thanks

snuyanzin · 2023-03-20T22:53:27Z

...lanner/src/test/java/org/apache/flink/table/planner/functions/CollectionFunctionsITCase.java

+                                        .nullable())
+                        .testResult(
+                                $("f3").arrayRemove(null),
+                                "ARRAY_REMOVE(f3, null)",


Suggested change

"ARRAY_REMOVE(f3, null)",

"ARRAY_REMOVE(f3, NULL)",

snuyanzin · 2023-03-20T22:53:41Z

...lanner/src/test/java/org/apache/flink/table/planner/functions/CollectionFunctionsITCase.java

+                        // ARRAY<ARRAY<INT>>
+                        .testResult(
+                                $("f5").arrayRemove(new Integer[] {0}),
+                                "ARRAY_REMOVE(f5, array[0])",


Suggested change

"ARRAY_REMOVE(f5, array[0])",

"ARRAY_REMOVE(f5, ARRAY[0])",

snuyanzin · 2023-03-20T22:54:21Z

...lanner/src/test/java/org/apache/flink/table/planner/functions/CollectionFunctionsITCase.java

+                                                DataTypes.ROW(
+                                                        DataTypes.BOOLEAN(), DataTypes.DATE()))
+                                        .nullable())
+                        // ARRAY<INT> with null elements


Suggested change

// ARRAY<INT> with null elements

// ARRAY<INT> with NULL elements

snuyanzin · 2023-03-20T22:54:33Z

...lanner/src/test/java/org/apache/flink/table/planner/functions/CollectionFunctionsITCase.java

+                                "ARRAY_REMOVE(f5, array[0])",
+                                new Integer[][] {new Integer[] {1, null, 3}, new Integer[] {1}},
+                                DataTypes.ARRAY(DataTypes.ARRAY(DataTypes.INT()).nullable()))
+                        // ARRAY<Map<INT, STRING>> with null elements


Suggested change

// ARRAY<Map<INT, STRING>> with null elements

// ARRAY<MAP<INT, STRING>> with null elements

snuyanzin · 2023-03-20T22:57:38Z

...lanner/src/test/java/org/apache/flink/table/planner/functions/CollectionFunctionsITCase.java

+                        .testResult(
+                                $("f2").arrayRemove("Hello"),
+                                "ARRAY_REMOVE(f2, 'Hello')",
+                                new String[] {"World"},
+                                DataTypes.ARRAY(DataTypes.STRING()).notNull())
+                        .testResult(
+                                $("f2").arrayRemove(
+                                                lit(null, DataTypes.STRING())
+                                                        .cast(DataTypes.STRING())),
+                                "ARRAY_REMOVE(f2, cast(NULL AS VARCHAR))",


i do not see a big difference between these tests and tests for int types... ARRAY_REMOVE invokes same code for both.... Probably just one (int or varchar is enough)

liuyongvs · 2023-03-22T06:49:44Z

hi @snuyanzin fix your reviews

snuyanzin

Thanks for addressing comments
LGTM

will merge soon

liuyongvs · 2023-03-25T02:08:16Z

hi @snuyanzin , will it be merged now?

flinkbot added the component=TableSQL/Planner label Mar 10, 2023

liuyongvs force-pushed the array_remove branch from 603c5f9 to 2e33d70 Compare March 10, 2023 07:39

snuyanzin reviewed Mar 13, 2023

View reviewed changes

liuyongvs requested a review from snuyanzin March 14, 2023 05:46

snuyanzin reviewed Mar 16, 2023

View reviewed changes

docs/data/sql_functions.yml Outdated Show resolved Hide resolved

snuyanzin reviewed Mar 16, 2023

View reviewed changes

flink-python/pyflink/table/expression.py Outdated Show resolved Hide resolved

snuyanzin reviewed Mar 16, 2023

View reviewed changes

.../flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/BaseExpressions.java Outdated Show resolved Hide resolved

liuyongvs force-pushed the array_remove branch from 7ad5d2d to 03b76d2 Compare March 17, 2023 02:10

liuyongvs requested review from snuyanzin and removed request for snuyanzin March 17, 2023 02:13

liuyongvs force-pushed the array_remove branch 2 times, most recently from 7f7c7ba to d996a53 Compare March 17, 2023 08:14

[FLINK-31102][table] Add ARRAY_REMOVE function.

cbf830a

liuyongvs force-pushed the array_remove branch from d996a53 to cbf830a Compare March 17, 2023 08:22

liuyongvs requested a review from snuyanzin March 17, 2023 08:36

snuyanzin reviewed Mar 20, 2023

View reviewed changes

[FLINK-31102][table] fix review.

9b66276

liuyongvs requested a review from snuyanzin March 21, 2023 06:38

snuyanzin approved these changes Mar 22, 2023

View reviewed changes

liuyongvs requested a review from snuyanzin March 24, 2023 07:25

snuyanzin merged commit f3c653e into apache:master Mar 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-31102][table] Add ARRAY_REMOVE function. #22144

[FLINK-31102][table] Add ARRAY_REMOVE function. #22144

liuyongvs commented Mar 10, 2023 •

edited

flinkbot commented Mar 10, 2023 •

edited

liuyongvs commented Mar 10, 2023 •

edited

snuyanzin commented Mar 10, 2023

liuyongvs commented Mar 10, 2023 •

edited

liuyongvs commented Mar 13, 2023

snuyanzin Mar 13, 2023

liuyongvs Mar 14, 2023

snuyanzin Mar 13, 2023

liuyongvs Mar 14, 2023

liuyongvs Mar 14, 2023

liuyongvs commented Mar 14, 2023

liuyongvs commented Mar 15, 2023

liuyongvs commented Mar 17, 2023

liuyongvs commented Mar 20, 2023

snuyanzin Mar 20, 2023

snuyanzin Mar 20, 2023

liuyongvs Mar 21, 2023

snuyanzin Mar 20, 2023

snuyanzin Mar 20, 2023

snuyanzin Mar 20, 2023

snuyanzin Mar 20, 2023

snuyanzin Mar 20, 2023

liuyongvs commented Mar 22, 2023

snuyanzin left a comment

liuyongvs commented Mar 25, 2023

	"ARRAY_REMOVE(f0, cast(NULL AS INT))",
	"ARRAY_REMOVE(f0, CAST(NULL AS INT))",

	"ARRAY_REMOVE(f2, cast(NULL AS VARCHAR))",
	"ARRAY_REMOVE(f2, CAST(NULL AS VARCHAR))",

	// ARRAY<INT> with null elements
	// ARRAY<INT> with NULL elements

	// ARRAY<Map<INT, STRING>> with null elements
	// ARRAY<MAP<INT, STRING>> with null elements

[FLINK-31102][table] Add ARRAY_REMOVE function. #22144

[FLINK-31102][table] Add ARRAY_REMOVE function. #22144

Conversation

liuyongvs commented Mar 10, 2023 • edited

flinkbot commented Mar 10, 2023 • edited

CI report:

liuyongvs commented Mar 10, 2023 • edited

snuyanzin commented Mar 10, 2023

liuyongvs commented Mar 10, 2023 • edited

liuyongvs commented Mar 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuyongvs commented Mar 14, 2023

liuyongvs commented Mar 15, 2023

liuyongvs commented Mar 17, 2023

liuyongvs commented Mar 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuyongvs commented Mar 22, 2023

snuyanzin left a comment

Choose a reason for hiding this comment

liuyongvs commented Mar 25, 2023

liuyongvs commented Mar 10, 2023 •

edited

flinkbot commented Mar 10, 2023 •

edited

liuyongvs commented Mar 10, 2023 •

edited

liuyongvs commented Mar 10, 2023 •

edited