[CALCITE-5695] Add MAP_KEYS, MAP_VALUES function (enabled in Spark li… #3194

liuyongvs · 2023-05-10T06:56:59Z

MAP_KEYS - Returns an unordered array containing the keys of the map.
MAP_VALUES - Returns an unordered array containing the values of the map.
For more details
https://spark.apache.org/docs/latest/sql-ref-functions-builtin.html

liuyongvs · 2023-05-10T06:58:01Z

hi @JiajunBernoulli could you also help review it?

zoudan · 2023-05-10T08:48:00Z

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

@@ -3797,6 +3798,16 @@ public static List distinct(List list) {
    return new ArrayList<>(result);
  }

+  /** Support the MAP_KEYS function. */
+  public static List mapKeys(Map map) {
+    return (List) map.keySet().stream().collect(Collectors.toList());


what is the expected result if the input map is null?

return null @zoudan

Then we should handle null here? It seems a NPE Exception will be thrown?

@zoudan not needs. the runtime will does it like other engine， the NullPolicy does that

and why i doesn't add unit test map_keys(cast(null as map[keyType, valueType])) is because the calcite doesn't supported, which you can see here #3189
java.lang.RuntimeException: Error while parsing query: select cast(map['washington', 1, null, 44] as map[CHAR(10), INTEGER NOT NULL>] from (values (1))
at org.apache.calcite.sql.test.AbstractSqlTester.parseAndValidate(AbstractSqlTester.java:157)
at org.apache.calcite.sql.test.AbstractSqlTester.validateAndThen(AbstractSqlTester.java:248)
at org.apache.calcite.sql.test.AbstractSqlTester.checkColumnType(AbstractSqlTester.java:177)
at org.apache.calcite.test.SqlValidatorFixture.columnType(

Why not use new ArrayList<>(map.keySets())?

tanclary · 2023-05-10T16:22:32Z

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

+  /** Tests {@code MAP_KEYS} function from Spark. */
+  @Test void testMapKeysFunc() {
+    final SqlOperatorFixture f = fixture()
+        .setFor(SqlLibraryOperators.MAP_KEYS)


I think it could be good to write a test that fails if library is not set to Spark (you only want it to work for Spark).

added @tanclary

julianhyde · 2023-05-10T20:13:48Z

site/_docs/reference.md

@@ -2723,6 +2723,8 @@ BigQuery's type system uses confusingly different names for types and functions:
 | m | TO_BASE64(string)                              | Converts the *string* to base-64 encoded form and returns a encoded string
 | b m | FROM_BASE64(string)                          | Returns the decoded result of a base-64 *string* as a string
 | b o | LTRIM(string)                                | Returns *string* with all blanks removed from the start
+| s | MAP_KEYS(map)                                  | Returns an unordered array containing the keys of the map.


fixed @julianhyde

liuyongvs · 2023-05-11T01:52:58Z

hi @julianhyde @tanclary @zoudan fix all your reviews, please have a look again,thanks. and i will supports all the spark collection functions latter.

liuyongvs · 2023-05-12T01:47:03Z

hi @JiajunBernoulli do you have time to review it ?

JiajunBernoulli · 2023-05-12T01:51:32Z

hi @JiajunBernoulli do you have time to review it ?

I usually only have time on weekends.

So I will review it tomorrow.

julianhyde · 2023-05-12T17:51:48Z

@liuyongvs Can you respond to my comment in Jira? What is an 'unordered array'?

JiajunBernoulli · 2023-05-13T02:49:28Z

site/_docs/reference.md

@@ -2723,6 +2723,8 @@ BigQuery's type system uses confusingly different names for types and functions:
 | m | TO_BASE64(string)                              | Converts the *string* to base-64 encoded form and returns a encoded string
 | b m | FROM_BASE64(string)                          | Returns the decoded result of a base-64 *string* as a string
 | b o | LTRIM(string)                                | Returns *string* with all blanks removed from the start
+| s | MAP_KEYS(map)                                  | Returns an unordered array containing the keys of the map
+| s | MAP_VALUES(map)                                | Returns an unordered array containing the values of the map


map should be *map*.

JiajunBernoulli · 2023-05-13T03:08:02Z

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

+   * <p>For example, given {@code (INTEGER, DATE) MAP}, returns
+   * {@code DATE ARRAY}.
+   */
+  public static final SqlReturnTypeInference TO_MAP_VALUES_NULLABLE =


We should have two types: not null and nullable.

public static final SqlReturnTypeInference TO_MAP_KEYS = ARG0.andThen(SqlTypeTransforms.TO_MAP_KEYS); public static final SqlReturnTypeInference TO_MAP_KEYS_NULLABLE = TO_MAP_KEYS.andThen(SqlTypeTransforms.TO_NULLABLE);

JiajunBernoulli · 2023-05-13T03:14:30Z

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

+
+  /** Support the MAP_VALUES function. */
+  public static List mapValues(Map map) {
+    return (List) map.values().stream().collect(Collectors.toList());


return new ArrayList<>(map.values());

I think it's clearer and there won't be NPE.

JiajunBernoulli · 2023-05-13T03:18:23Z

I left some comments for the PR.

And we should discuss 'unordered array' in JIRA.

liuyongvs · 2023-05-14T08:37:48Z

fix all your review @JiajunBernoulli
and the spark doesn't support MULTISET type. an unordered array means the element doesn't sort.
and i response in the issue, sorry for late response @julianhyde

JiajunBernoulli · 2023-05-15T02:00:18Z

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

@@ -3797,6 +3798,16 @@ public static List distinct(List list) {
    return new ArrayList<>(result);
  }

+  /** Support the MAP_KEYS function. */
+  public static List mapKeys(Map map) {
+    return (List) map.keySet().stream().collect(Collectors.toList());


Why not use new ArrayList<>(map.keySet())?

fixed @JiajunBernoulli

liuyongvs · 2023-05-15T03:53:12Z

fixed , thanks for your details review @JiajunBernoulli. do you also have any comments.
If not, i will squash the commits.

liuyongvs · 2023-05-16T11:19:41Z

have squashed the commits, could anyone merge it thanks @JiajunBernoulli @julianhyde @zoudan

JiajunBernoulli · 2023-05-17T01:49:08Z

Conflicting files because #3198

Please resolve them.

liuyongvs · 2023-05-17T02:59:48Z

hi @JiajunBernoulli fixed the conflict and ci passed

JiajunBernoulli · 2023-05-18T01:58:21Z

Please delete '.' in commit message.

…brary)

liuyongvs · 2023-05-18T02:04:54Z

@JiajunBernoulli fixed all the current prs message ending with '.'

sonarqubecloud · 2023-05-18T02:16:23Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
10 Code Smells

95.7% Coverage
0.0% Duplication

zoudan reviewed May 10, 2023

View reviewed changes

liuyongvs force-pushed the map_keys_map_values branch 2 times, most recently from 79ab686 to f1fad81 Compare May 10, 2023 10:10

JiajunBernoulli self-assigned this May 10, 2023

tanclary reviewed May 10, 2023

View reviewed changes

julianhyde reviewed May 10, 2023

View reviewed changes

liuyongvs requested review from julianhyde, tanclary and zoudan May 11, 2023 01:53

liuyongvs force-pushed the map_keys_map_values branch from 1110d8d to 48def8d Compare May 12, 2023 01:42

JiajunBernoulli reviewed May 13, 2023

View reviewed changes

JiajunBernoulli added the discussion-in-jira There's open discussion in JIRA to be resolved before proceeding with the PR label May 13, 2023

liuyongvs requested a review from JiajunBernoulli May 14, 2023 12:26

JiajunBernoulli reviewed May 15, 2023

View reviewed changes

liuyongvs requested a review from JiajunBernoulli May 15, 2023 03:51

liuyongvs force-pushed the map_keys_map_values branch 2 times, most recently from ecd2ba1 to 822509a Compare May 16, 2023 11:18

liuyongvs force-pushed the map_keys_map_values branch 2 times, most recently from 2d5c4b0 to 42c02f2 Compare May 17, 2023 00:01

JiajunBernoulli removed the discussion-in-jira There's open discussion in JIRA to be resolved before proceeding with the PR label May 17, 2023

JiajunBernoulli mentioned this pull request May 17, 2023

[CALCITE-5704] Add ARRAY_EXCEPT, ARRAY_INTERSECT, ARRAY_UNION function (enabled in Spark library) #3202

Merged

liuyongvs force-pushed the map_keys_map_values branch from 42c02f2 to a212a06 Compare May 17, 2023 02:35

[CALCITE-5695] Add MAP_KEYS, MAP_VALUES function (enabled in Spark li…

281d094

…brary)

liuyongvs force-pushed the map_keys_map_values branch from a212a06 to 281d094 Compare May 18, 2023 02:01

JiajunBernoulli approved these changes May 19, 2023

View reviewed changes

JiajunBernoulli added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label May 19, 2023

JiajunBernoulli merged commit 9c33d7a into apache:main May 21, 2023

[CALCITE-5695] Add MAP_KEYS, MAP_VALUES function (enabled in Spark li… #3194

[CALCITE-5695] Add MAP_KEYS, MAP_VALUES function (enabled in Spark li… #3194

Uh oh!

Conversation

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

Uh oh!

Uh oh!

liuyongvs commented May 10, 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!

liuyongvs May 10, 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!

liuyongvs commented May 11, 2023

Uh oh!

liuyongvs commented May 12, 2023

Uh oh!

JiajunBernoulli commented May 12, 2023

Uh oh!

julianhyde commented May 12, 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!

JiajunBernoulli commented May 13, 2023

Uh oh!

liuyongvs commented May 14, 2023 • 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!

liuyongvs commented May 15, 2023

Uh oh!

liuyongvs commented May 16, 2023

Uh oh!

JiajunBernoulli commented May 17, 2023

Uh oh!

liuyongvs commented May 17, 2023

Uh oh!

JiajunBernoulli commented May 18, 2023

Uh oh!

liuyongvs commented May 18, 2023

Uh oh!

sonarqubecloud bot commented May 18, 2023

Uh oh!

Uh oh!

liuyongvs commented May 10, 2023 •

edited

Loading

liuyongvs May 10, 2023 •

edited

Loading

liuyongvs commented May 14, 2023 •

edited

Loading