[FLINK-31098][table] Add ARRAY_SIZE function. #21947

liuyongvs · 2023-02-16T05:04:55Z

What is the purpose of the change
This is an implementation of ARRAY_SIZE/ARRAY_REMOVE
Brief change log
ARRAY_SIZE for Table API and SQL
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-02-16T05:10:46Z

CI report:

f7910e6 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

liuyongvs · 2023-02-16T10:01:43Z

@snuyanzin could you help review it?

snuyanzin · 2023-02-16T10:37:54Z

Thank you for your contribution.
To be honest I'm a bit skeptic about ARRAY_SIZE, since this functionality already exists in Flink like CARDINALITY.
Also please see at my comment in corresponding jira issue.

snuyanzin · 2023-02-16T10:52:27Z

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

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


I would also add tests for cases with not null elements and removal of null.
E.g.

select array_remove(array[1, 2], cast(null as int));

i would expect same array as output, or how do other systems behave?

have added ARRAY_REMOVE(f0, cast(NULL AS INT))

snuyanzin · 2023-02-16T11:02:15Z

docs/data/sql_functions.yml

+  - sql: ARRAY_SIZE(haystack)
+    table: haystack.arraySize()
+    description: Returns the size of an array. If the array itself is null, the function will return null.
+  - sql: ARRAY_REMOVE(haystack)


didn't get ...
shouldn't ARRAY_REMOVE have 2 args?

snuyanzin · 2023-02-16T11:03:17Z

flink-python/docs/reference/pyflink.table/expressions.rst

    Expression.array_contains
    Expression.array_distinct
+    Expression.array_size
+    Expression.array_remove


I would recommend keeping abc order (at least for array_* expressions) here since it simplifies search/navigation

snuyanzin · 2023-02-16T11:10:29Z

...-table-common/src/main/java/org/apache/flink/table/functions/BuiltInFunctionDefinitions.java

+                            sequence(
+                                    Arrays.asList("haystack", "needle"),
+                                    Arrays.asList(
+                                            logical(LogicalTypeRoot.ARRAY), ARRAY_ELEMENT_ARG)))


ARRAY_ELEMENT_ARG not sure about this since array could consists of non null elements and a query could try to remove null e.g.

SELECT array_remove(array['abc', 'klm', 'xyz'], cast(null as varchar));

i test remove null, why should do cast(null as varchar)

you test remove null from array with nullable types
I'm asking to test remove of null from array with NON nullable types
that's the difference

have added test ARRAY_REMOVE(f2, cast(NULL AS VARCHAR))

i didn't go into details.
However there is a simple test to check if it's working or not: build flink
start flink (standalone is enough)
run sql-client
submit a query

select array_remove(array[1, 2, 3], cast(null as int));

Expected result is array[1, 2, 3]
Actual result: it fails with exception

same query e.g. in postgres is working fine

i test SELECT array_contains(array[1, 2, 3], cast(null as int)) it also fails with exception.
the reason is ARRAY_ELEMENT_ARG make the second arg must be same with array element type
Caused by: org.apache.flink.table.api.ValidationException: Invalid function call:
array_remove(ARRAY NOT NULL, INT).

so select array_remove(array[1, 2, 3, null], cast(null as int));
return array[1, 2, 3].

do we should also fix the array_contains.

yes, looks like should be fixed as well, however could be done in a separate pr

snuyanzin · 2023-02-16T11:11:48Z

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

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


I would suggest to add more complex cases like
removal from array of arrays or array of maps or array of rows

i have added array<array> and array<map<int ,string>> unit test.

liuyongvs · 2023-02-17T04:53:58Z

hi @snuyanzin please review it again, i fix all your review infos

liuyongvs · 2023-02-17T09:02:45Z

hi @snuyanzin have changed, i reorganized the commits and make it two commit

snuyanzin · 2023-02-17T11:07:07Z

docs/data/sql_functions.yml

+  - sql: ARRAY_SIZE(haystack)
+    table: haystack.arraySize()
+    description: Returns the size of an array. If the array itself is null, the function will return null.


I still don't understand why ARRAY_SIZE is required if there is already CARDINALITY which does same thing.
There is a discussion in jira about that

To align the other engines like spark,snowflake, so that users can migrate easily

Well that's very arguable since different engines support different syntax
BigQuery has ARRAY_LENGTH
Hive has SIZE
Vertica has ARRAY_LENGTH
Postgresql ARRAY_LENGTH

why to support only snowflake and spark?
Support of everything seems to be an overkill...
I guess somewhere it should be stopped

thanks for your details response, i will drop the array_size commit.

liuyongvs · 2023-02-20T04:27:31Z

hi @snuyanzin please review it again

liuyongvs · 2023-02-21T03:35:44Z

hi @snuyanzin do you have time to review it again?

snuyanzin · 2023-03-30T20:13:37Z

Closing the issue since #21947 is closed

[FLINK-31098][table] Add ARRAY_SIZE function.

5e6e4d1

flinkbot added the component=TableSQL/Planner label Feb 16, 2023

snuyanzin reviewed Feb 16, 2023

View reviewed changes

snuyanzin requested changes Feb 16, 2023

View reviewed changes

liuyongvs force-pushed the array_size branch from aee8f4e to 1342c6b Compare February 17, 2023 04:17

liuyongvs requested a review from snuyanzin February 17, 2023 04:54

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

f7910e6

liuyongvs force-pushed the array_size branch from 1342c6b to f7910e6 Compare February 17, 2023 08:59

snuyanzin requested changes Feb 17, 2023

View reviewed changes

liuyongvs requested a review from snuyanzin February 17, 2023 16:34

liuyongvs mentioned this pull request Mar 10, 2023

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

Merged

liuyongvs closed this Mar 28, 2023

[FLINK-31098][table] Add ARRAY_SIZE function. #21947

[FLINK-31098][table] Add ARRAY_SIZE function. #21947

Uh oh!

Conversation

liuyongvs commented Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flinkbot commented Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

liuyongvs commented Feb 16, 2023

Uh oh!

snuyanzin commented Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snuyanzin Feb 16, 2023 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

snuyanzin Feb 17, 2023 • 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!

snuyanzin Feb 21, 2023 • 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!

liuyongvs commented Feb 17, 2023

Uh oh!

liuyongvs commented Feb 17, 2023

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!

liuyongvs commented Feb 20, 2023

Uh oh!

liuyongvs commented Feb 21, 2023

Uh oh!

snuyanzin commented Mar 30, 2023

Uh oh!

Uh oh!

liuyongvs commented Feb 16, 2023 •

edited

Loading

flinkbot commented Feb 16, 2023 •

edited

Loading

snuyanzin commented Feb 16, 2023 •

edited

Loading

snuyanzin Feb 16, 2023 •

edited

Loading

snuyanzin Feb 17, 2023 •

edited

Loading

snuyanzin Feb 21, 2023 •

edited

Loading