[CALCITE-5918] Add MAP function (enabled in Spark library) #3459

chucheng92 · 2023-10-07T14:31:22Z

https://issues.apache.org/jira/browse/CALCITE-5918

the spark map function has 2 different points with calcite map constructor:

use map(k, v, ...) rather than map[k, v, ...]
allows empty map such as map(); however calcite map constructor not support it.

site/_docs/reference.md

macroguo-ghy · 2023-10-08T06:36:59Z

core/src/main/codegen/templates/Parser.jj

+                return SqlStdOperatorTable.MAP_QUERY.createCall(s.end(this), args.get(0));
+            } else {
+                // MAP function e.g. "MAP(1, 2)" equivalent to standard "MAP[1, 2]"
+                return SqlLibraryOperators.MAP.createCall(s.end(this), args.getList());


I think we can check if the size of args is an even number.

I hope to retain the original parser logic here, because it will be detected later on OperandTypeChecker. And here is similar with spark's array handling and calcite array constructor. pls see: https://github.com/apache/calcite/pull/3141/files

Another benefit is that we can use some more human-readable error messages in predefined CalciteResource.

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

core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java

tanclary · 2023-10-12T15:54:39Z

core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java

@@ -873,6 +874,7 @@ Builder populate2() {
      map.put(MAP_VALUE_CONSTRUCTOR, value);
      map.put(ARRAY_VALUE_CONSTRUCTOR, value);
      defineMethod(ARRAY, BuiltInMethod.ARRAYS_AS_LIST.method, NullPolicy.NONE);
+      defineMethod(MAP, BuiltInMethod.MAP_FUNCTION.method, NullPolicy.NONE);


should we call it just MAP to be consistent with other functions?

In fact, there is a MAP object in BuiltInMethod, occupy this name. but yes, if we rename that, we can use MAP. I have updated it.

tanclary · 2023-10-12T15:54:52Z

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

@@ -5251,6 +5251,17 @@ public static Map mapFromArrays(List keysArray, List valuesArray) {
    return map;
  }

+  /** Support the MAP function. */
+  public static Map mapFunction(Object... args) {


same comment as above

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

core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java

tanclary · 2023-10-12T16:00:54Z

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

@@ -568,6 +569,9 @@ public static SqlOperandTypeChecker variadic(
  public static final SqlSingleOperandTypeChecker MAP_FROM_ENTRIES =
      new MapFromEntriesOperandTypeChecker();

+  public static final SqlSingleOperandTypeChecker MAP_FUNCTION =


same naming comment as above

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

tanclary · 2023-10-12T16:05:10Z

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

+        RelDataTypeFactory typeFactory,
+        List<RelDataType> argTypes) {
+      return Pair.of(
+          typeFactory.leastRestrictive(Util.quotientList(argTypes, 2, 0)),


Where are these constants coming from and what do they signify?

Util.quotientList(argTypes, 2, 0):
This extracts all elements at even indices from argTypes.
It represents the types of keys in the map as they are placed at even positions
e.g. 0, 2, 4, etc. details please see Util.quotientList.

std MapValueConstructor has same logic. see:

calcite/core/src/main/java/org/apache/calcite/sql/fun/SqlMapValueConstructor.java

Line 90 in 5151168

typeFactory.leastRestrictive(Util.quotientList(argTypes, 2, 0)),

I've added some comments here for better readability.

Can we add UT for typeFactory.leastRestrictive?

How about?
MAP('k1', 1, 'k2', 2.0, 'k3', '3')

Can we add UT for typeFactory.leastRestrictive?

How about? MAP('k1', 1, 'k2', 2.0, 'k3', '3')

yes, added it. thanks for reviewing.

site/_docs/reference.md

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

chucheng92 · 2023-10-13T12:03:04Z

hi, @tanclary I have solved the comments and updated the PR.

btw, I have rebased the main to use CALCITE-5570, I hope it won’t cause any trouble to your review.

chucheng92 · 2023-10-25T12:53:04Z

hi, @tanclary Sorry to pin you, I have solved the comments. could you help to re-check it?

tanclary

I think this looks pretty good. Left one naming comment

tanclary · 2023-10-25T15:46:01Z

core/src/main/java/org/apache/calcite/prepare/LixToRelTranslator.java

@@ -97,7 +97,7 @@ public <T> RelNode translate(Queryable<T> queryable) {
  public RelNode translate(Expression expression) {
    if (expression instanceof MethodCallExpression) {
      final MethodCallExpression call = (MethodCallExpression) expression;
-      BuiltInMethod method = BuiltInMethod.MAP.get(call.method);
+      BuiltInMethod method = BuiltInMethod.FUNCTIONS_MAPS.get(call.method);


why call it FUNCTIONS_MAPS?

because this is a static field in BuiltInMethod to store all the methods. it occupy 'MAP' name before, so I can't continue to use this name as our spark map method name, so i changed it, then we can use 'MAP' as method name as you suggested for consistency in naming. (I set 'MAP_FUNCTION' in BuiltInMethod previously).

one disadvantage I can think of here is that it may cause compatibility issues? If there is an upper-layer application/engine use java reflection to call this field. But is this as expected?

I'm ok if we need to reset spark MAP method to 'MAP_FUNCTION' name in BuiltInMethod and keep here unchanged. @tanclary what do you think?

tanclary

I think this looks good, you might find some of the sonarcloud reports helpful, it looks like they have some suggestions on refactoring the for loops and providing types for the generic Map, not sure how feasible these are in your case but you can just check and see if any of them prove useful.

chucheng92 · 2023-10-26T16:57:07Z

@tanclary

Thanks for the reminder. I checked sonar report and problems are roughly divided into three categories:

Assignment of index i in for-loop
The MAP of SqlFunctions does not specify a generic type
Reuse string constants

I have modified 1, but 2 and 3 actually not applicable for us.

2: The methods of SqlFunctions are all used by reflection. It can be written as generics or not. However, the current collection types Map and List in SqlFunctions are expressed using raw, I hope we can keep consistent.

3: is something like "CHAR(10) NOT NULL", there are multiple checkScalar, sonar wants us to define a variable and reuse it. In fact, it is not necessary. The current way of writing is more intuitive.

If you agree with these, I will squash the commits. WDYT?

tanclary

LGTM, think there might be some checkstyle issues but feel free to squash and ping me after!

chucheng92 · 2023-10-26T17:51:49Z

thank you all for patient reviewing! all comments are resolved.

@tanclary Thanks for approve! could you help to merge it? so we can add it in up-coming 1.36.

sonarcloud · 2023-10-26T19:17:37Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
14 Code Smells

98.5% Coverage
0.0% Duplication

tanclary

LGTM, sorry for the delay! Congrats on becoming a committer!

chucheng92 · 2023-10-30T13:41:15Z

LGTM, sorry for the delay! Congrats on becoming a committer!

thanks tanner. you helped me to a lot. :)

chucheng92 force-pushed the CALCITE-5918 branch 2 times, most recently from e592b6c to 752772f Compare October 8, 2023 04:04

macroguo-ghy reviewed Oct 8, 2023

View reviewed changes

chucheng92 force-pushed the CALCITE-5918 branch 3 times, most recently from a993e1e to 184d3f4 Compare October 11, 2023 07:30

tanclary requested changes Oct 12, 2023

View reviewed changes

chucheng92 force-pushed the CALCITE-5918 branch from 184d3f4 to 03993a9 Compare October 13, 2023 11:47

chucheng92 force-pushed the CALCITE-5918 branch from 03993a9 to 3ae9c56 Compare October 17, 2023 12:57

chucheng92 requested a review from tanclary October 17, 2023 16:13

tanclary reviewed Oct 25, 2023

View reviewed changes

chucheng92 force-pushed the CALCITE-5918 branch from 1b9531e to 84e97c1 Compare October 26, 2023 04:45

tanclary reviewed Oct 26, 2023

View reviewed changes

tanclary approved these changes Oct 26, 2023

View reviewed changes

chucheng92 force-pushed the CALCITE-5918 branch from 97435b7 to feb95fc Compare October 26, 2023 17:21

[CALCITE-5918] Add MAP function (enabled in Spark library)

73ea5c7

chucheng92 force-pushed the CALCITE-5918 branch from feb95fc to 73ea5c7 Compare October 26, 2023 18:47

chucheng92 requested a review from tanclary October 26, 2023 22:42

tanclary approved these changes Oct 27, 2023

View reviewed changes

tanclary merged commit b11f179 into apache:main Oct 27, 2023
18 checks passed

chucheng92 deleted the CALCITE-5918 branch October 30, 2023 13:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-5918] Add MAP function (enabled in Spark library) #3459

[CALCITE-5918] Add MAP function (enabled in Spark library) #3459

chucheng92 commented Oct 7, 2023

macroguo-ghy Oct 8, 2023

chucheng92 Oct 8, 2023 •

edited

Loading

tanclary Oct 12, 2023

chucheng92 Oct 13, 2023 •

edited

Loading

tanclary Oct 12, 2023

chucheng92 Oct 13, 2023

tanclary Oct 12, 2023

chucheng92 Oct 13, 2023

tanclary Oct 12, 2023

chucheng92 Oct 13, 2023 •

edited

Loading

JiajunBernoulli Oct 26, 2023

chucheng92 Oct 26, 2023

chucheng92 commented Oct 13, 2023 •

edited

Loading

chucheng92 commented Oct 25, 2023

tanclary left a comment

tanclary Oct 25, 2023

chucheng92 Oct 25, 2023 •

edited

Loading

tanclary left a comment •

edited

Loading

chucheng92 commented Oct 26, 2023 •

edited

Loading

tanclary left a comment

chucheng92 commented Oct 26, 2023 •

edited

Loading

sonarcloud bot commented Oct 26, 2023

tanclary left a comment

chucheng92 commented Oct 30, 2023

[CALCITE-5918] Add MAP function (enabled in Spark library) #3459

[CALCITE-5918] Add MAP function (enabled in Spark library) #3459

Conversation

chucheng92 commented Oct 7, 2023

Choose a reason for hiding this comment

chucheng92 Oct 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chucheng92 Oct 13, 2023 • edited Loading

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

chucheng92 Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chucheng92 commented Oct 13, 2023 • edited Loading

chucheng92 commented Oct 25, 2023

tanclary left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chucheng92 Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

tanclary left a comment • edited Loading

Choose a reason for hiding this comment

chucheng92 commented Oct 26, 2023 • edited Loading

tanclary left a comment

Choose a reason for hiding this comment

chucheng92 commented Oct 26, 2023 • edited Loading

sonarcloud bot commented Oct 26, 2023

tanclary left a comment

Choose a reason for hiding this comment

chucheng92 commented Oct 30, 2023

chucheng92 Oct 8, 2023 •

edited

Loading

chucheng92 Oct 13, 2023 •

edited

Loading

chucheng92 Oct 13, 2023 •

edited

Loading

chucheng92 commented Oct 13, 2023 •

edited

Loading

chucheng92 Oct 25, 2023 •

edited

Loading

tanclary left a comment •

edited

Loading

chucheng92 commented Oct 26, 2023 •

edited

Loading

chucheng92 commented Oct 26, 2023 •

edited

Loading