[CALCITE-5862] Incorrect semantics of ARRAY function (Spark library) when elements have Numeric and Character types #3324

chucheng92 · 2023-07-19T08:13:28Z

What is the purpose of the change

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

Brief change log

Fix incorrect spark semantics of array function when array elements have Numeric and Character types.

Verifying this change

SqlOperatorTests#testArrayFunc()

tanclary · 2023-07-19T14:53:34Z

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

+    List<RelDataType> nullTypes = new ArrayList<>();
+    for (int i = 0; i < opBinding.getOperandCount(); i++) {
+      SqlTypeFamily family = opBinding.getOperandType(i).getSqlTypeName().getFamily();
+      if (family == SqlTypeFamily.NUMERIC) {


can this be rewritten as a switch case?

tanclary

Left a couple of comments. Thanks for filing and opening a fix for this bug!

tanclary · 2023-07-19T15:00:44Z

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

+      }
+    }
+
+    boolean onlyNumericCharacterTypes = opBinding.getOperandCount() > 0


I could be misunderstanding, but if you only care about the size of the lists and not the elements themselves, you could just use boolean flags to keep track of which types have appeared. I think it will simplify the expression below.

final List<RelDataType> types = opBinding.collectOperandTypes(); boolean hasNumeric = false; boolean hasCharacter = false types.forEach(type -> { // null check here maybe switch (type.getSqlTypeName().getFamily()) { case x: hasNumeric = true; break; case y: hasCharacter = true; break; default: } if hasNumeric || hasCharacter || c {....} ``` you could do something like this maybe? (Excuse the pseudocode it's just an example)

Yes. You are right. I have fixed this. PTAL~

chucheng92 · 2023-07-20T02:15:53Z

@tanclary thanks for reviewing. I will address these comments later.

JiajunBernoulli

I think array must have same type.

You should use TypeCoercionImpl to solve your problem.

chucheng92 · 2023-07-23T07:50:03Z

I think array must have same type.

You should use TypeCoercionImpl to solve your problem.

Hi, Jiajun. thanks for reviewing.

However, this PR does not modify the SameOperandTypeChecker constraint of array. In fact, the current SameOperandTypeChecker allows character and numeric to exist at the same time. It's just that inferReturnType does not handle the simultaneous existence of character and numeric. please see:

  public static final SqlFunction ARRAY =
      SqlBasicFunction.create("ARRAY",
          SqlLibraryOperators::arrayReturnType,
          OperandTypes.SAME_VARIADIC);

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

Line 92 in bdfb170

if (!SqlTypeUtil.isComparable(types[i], types[prev])) {

The SameOperandTypeChecker will handle the special case of character, which itself allows character and numeric to coexist.

so currently run select array(1, 2, 'Hi'); can pass the OperandTypeChecker but with NPE in return type checker. This pr is used to fix the inconsistent behavior of operandTypeChecker and returnTypeInference. WDYT?

chucheng92 · 2023-07-23T08:12:32Z

btw, this PR just modify array function with Spark semantics, and the arrayReturnType is a private method just for spark semantics. This PR modify the spark arrayReturnType to solve this bug, I think it's a reasonable solution. @tanclary @JiajunBernoulli WDYT? Looking forward to your opinions.

chucheng92 · 2023-08-14T12:07:45Z

@JiajunBernoulli @tanclary Hi, Sorry to ping you. It has been a long time for this PR.

Do you have any other concerns? If you have any comments, I hope to receive your feedback. If there are some problems in my implementation, maybe you can give some specific opinions. I will appreciate it.

tanclary · 2023-08-14T17:29:08Z

So to clarify: you can have an array with multiple types, but only if they are NUMERIC/CHARACTER? What if you have another combination like [1, Date '2008-12-25', 'a']?

chucheng92 · 2023-08-15T06:42:45Z

So to clarify: you can have an array with multiple types, but only if they are NUMERIC/CHARACTER? What if you have another combination like [1, Date '2008-12-25', 'a']?

@tanclary thanks for reviewing. I have updated the PR, if you help time, pls help to re-check it.

btw, the cases which have multi-types just like the case '[1, Date '2008-12-25', 'a']' can't enter in arrayReturnType method above, because it will fail in SameOperandTypeChecker. but if we call separately, we do need other types check.

chucheng92 · 2023-09-07T17:35:49Z

hi, @tanclary It’s been a long time since last review, could we continue on this PR (if you have time)? thanks.

tanclary · 2023-09-07T17:47:12Z

@chucheng92 Yes I think this looks good but I will wait for @JiajunBernoulli to approve/give + 1 because they also had some comments if I remember correctly.

JiajunBernoulli · 2023-09-08T02:07:45Z

@chucheng92 , Compared to NPE, the is indeed an improvement.

LGTM, remember to squash your commits before merging.

chucheng92 · 2023-09-08T10:32:19Z

@tanclary @JiajunBernoulli thanks for your patient review. all comments resolved. @tanclary If you have time, could you help to merge this?

tanclary · 2023-09-11T16:02:53Z

@chucheng92 Hi yes, would you mind editing the commit message to add a space between "function" and "(Spark library)"? Apologies for the nit. Re-ping me after and I will help merge 👍

…when elements have Numeric and Character types

sonarcloud · 2023-09-12T03:10:50Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

93.0% Coverage
0.0% Duplication

chucheng92 · 2023-09-12T03:33:06Z

@chucheng92 Hi yes, would you mind editing the commit message to add a space between "function" and "(Spark library)"? Apologies for the nit. Re-ping me after and I will help merge 👍

Thanks, fixed it.

MasseGuillaume · 2023-11-17T17:49:34Z

This PR introduce a regression when building an array with structs.

I defined in my codebase named_struct that can return a struct type:

public class NamedStructUtils {

  private NamedStructUtils() {
    throw new UnsupportedOperationException("This is a utility class and cannot be instantiated");
  }

  public static SqlBasicFunction create() {
    return SqlBasicFunction.create(
      "NAMED_STRUCT",
      NamedStructUtils::returnType,
      OPERAND_TYPE_CHECKER);
  }

  private static String namedStructRequiresLiteralStringKeysGotExpression() {
    return "named_struct key is not a string literal, got an expression";
  }

  private static String namedStructRequiresLiteralStringKeysGotOtherType(String tpe) {
    return String.format("named_struct key is not a string literal, found type '%s'", tpe);
  }

  private static String namedStructRequiresEvenNumberOfArgs() {
    return "named_struct requires an even number for arguments";
  }
  
  private static RelDataType returnType(SqlOperatorBinding operatorBinding) {
    final List<String> keys = new ArrayList<String>();
    final List<RelDataType> values = new ArrayList<RelDataType>();
    int i = 0;
    for (RelDataType type : operatorBinding.collectOperandTypes()) {
      if (i % 2 == 0) {
        Object key = operatorBinding.getOperandLiteralValue(i, Object.class);
        if (key == null) {
          throw new CalciteException(
            namedStructRequiresLiteralStringKeysGotExpression(), null);
        }
        if (key instanceof NlsString) {
          keys.add(((NlsString) key).getValue());
        } else {
          String tpe = key.getClass().getSimpleName();
          throw new CalciteException(
            namedStructRequiresLiteralStringKeysGotOtherType(tpe), null);
        }

      } else {
        values.add(type);
      }
      i++;
    }
    final RelDataTypeFactory typeFactory = operatorBinding.getTypeFactory();
    return typeFactory.createStructType(values, keys);
  }

  private static final SqlOperandTypeChecker TWO_OR_MORE =
      OperandTypes.variadic(SqlOperandCountRanges.from(2));

  private static final SqlOperandTypeChecker OPERAND_TYPE_CHECKER =
      new SqlOperandTypeChecker() {
        @Override public boolean checkOperandTypes(
            SqlCallBinding callBinding,
            boolean throwOnFailure) {
          final boolean isRangeOk = TWO_OR_MORE.checkOperandTypes(callBinding, throwOnFailure);
          final int operandCount = callBinding.operands().size();
          final boolean isEvenNumberOfArgs = operandCount % 2 != 0;

          if (throwOnFailure && isEvenNumberOfArgs) {
            throw new CalciteException(namedStructRequiresEvenNumberOfArgs(), null);
          }

          return isRangeOk && isEvenNumberOfArgs;
        }

        @Override public SqlOperandCountRange getOperandCountRange() {
          return TWO_OR_MORE.getOperandCountRange();
        }

        @Override public String getAllowedSignatures(SqlOperator op, String opName) {
          return opName + "(...)";
        }
      };
}

Since SqlTypeName.ROW has type family null: ROW(PrecScale.NO_NO, false, Types.STRUCT, null),
it fails the check requireNonNull(family, "array element type family");

eg

array(named_struct('k', 'v'))

chucheng92 · 2023-11-20T04:57:59Z

@MasseGuillaume thanks for reporting this. I will try to reproduce/fix this.

chucheng92 · 2023-11-21T10:07:13Z

opened a ticket to track this: https://issues.apache.org/jira/browse/CALCITE-6127

chucheng92 changed the title ~~[CALCITE-5862] Fix incorrect semantics of array function enabled in Spark Library~~ [CALCITE-5862] Incorrect semantics of array function when elements have Numeric and Character types(enabled in Spark Library) Jul 19, 2023

chucheng92 force-pushed the array_fix branch from 40a3a8e to 6e01d03 Compare July 19, 2023 09:37

chucheng92 changed the title ~~[CALCITE-5862] Incorrect semantics of array function when elements have Numeric and Character types(enabled in Spark Library)~~ [CALCITE-5862] Incorrect spark semantics of array function when elements have Numeric and Character types Jul 19, 2023

chucheng92 force-pushed the array_fix branch from 6e01d03 to ef4f438 Compare July 19, 2023 09:41

tanclary reviewed Jul 19, 2023

View reviewed changes

chucheng92 force-pushed the array_fix branch 2 times, most recently from 65f4e2f to b93fd1c Compare July 20, 2023 08:24

JiajunBernoulli requested changes Jul 23, 2023

View reviewed changes

chucheng92 requested a review from tanclary July 27, 2023 12:50

chucheng92 changed the title ~~[CALCITE-5862] Incorrect spark semantics of array function when elements have Numeric and Character types~~ [CALCITE-5862] Incorrect semantics of array function when elements have Numeric and Character types (enabled in Spark Library) Aug 1, 2023

chucheng92 force-pushed the array_fix branch 2 times, most recently from 9846350 to 009e2d6 Compare August 2, 2023 02:22

chucheng92 changed the title ~~[CALCITE-5862] Incorrect semantics of array function when elements have Numeric and Character types (enabled in Spark Library)~~ [CALCITE-5862] Incorrect semantics of ARRAY function(Spark library) when elements have Numeric and Character types Aug 2, 2023

chucheng92 force-pushed the array_fix branch 2 times, most recently from fc60f5f to f3d434e Compare August 16, 2023 02:40

tanclary force-pushed the main branch from 4804912 to 00db001 Compare September 6, 2023 00:42

chucheng92 force-pushed the array_fix branch from f3d434e to ef178dd Compare September 7, 2023 17:35

JiajunBernoulli approved these changes Sep 8, 2023

View reviewed changes

chucheng92 force-pushed the array_fix branch 2 times, most recently from f28e385 to 5f632c0 Compare September 8, 2023 09:54

tanclary changed the title ~~[CALCITE-5862] Incorrect semantics of ARRAY function(Spark library) when elements have Numeric and Character types~~ [CALCITE-5862] Incorrect semantics of ARRAY function (Spark library) when elements have Numeric and Character types Sep 11, 2023

chucheng92 force-pushed the array_fix branch from 5f632c0 to ae17bd2 Compare September 12, 2023 02:18

[CALCITE-5862] Incorrect semantics of ARRAY function (Spark library) …

60797bf

…when elements have Numeric and Character types

chucheng92 force-pushed the array_fix branch from ae17bd2 to 60797bf Compare September 12, 2023 02:58

tanclary merged commit 2cb8167 into apache:main Sep 12, 2023
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-5862] Incorrect semantics of ARRAY function (Spark library) when elements have Numeric and Character types #3324

[CALCITE-5862] Incorrect semantics of ARRAY function (Spark library) when elements have Numeric and Character types #3324

chucheng92 commented Jul 19, 2023 •

edited

Loading

tanclary Jul 19, 2023

tanclary left a comment

tanclary Jul 19, 2023

tanclary Jul 19, 2023 •

edited

Loading

chucheng92 Jul 20, 2023 •

edited

Loading

chucheng92 commented Jul 20, 2023

JiajunBernoulli left a comment

chucheng92 commented Jul 23, 2023 •

edited

Loading

chucheng92 commented Jul 23, 2023 •

edited

Loading

chucheng92 commented Aug 14, 2023 •

edited

Loading

tanclary commented Aug 14, 2023

chucheng92 commented Aug 15, 2023 •

edited

Loading

chucheng92 commented Sep 7, 2023 •

edited

Loading

tanclary commented Sep 7, 2023

JiajunBernoulli commented Sep 8, 2023

chucheng92 commented Sep 8, 2023

tanclary commented Sep 11, 2023

sonarcloud bot commented Sep 12, 2023

chucheng92 commented Sep 12, 2023

MasseGuillaume commented Nov 17, 2023

chucheng92 commented Nov 20, 2023

chucheng92 commented Nov 21, 2023

[CALCITE-5862] Incorrect semantics of ARRAY function (Spark library) when elements have Numeric and Character types #3324

[CALCITE-5862] Incorrect semantics of ARRAY function (Spark library) when elements have Numeric and Character types #3324

Conversation

chucheng92 commented Jul 19, 2023 • edited Loading

What is the purpose of the change

Brief change log

Verifying this change

tanclary Jul 19, 2023

Choose a reason for hiding this comment

tanclary left a comment

Choose a reason for hiding this comment

tanclary Jul 19, 2023

Choose a reason for hiding this comment

tanclary Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

chucheng92 Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

chucheng92 commented Jul 20, 2023

JiajunBernoulli left a comment

Choose a reason for hiding this comment

chucheng92 commented Jul 23, 2023 • edited Loading

chucheng92 commented Jul 23, 2023 • edited Loading

chucheng92 commented Aug 14, 2023 • edited Loading

tanclary commented Aug 14, 2023

chucheng92 commented Aug 15, 2023 • edited Loading

chucheng92 commented Sep 7, 2023 • edited Loading

tanclary commented Sep 7, 2023

JiajunBernoulli commented Sep 8, 2023

chucheng92 commented Sep 8, 2023

tanclary commented Sep 11, 2023

sonarcloud bot commented Sep 12, 2023

chucheng92 commented Sep 12, 2023

MasseGuillaume commented Nov 17, 2023

chucheng92 commented Nov 20, 2023

chucheng92 commented Nov 21, 2023

chucheng92 commented Jul 19, 2023 •

edited

Loading

tanclary Jul 19, 2023 •

edited

Loading

chucheng92 Jul 20, 2023 •

edited

Loading

chucheng92 commented Jul 23, 2023 •

edited

Loading

chucheng92 commented Jul 23, 2023 •

edited

Loading

chucheng92 commented Aug 14, 2023 •

edited

Loading

chucheng92 commented Aug 15, 2023 •

edited

Loading

chucheng92 commented Sep 7, 2023 •

edited

Loading