[CALCITE-6690] Refactor the Arrow adapter type system #4052

caicancai · 2024-11-17T12:36:30Z

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

caicancai · 2024-11-17T12:39:14Z

After this pr, for some data types, we can easily identify their precision and scale such as decimal, time, timestamp

mihaibudiu · 2024-11-17T19:34:34Z

arrow/src/main/java/org/apache/calcite/adapter/arrow/ArrowFieldType.java

-  LONG(Primitive.LONG),
-  BYTE(Primitive.BYTE),
-  SHORT(Primitive.SHORT);
+class ArrowFieldType {


I think enums are preferable, since they are typechecked by the compiler.
A Class can be anything.

Later, I may dynamically pass some parameters according to the data type of arrow, which is difficult to achieve through enumeration.
e.g.

new ArrowFieldType(BigDecimal.class, ((ArrowType.Decimal) arrowType).getPrecision(), ((ArrowType.Decimal) arrowType).getScale())

Why later if you are refactoring the code now?
Do it correctly the first time.
I am suggesting to have 3 arguments for the enum constructor: Class, precision, scale.

@mihaibudiu Sorry for the late reply, you are right. I looked at the arrow types and it seems that Class, precision, and scale are sufficient.

I will finish this work on the weekend

@mihaibudiu Hi. I have a question, how to dynamically pass parameters in enumeration class? Precision and scale are not constants for decimal, timestamp and other types.

What is the CLI?

What is the CLI?

Sorry. it is CI

@mihaibudiu I'm using an enum class to represent this, but it doesn't seem ideal, CI always fails.
https://github.com/apache/calcite/actions/runs/12014265742/job/33489690898
https://github.com/apache/calcite/actions/runs/12071684172/job/33663981102?pr=4052

What do you recommend?

Have you tried to apply the recommendations made in the output of the failed CI run?

/home/runner/work/calcite/calcite/core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:2654: Note: [MethodCanBeStatic] A private method that does not reference the enclosing instance can be static private void onMatch2( ^ (see https://errorprone.info/bugpattern/MethodCanBeStatic) Did you mean 'private static void onMatch2('? /home/runner/work/calcite/calcite/core/src/main/java/org/apache/calcite/util/format/postgresql/format/compiled/NumberCompiledPattern.java:82: Note: [MethodCanBeStatic] A private method that does not reference the enclosing instance can be static private String getOrdinalSuffix(String stringValue) { ^ (see https://errorprone.info/bugpattern/MethodCanBeStatic) Did you mean 'private static String getOrdinalSuffix(String stringValue) {'? Note: Some input files use unchecked or unsafe operations. Note: Recompile with -Xlint:unchecked for details. > Task :arrow:compileJava /home/runner/work/calcite/calcite/arrow/src/main/java/org/apache/calcite/adapter/arrow/ArrowTranslator.java:233: Note: [MethodCanBeStatic] A private method that does not reference the enclosing instance can be static private String translateUnaryOp(String op, String name) { ^ (see https://errorprone.info/bugpattern/MethodCanBeStatic) Did you mean 'private static String translateUnaryOp(String op, String name) {'? /home/runner/work/calcite/calcite/arrow/src/main/java/org/apache/calcite/adapter/arrow/ArrowFieldType.java:47: warning: [ImmutableEnumChecker] enums should be immutable: 'ArrowFieldType' has non-final field 'precision' private int precision; ^ (see https://errorprone.info/bugpattern/ImmutableEnumChecker) Did you mean 'private final int precision;'? /home/runner/work/calcite/calcite/arrow/src/main/java/org/apache/calcite/adapter/arrow/ArrowFieldType.java:48: warning: [ImmutableEnumChecker] enums should be immutable: 'ArrowFieldType' has non-final field 'scale' private int scale; ^ (see https://errorprone.info/bugpattern/ImmutableEnumChecker) Did you mean 'private final int scale;'?

I optimized your CI suggestion above, but unfortunately it still didn't work, and the code looks more confusing. I guess it has something to do with the arrow-java build mode.
I'm not sure if we still have to stick to the enumeration class to implement this.

NobiGo · 2024-11-20T06:20:23Z

arrow/src/test/java/org/apache/calcite/adapter/arrow/ArrowData.java

-        switch (vector.getMinorType()) {
-        case TINYINT:
+        switch (field.getName()) {
+        case "tinyIntField":


This is a necessary change? What is the difference between decimalField2 and decimalField?

Required, the scale of decimalfield and decimalfiled2 are different, one is 2, the other is 3, the main purpose is to capture the accuracy of whether it is effective

caicancai · 2024-11-25T16:23:00Z

Sorry for confusing the commit message. I'm not sure if it was my changes that caused the cli to fail. I haven't reproduced the problem locally.

mihaibudiu · 2024-11-26T17:53:30Z

arrow/src/main/java/org/apache/calcite/adapter/arrow/ArrowFieldType.java

@@ -55,9 +45,20 @@ enum ArrowFieldType {
    this.clazz = clazz;
  }

+  ArrowFieldType(Class<?> clazz, int precision, int scale) {


I think the other constructors should call this one.
There are some values in the type system for "unspecified", maybe these could be reused.
Using 0 may not be always safe.

mihaibudiu · 2024-11-26T17:56:36Z

arrow/src/test/java/org/apache/calcite/adapter/arrow/ArrowAdapterTest.java

@@ -732,8 +732,8 @@ static void initializeArrowState(@TempDir Path sharedTempDir)
  @Test void testFilteredAgg() {
    String sql = "select SUM(SAL) FILTER (WHERE COMM > 400) as SALESSUM from EMP";
    String plan = "PLAN=EnumerableAggregate(group=[{}], SALESSUM=[SUM($0) FILTER $1])\n"
-        + "  EnumerableCalc(expr#0..7=[{inputs}], expr#8=[400:DECIMAL(19, 0)], expr#9=[>($t6, $t8)], "
-        + "expr#10=[IS TRUE($t9)], SAL=[$t5], $f1=[$t10])\n"
+        + "  EnumerableCalc(expr#0..7=[{inputs}], expr#8=[CAST($t6):DECIMAL(10, 2)], expr#9=[400.00:DECIMAL(10, 2)], "


can you also add or modify a test that uses a decimal number with digits after the decimal point? This would exercise the scale.

This is a table that comes with calcite and is not filled in the arrow adapter. If you modify it, it will affect many tests.

caicancai · 2024-12-01T15:52:35Z

I increasingly feel that the ArrowFieldType design is a bit complicated. In fact, it is unnecessary. We can directly convert Arrowtype to RelDataType. I don't understand why there is an extra layer of conversion.

I will refactor this class next weekend to make ArrowFieldType align directly with RelDataType.

cc @mihaibudiu

sonarqubecloud · 2024-12-01T15:52:52Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

mihaibudiu reviewed Nov 17, 2024

View reviewed changes

caicancai requested a review from mihaibudiu November 18, 2024 14:38

NobiGo reviewed Nov 20, 2024

View reviewed changes

caicancai force-pushed the 6690 branch from 15199b6 to 295f17d Compare November 25, 2024 15:20

caicancai requested a review from NobiGo November 25, 2024 15:23

caicancai force-pushed the 6690 branch 2 times, most recently from d467a5b to fd5f727 Compare November 25, 2024 16:06

mihaibudiu reviewed Nov 26, 2024

View reviewed changes

caicancai force-pushed the 6690 branch from 6ed11b4 to 565444d Compare November 28, 2024 15:05

caicancai added 3 commits November 28, 2024 23:17

[CALCITE-6690] Refactor the Arrow adapter type system

866ca2a

Improve code

39209a1

Test CI

9be6b8b

caicancai force-pushed the 6690 branch from 565444d to 9be6b8b Compare November 28, 2024 15:24

Test CI

1313779

caicancai closed this Dec 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-6690] Refactor the Arrow adapter type system #4052

[CALCITE-6690] Refactor the Arrow adapter type system #4052

caicancai commented Nov 17, 2024 •

edited

Loading

caicancai commented Nov 17, 2024 •

edited

Loading

mihaibudiu Nov 17, 2024

caicancai Nov 18, 2024 •

edited

Loading

mihaibudiu Nov 18, 2024

caicancai Nov 20, 2024 •

edited

Loading

caicancai Nov 22, 2024

mihaibudiu Nov 26, 2024

caicancai Nov 27, 2024

caicancai Nov 28, 2024 •

edited

Loading

mihaibudiu Nov 28, 2024

caicancai Dec 1, 2024

NobiGo Nov 20, 2024

caicancai Nov 20, 2024

caicancai commented Nov 25, 2024

mihaibudiu Nov 26, 2024

mihaibudiu Nov 26, 2024

caicancai Nov 28, 2024

caicancai commented Dec 1, 2024

sonarqubecloud bot commented Dec 1, 2024

[CALCITE-6690] Refactor the Arrow adapter type system #4052

[CALCITE-6690] Refactor the Arrow adapter type system #4052

Conversation

caicancai commented Nov 17, 2024 • edited Loading

caicancai commented Nov 17, 2024 • edited Loading

Choose a reason for hiding this comment

caicancai Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caicancai Nov 20, 2024 • 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

caicancai Nov 28, 2024 • 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

caicancai commented Nov 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caicancai commented Dec 1, 2024

sonarqubecloud bot commented Dec 1, 2024

Quality Gate passed

caicancai commented Nov 17, 2024 •

edited

Loading

caicancai commented Nov 17, 2024 •

edited

Loading

caicancai Nov 18, 2024 •

edited

Loading

caicancai Nov 20, 2024 •

edited

Loading

caicancai Nov 28, 2024 •

edited

Loading