calcite icon indicating copy to clipboard operation
calcite copied to clipboard

[CALCITE-6042] Add test cases for ARRAY-related functions by using spark array function

Open chucheng92 opened this issue 2 years ago • 8 comments

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

Add test cases by using spark array function rather than calcite array constructor. these cases only work in spark library.

chucheng92 avatar Oct 12 '23 07:10 chucheng92

I have wrote a array-expr-generator, it looks like:

the usage:

    // array_append(array(1), 2) & array_append(array[1], 2)
    f.checkScalarArray(ArrayType.ALL, "[1, 2]", "INTEGER NOT NULL ARRAY NOT NULL",
        "array_append(%A%0, %1)", "1", "2");

    // array_append(array(), 1)        
    f.checkScalarArray(ArrayType.FUNCTION, "[1]", "INTEGER NOT NULL ARRAY NOT NULL",
        "array_append(%A%0, %1)", "", "1");

    // array_append(array(array(1,2)), array(3,4)) & array_append(array[array[1,2]], array[3,4]) 
    // -> It's a nested array case.
    f.checkScalarArray(ArrayType.ALL, "[[1, 2], [3, 4]]",
        "INTEGER NOT NULL ARRAY NOT NULL ARRAY NOT NULL",
        "array_append(%A1%0, %A%1)", "1, 2", "3, 4");

the implemented method:

  default void checkScalarArray(
      ArrayType syntaxType,
      String result,
      String resultType,
      String exprPattern,
      String... exprArgs) {
    if (syntaxType == ArrayType.ALL) {
      String constructorExpr = generateArrayExpr(true, exprPattern, exprArgs);
      String functionExpr = generateArrayExpr(false, exprPattern, exprArgs);
      checkScalar(constructorExpr, result, resultType);
      checkScalar(functionExpr, result, resultType);
    } else {
      boolean useConstructor = syntaxType == ArrayType.CONSTRUCTOR;
      String arrayExpr = generateArrayExpr(useConstructor, exprPattern, exprArgs);
      checkScalar(arrayExpr, result, resultType);
    }
  }

generateArrayExpr generate array() or array[] with given syntaxType, and it supports nested array.

  • exprPattern %A[n]%i placeholders are used for array syntax, where n is the optional nested level and i is the index of the argument to be inserted. %i placeholders without %A will be replaced without array syntax.

  • exprArgs Arguments to replace the placeholders in the expression pattern. They will replace %0, %1, etc.

I feel it will be less readable, but it does remove duplicate code. I want to know what everyone thinks of this plan? If possible, I will update all array cases to this form.

@mihaibudiu what do you think?

chucheng92 avatar Oct 13 '23 10:10 chucheng92

There is actually an even better way to do it, I should have thought about it sooner. The idea is to use Calcite itself to rewrite the code. The way this is done is by implementing a SqlShuttle which replaces calls to one of the operators with calls to the other one. Then for each test you:

  • run the test as is
  • parse the Sql, run the shuttle, unparse the sql
  • run the resulting sql

Let me know if you need help implementing this solution.

mihaibudiu avatar Oct 13 '23 17:10 mihaibudiu

thanks for suggestions. @mihaibudiu

Frankly speaking, I'm not sure if this is the best solution, it may be over-optimized, and the duplication in the PR may just be sonar's strategy (since I handle all array functions)

Try to see if others have other opinions, I might be leaning towards good readability.

chucheng92 avatar Oct 15 '23 06:10 chucheng92

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

github-actions[bot] avatar Dec 30 '24 03:12 github-actions[bot]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

github-actions[bot] avatar Mar 20 '25 03:03 github-actions[bot]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

github-actions[bot] avatar Jun 28 '25 03:06 github-actions[bot]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

github-actions[bot] avatar Oct 25 '25 03:10 github-actions[bot]