From 4c9011c388f826c36463ae7da875ddb525104376 Mon Sep 17 00:00:00 2001 From: Mihai Budiu Date: Wed, 1 Nov 2023 19:34:53 -0700 Subject: [PATCH] [CALCITE-5884] ARRAY_TO_STRING function should return NULL if its 'nullValue' argument is NULL Signed-off-by: Mihai Budiu --- .../apache/calcite/runtime/SqlFunctions.java | 4 ++ .../apache/calcite/sql/SqlNumericLiteral.java | 2 +- .../apache/calcite/sql/type/ReturnTypes.java | 2 +- .../apache/calcite/test/RelOptRulesTest.java | 57 ++++++++++++++----- .../apache/calcite/test/RelOptRulesTest.xml | 17 ++++++ site/_docs/reference.md | 2 +- .../apache/calcite/test/SqlOperatorTest.java | 1 + 7 files changed, 68 insertions(+), 17 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java b/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java index 5f3cbf95631..4e11245ae3e 100644 --- a/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java +++ b/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java @@ -5583,6 +5583,10 @@ public static String arrayToString(List list, String delimiter) { /** SQL {@code ARRAY_TO_STRING(array, delimiter, nullText)} function. */ public static String arrayToString(List list, String delimiter, @Nullable String nullText) { + // Note that the SQL function ARRAY_TO_STRING that we implement will return + // 'NULL' when the nullText argument is NULL. However, that is handled by + // the nullPolicy of the RexToLixTranslator. So here a NULL value + // for the nullText argument can only come from the above 2-argument version. StringBuilder sb = new StringBuilder(); boolean isFirst = true; for (Object item : list) { diff --git a/core/src/main/java/org/apache/calcite/sql/SqlNumericLiteral.java b/core/src/main/java/org/apache/calcite/sql/SqlNumericLiteral.java index 25205b99ab2..ebf018ea5e8 100644 --- a/core/src/main/java/org/apache/calcite/sql/SqlNumericLiteral.java +++ b/core/src/main/java/org/apache/calcite/sql/SqlNumericLiteral.java @@ -117,7 +117,7 @@ public boolean isExact() { scaleValue); } - // else we have a a float, real or double. make them all double for + // else we have a FLOAT, REAL or DOUBLE. make them all DOUBLE for // now. return typeFactory.createSqlType(SqlTypeName.DOUBLE); } diff --git a/core/src/main/java/org/apache/calcite/sql/type/ReturnTypes.java b/core/src/main/java/org/apache/calcite/sql/type/ReturnTypes.java index 8fe9284ee99..48da8b88a61 100644 --- a/core/src/main/java/org/apache/calcite/sql/type/ReturnTypes.java +++ b/core/src/main/java/org/apache/calcite/sql/type/ReturnTypes.java @@ -591,7 +591,7 @@ public static SqlCall stripSeparator(SqlCall call) { }; /** - * Returns the element type of a ARRAY or MULTISET. + * Returns the element type of an ARRAY or MULTISET. * *

For example, given INTEGER ARRAY or MULTISET ARRAY, returns * INTEGER. diff --git a/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java b/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java index 9d84c823107..9b5f3a75b85 100644 --- a/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java +++ b/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java @@ -225,7 +225,8 @@ private static boolean skipItem(RexNode expr) { } /** - * Test case for [CALCITE-5813] + * Test case for + * [CALCITE-5813] * Type inference for sql functions REPEAT, SPACE, XML_TRANSFORM, * and XML_EXTRACT is incorrect. */ @Test void testRepeat() { @@ -246,7 +247,8 @@ private static boolean skipItem(RexNode expr) { } /** - * Test case for [CALCITE-5813] + * Test case for + * [CALCITE-5813] * Type inference for sql functions REPEAT, SPACE, XML_TRANSFORM, * and XML_EXTRACT is incorrect. */ @Test void testReplace() { @@ -263,7 +265,8 @@ private static boolean skipItem(RexNode expr) { } /** - * Test case for [CALCITE-5989] + * Test case for + * [CALCITE-5989] * Type inference for RPAD and LPAD functions (BIGQUERY) is incorrect. */ @Test void testRpad() { HepProgramBuilder builder = new HepProgramBuilder(); @@ -283,7 +286,8 @@ private static boolean skipItem(RexNode expr) { } /** - * Test case for [CALCITE-5989] + * Test case for + * [CALCITE-5989] * Type inference for RPAD and LPAD functions (BIGQUERY) is incorrect. */ @Test void testLpad() { HepProgramBuilder builder = new HepProgramBuilder(); @@ -303,7 +307,8 @@ private static boolean skipItem(RexNode expr) { } /** - * Test case for [CALCITE-5971] + * Test case for + * [CALCITE-5971] * Add the RelRule to rewrite the bernoulli sample as Filter. */ @Test void testSampleToFilter() { final String sql = "select deptno from emp tablesample bernoulli(50)"; @@ -313,7 +318,8 @@ private static boolean skipItem(RexNode expr) { } /** - * Test case for [CALCITE-5971] + * Test case for + * [CALCITE-5971] * Add the RelRule to rewrite the bernoulli sample as Filter. */ @Test void testSampleToFilterWithSeed() { final String sql = "select deptno from emp tablesample bernoulli(50) REPEATABLE(10)"; @@ -323,7 +329,8 @@ private static boolean skipItem(RexNode expr) { } /** - * Test case for [CALCITE-5813] + * Test case for + * [CALCITE-5813] * Type inference for sql functions REPEAT, SPACE, XML_TRANSFORM, * and XML_EXTRACT is incorrect. */ @Test void testSpace() { @@ -2034,6 +2041,21 @@ private void checkSemiOrAntiJoinProjectTranspose(JoinRelType type) { .check(); } + /** Test case for + * [CALCITE-5884] + * ARRAY_TO_STRING function should return NULL if its 'nullValue' argument is NULL. */ + @Test void testArrayToString() { + final String sql = "select array_to_string(array['1','2','3','4',NULL,'6'], ',', NULL)"; + // We expect the result to be NULL, since array_to_string returns NULL if + // any argument is NULL. + sql(sql).withFactory( + t -> t.withOperatorTable( + opTab -> SqlLibraryOperatorTableFactory.INSTANCE.getOperatorTable( + SqlLibrary.STANDARD, SqlLibrary.BIG_QUERY))) // for array_to_string function + .withRule(CoreRules.PROJECT_REDUCE_EXPRESSIONS) + .check(); + } + /** Test case for * [CALCITE-1558] * AggregateExpandDistinctAggregatesRule gets field mapping wrong if groupKey @@ -2733,7 +2755,8 @@ private void checkPushJoinThroughUnionOnRightDoesNotMatchSemiOrAntiJoin(JoinRelT } /** - * Test case for [CALCITE-5073] + * Test case for + * [CALCITE-5073] * JoinConditionPushRule cannot infer 'LHS.C1 = LHS.C2' from * 'LHS.C1 = RHS.C1 AND LHS.C2 = RHS.C1'. */ @@ -2750,7 +2773,8 @@ private void checkPushJoinThroughUnionOnRightDoesNotMatchSemiOrAntiJoin(JoinRelT } /** - * Test case for [CALCITE-5073] + * Test case for + * [CALCITE-5073] * JoinConditionPushRule cannot infer 'LHS.C1 = LHS.C2' from * 'LHS.C1 = RHS.C1 AND LHS.C2 = RHS.C1'. */ @@ -5483,7 +5507,8 @@ private HepProgram getTransitiveProgram() { } /** - * Test case for + * Test case for + * * [CALCITE-5861] ReduceExpressionsRule rules should constant-fold * expressions in window bounds. */ @@ -5682,7 +5707,8 @@ private HepProgram getTransitiveProgram() { .checkUnchanged(); } - /** Test case for + /** Test case for + * * AssertionError during constant reduction of SPLIT expression that returns NULL. */ @Test public void testSplitNull() { final String query = "select split('1|2|3', NULL)"; @@ -5695,7 +5721,8 @@ private HepProgram getTransitiveProgram() { .check(); } - /** Test case for + /** Test case for + * * AssertionError during constant reduction of SPLIT expression that returns NULL. */ @Test public void testSplitNull1() { final String query = "select split(NULL, '|')"; @@ -5708,7 +5735,8 @@ private HepProgram getTransitiveProgram() { .check(); } - /** Test case for + /** Test case for + * * AssertionError during constant reduction of SPLIT expression that returns NULL. */ @Test public void testSplitNull2() { final String query = "select split(NULL, NULL)"; @@ -5721,7 +5749,8 @@ private HepProgram getTransitiveProgram() { .check(); } - /** Test case for + /** Test case for + * * [CALCITE-5882] Compile-time evaluation of SPLIT function returns incorrect result. */ @Test public void testSplit() { final String query = "select split('1|2|3', '|')"; diff --git a/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml b/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml index b322976ac28..0bfe265d30a 100644 --- a/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml +++ b/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml @@ -1283,6 +1283,23 @@ LogicalProject(DEPTNO=[$0], EXPR$1=[OR(AND(IS NOT NULL($5), <>($2, 0)), AND(<($3 LogicalAggregate(group=[{0}], i=[LITERAL_AGG(true)]) LogicalProject(MGR=[$3]) LogicalTableScan(table=[[CATALOG, SALES, EMP]]) +]]> + + + + + + + + + + + diff --git a/site/_docs/reference.md b/site/_docs/reference.md index 6210c1d8ffe..1322abdc88a 100644 --- a/site/_docs/reference.md +++ b/site/_docs/reference.md @@ -2673,7 +2673,7 @@ BigQuery's type system uses confusingly different names for types and functions: | s | ARRAY_REPEAT(element, count) | Returns the array containing element count times. | b | ARRAY_REVERSE(array) | Reverses elements of *array* | s | ARRAY_SIZE(array) | Synonym for `CARDINALITY` -| b | ARRAY_TO_STRING(array, delimiter [, nullText ])| Returns a concatenation of the elements in *array* as a STRING and take *delimiter* as the delimiter. If the *nullText* parameter is used, the function replaces any `NULL` values in the array with the value of *nullText*. If the *nullText* parameter is not used, the function omits the `NULL` value and its preceding delimiter +| b | ARRAY_TO_STRING(array, delimiter [, nullText ])| Returns a concatenation of the elements in *array* as a STRING and take *delimiter* as the delimiter. If the *nullText* parameter is used, the function replaces any `NULL` values in the array with the value of *nullText*. If the *nullText* parameter is not used, the function omits the `NULL` value and its preceding delimiter. Returns `NULL` if any argument is `NULL` | s | ARRAY_UNION(array1, array2) | Returns an array of the elements in the union of *array1* and *array2*, without duplicates | s | ARRAYS_OVERLAP(array1, array2) | Returns true if *array1 contains at least a non-null element present also in *array2*. If the arrays have no common element and they are both non-empty and either of them contains a null element null is returned, false otherwise | s | ARRAYS_ZIP(array [, array ]*) | Returns a merged *array* of structs in which the N-th struct contains all N-th values of input arrays diff --git a/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java b/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java index 799b5ddbd0a..e7557476815 100644 --- a/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java +++ b/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java @@ -6600,6 +6600,7 @@ private static void checkIf(SqlOperatorFixture f) { f.checkScalar("array_to_string(array['', ''], '-')", "-", "VARCHAR NOT NULL"); f.checkNull("array_to_string(null, '-')"); f.checkNull("array_to_string(array['a', 'b', null], null)"); + f.checkNull("array_to_string(array['1','2','3'], ',', null)"); f.checkFails("^array_to_string(array[1, 2, 3], '-', ' ')^", "Cannot apply 'ARRAY_TO_STRING' to arguments of type 'ARRAY_TO_STRING" + "\\(, , \\)'\\. Supported form\\(s\\):"