[CALCITE-5744] Add MAP_FROM_ARRAYS, STR_TO_MAP function (enabled in Spark library) #3238

liuyongvs · 2023-06-02T11:23:54Z

STR_TO_MAP(string[, stringDelimiter[, keyValueDelimiter]])

Returns a map after splitting the string into key/value pairs using delimiters. Default delimiters are ',' for stringDelimiter and ':' for keyValueDelimiter Both pairDelim and keyValueDelim are treated as regular expressions.

Examples:

> SELECT str_to_map('a:1,b:2,c:3', ',', ':');

- {"a":"1","b":"2","c":"3"}

> SELECT str_to_map('a');
 {"a":null}

MAP_FROM_ARRAYS (array1, array2)

Returns a map created from an array1 and *array2. Note that the lengths of two arrays should be the same

SELECT map_from_arrays(array(1.0, 3.0), array('2', '4'));
{1.0:"2",3.0:"4"}

liuyongvs · 2023-06-03T14:09:25Z

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

+        requireNonNull(keysArrayType.getComponentType(), "inferred key type"),
+        requireNonNull(valuesArrayType.getComponentType(), "inferred value type"),
+        nullable);
+  }


comments:
final boolean nullable = keysArrayType.isNullable() || valuesArrayType.isNullable();
this should be ,so i add the unit test
f.checkType("str_to_map(cast(null as varchar))",
"(VARCHAR, VARCHAR) MAP");

liuyongvs · 2023-06-03T14:12:48Z

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

+  /** Support the STR_TO_MAP function. */
+  public static Map strToMap(String string, String stringDelimiter, String keyValueDelimiter) {
+    final Map map = new LinkedHashMap();
+    final String[] keyValues = string.split(stringDelimiter, -1);


the limit should be -1, which aligns with spark.
what is the difference with limit -1, you can se here
https://stackabuse.com/how-to-split-a-string-in-java/

liuyongvs · 2023-06-03T14:14:02Z

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

+  /** Support the MAP_FROM_ARRAYS function. */
+  public static Map mapFromArrays(List keysArray, List valuesArray) {
+    if (keysArray.size() != valuesArray.size()) {
+      throw new IllegalArgumentException(


the length check is need, and i add a negative test for it

f.checkFails("map_from_arrays(array[1, 2], array['foo'])", "Invalid function MAP_FROM_ARRAYS call:\n" + "The length of the keys array 2 is not equal to the length of the values array 1", true);

liuyongvs · 2023-06-04T16:48:06Z

hi @tanclary @JiajunBernoulli @MasseGuillaume do you have time to help review ?

tanclary

Left some comments, hope this helps!

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

tanclary · 2023-06-06T00:02:56Z

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

+
+    @Override Expression implementSafe(RexToLixTranslator translator,
+        RexCall call, List<Expression> argValueList) {
+      String defaultStringDelimiter = ",";


is there a way to use the "SPLIT" implementation offered by Bigquery here? Some of this looks very similar.

but not same

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

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

tanclary · 2023-06-06T00:06:36Z

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

+  /** Support the MAP_FROM_ARRAYS function. */
+  public static Map mapFromArrays(List keysArray, List valuesArray) {
+    if (keysArray.size() != valuesArray.size()) {
+      throw new IllegalArgumentException(


can this exception be added as a constant to Resources like the other errors thrown from this class?

use Resources

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

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

liuyongvs · 2023-06-06T04:07:28Z

hi @tanclary thanks for your review and your several valuable suggestion +1

and do you have time to look again?

tanclary · 2023-06-06T15:08:42Z

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

@@ -965,6 +965,31 @@ private static RelDataType arrayReturnType(SqlOperatorBinding opBinding) {
          ReturnTypes.TO_MAP_VALUES_NULLABLE,
          OperandTypes.MAP);

+  private static RelDataType deriveTypeMapFromArrays(SqlOperatorBinding opBinding) {
+    final RelDataType keysArrayType = opBinding.collectOperandTypes().get(0);


can you do opBinding.getOperandType? if so, that's a little cleaner I think

fixed @tanclary

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

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

liuyongvs · 2023-06-07T15:24:54Z

@tanclary @MasseGuillaume will you help look again?

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

MasseGuillaume · 2023-06-07T19:00:18Z

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

+    f.checkScalar("map_from_arrays(array[1, 1, null], array['foo', 'bar', 'name'])",
+        "{1=bar, null=name}", "(INTEGER, CHAR(4) NOT NULL) MAP NOT NULL");


spark.sql("""select map_from_arrays(array(1, 1, null), array('foo', 'bar', 'name'))""").show java.lang.RuntimeException: Duplicate map key 1 was found, please check the input data. If you want to remove the duplicated keys, you can set spark.sql.mapKeyDedupPolicy to LAST_WIN so that the key inserted at last takes precedence.

this is because spark's map is different from calcite is map implementation.
spark suppprts two way, while calcite is LAST_WIN. if we 100% match spark, it should break the behavior of all map functions

object MapKeyDedupPolicy extends Enumeration { val EXCEPTION, LAST_WIN = Value }

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

liuyongvs · 2023-06-08T05:41:00Z

because both of you @tanclary @MasseGuillaume says add array with different element type, it will throw exception.
i add

   f.checkFails("map_from_arrays(^array[1, '1', true]^, array['a', 'b', 'c'])",
        "Parameters must be of the same type",
        false);
    f.checkFails("map_from_arrays(array['a', 'b', 'c'], ^array[1, '1', true]^)",
        "Parameters must be of the same type",
        false);

and from my side, it should be test in array_value_constructor function instead of this. i also add test in array_value_constructor

    f.checkFails("^array[1, '1', true]^", "Parameters must be of the same type", false);

liuyongvs · 2023-06-08T05:41:35Z

hi @tanclary @MasseGuillaume fix all your reviews, will you have a look again?

site/_docs/reference.md

liuyongvs · 2023-06-21T15:51:11Z

hi @tanclary the pr has approved, i squash the commits. will you help review it again or merge it?

tanclary

LGTM aside from one nit

liuyongvs · 2023-06-23T05:31:09Z

hi @tanclary thanks for your review, fix the nit.

NobiGo

LGTM. Only need to add some tests.

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

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

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

…park library)

sonarcloud · 2023-06-25T08:52:53Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
12 Code Smells

95.6% Coverage
0.0% Duplication

NobiGo

LGTM

liuyongvs force-pushed the str_to_map2 branch from c090fae to dd22482 Compare June 2, 2023 14:39

asfgit force-pushed the main branch from 8ea4160 to 6ffcb86 Compare June 3, 2023 05:30

liuyongvs force-pushed the str_to_map2 branch 2 times, most recently from 3a1f8b8 to 0b37684 Compare June 3, 2023 14:07

liuyongvs commented Jun 3, 2023

View reviewed changes

liuyongvs force-pushed the str_to_map2 branch from 0b37684 to ecc982f Compare June 4, 2023 16:47

tanclary reviewed Jun 6, 2023

View reviewed changes

liuyongvs force-pushed the str_to_map2 branch from ecc982f to 3ceda5a Compare June 6, 2023 04:01

liuyongvs requested a review from tanclary June 6, 2023 04:07

liuyongvs force-pushed the str_to_map2 branch 2 times, most recently from 61de8ef to 067ac20 Compare June 6, 2023 05:12

liuyongvs changed the title ~~[CALCITE-5744] Add STR_TO_MAP function (enabled in Spark library)~~ [CALCITE-5744] Add MAP_FROM_ARRAYS, STR_TO_MAP function (enabled in Spark library) Jun 6, 2023

liuyongvs force-pushed the str_to_map2 branch from 067ac20 to a6b542d Compare June 6, 2023 06:02

tanclary reviewed Jun 6, 2023

View reviewed changes

liuyongvs requested a review from tanclary June 6, 2023 15:22

tanclary reviewed Jun 6, 2023

View reviewed changes

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

tanclary reviewed Jun 6, 2023

View reviewed changes

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java Show resolved Hide resolved

liuyongvs requested a review from tanclary June 7, 2023 00:01

liuyongvs force-pushed the str_to_map2 branch from fe4ba42 to 82ff639 Compare June 7, 2023 00:03

MasseGuillaume reviewed Jun 7, 2023

View reviewed changes

liuyongvs requested a review from MasseGuillaume June 8, 2023 09:18

tanclary reviewed Jun 8, 2023

View reviewed changes

site/_docs/reference.md Outdated Show resolved Hide resolved

julianhyde force-pushed the main branch from 0ea24a0 to 8a5cf83 Compare June 8, 2023 21:20

julianhyde force-pushed the main branch from 8a5cf83 to cf7f71b Compare June 8, 2023 21:21

liuyongvs requested a review from tanclary June 8, 2023 22:58

MasseGuillaume approved these changes Jun 21, 2023

View reviewed changes

MasseGuillaume mentioned this pull request Jun 21, 2023

[CALCITE-5772] Add MAP_CONCAT, MAP_FROM_ENTRIES function (enabled in … #3262

Merged

liuyongvs force-pushed the str_to_map2 branch from 3cfb992 to 368f6ea Compare June 21, 2023 15:49

tanclary approved these changes Jun 22, 2023

View reviewed changes

liuyongvs force-pushed the str_to_map2 branch from 368f6ea to 2907dcd Compare June 23, 2023 05:29

NobiGo reviewed Jun 25, 2023

View reviewed changes

core/src/main/java/org/apache/calcite/runtime/CalciteResource.java Show resolved Hide resolved

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java Show resolved Hide resolved

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java Show resolved Hide resolved

[CALCITE-5744] Add MAP_FROM_ARRAYS, STR_TO_MAP function (enabled in S…

5348d8f

…park library)

liuyongvs force-pushed the str_to_map2 branch from 2907dcd to 5348d8f Compare June 25, 2023 08:32

liuyongvs requested a review from NobiGo June 25, 2023 08:32

NobiGo approved these changes Jun 25, 2023

View reviewed changes

NobiGo merged commit 95089c8 into apache:main Jun 26, 2023
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-5744] Add MAP_FROM_ARRAYS, STR_TO_MAP function (enabled in Spark library) #3238

[CALCITE-5744] Add MAP_FROM_ARRAYS, STR_TO_MAP function (enabled in Spark library) #3238

liuyongvs commented Jun 2, 2023 •

edited

liuyongvs Jun 3, 2023

liuyongvs Jun 3, 2023

liuyongvs Jun 3, 2023

liuyongvs commented Jun 4, 2023

tanclary left a comment

tanclary Jun 6, 2023

liuyongvs Jun 6, 2023

tanclary Jun 6, 2023

liuyongvs Jun 6, 2023

liuyongvs commented Jun 6, 2023 •

edited

tanclary Jun 6, 2023

liuyongvs Jun 6, 2023

liuyongvs commented Jun 7, 2023

MasseGuillaume Jun 7, 2023

liuyongvs Jun 8, 2023

liuyongvs commented Jun 8, 2023

liuyongvs commented Jun 8, 2023

liuyongvs commented Jun 21, 2023

tanclary left a comment

liuyongvs commented Jun 23, 2023

NobiGo left a comment

sonarcloud bot commented Jun 25, 2023

NobiGo left a comment

		f.checkScalar("map_from_arrays(array[1, 1, null], array['foo', 'bar', 'name'])",
		"{1=bar, null=name}", "(INTEGER, CHAR(4) NOT NULL) MAP NOT NULL");

[CALCITE-5744] Add MAP_FROM_ARRAYS, STR_TO_MAP function (enabled in Spark library) #3238

[CALCITE-5744] Add MAP_FROM_ARRAYS, STR_TO_MAP function (enabled in Spark library) #3238

Conversation

liuyongvs commented Jun 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuyongvs commented Jun 4, 2023

tanclary left a 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 Jun 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuyongvs commented Jun 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuyongvs commented Jun 8, 2023

liuyongvs commented Jun 8, 2023

liuyongvs commented Jun 21, 2023

tanclary left a comment

Choose a reason for hiding this comment

liuyongvs commented Jun 23, 2023

NobiGo left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 25, 2023

NobiGo left a comment

Choose a reason for hiding this comment

liuyongvs commented Jun 2, 2023 •

edited

liuyongvs commented Jun 6, 2023 •

edited