BSE-4460: Calcite 1.38 #148

IsaacWarren · 2025-01-15T17:05:18Z

Changes included in this PR

Upgrade Calcite to 1.38

New rule CoreRules.PROJECT_OVER_SUM_TO_SUM0_RULE

Questions/request for close review

RexLiteral around line 1145
RexSimplify around line 2331 do we still need this Bodo change?
SqlValidatorImpl does the new getFieldAliases function do what our addition at line 600 was doing?

Testing strategy

CI

User facing changes

Checklist

Pipelines passed before requesting review. To run CI you must include [run CI] in your commit message.
I am familiar with the Contributing Guide
I have installed + ran pre-commit hooks.

Comments

RexShuttle, RexBuilder, RexWindow, SqlExtractFunction, SqlStdOperatorTable need re-indented

...bodosql-calcite-application/src/main/java/org/apache/calcite/plan/RelOptMaterialization.java

njriasan · 2025-01-15T17:39:42Z

...odosql-calcite-application/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java

@@ -990,12 +991,25 @@ void propagateCostImprovements(RelNode rel) {
                if (!relNode.getTraitSet().satisfies(subset.getTraitSet())) {
                    continue;
                }
-                if (!cost.isLt(subset.bestCost)) {
+
+                // Update subset best and best's cost when we find a cheaper rel


It's possible this fixes an issue I found with cycles in the planner. However, I'm too afraid to try remove this, especially given our limited access to the customer tests, so can you open a backlog issue to investigate it?

njriasan · 2025-01-15T17:43:49Z

...e_sql/bodosql-calcite-application/src/main/java/org/apache/calcite/rel/core/TableModify.java

Can you add the Bodo Change based on the diff? Its just for merge into but I was confused why we have this process.

...QL/calcite_sql/bodosql-calcite-application/src/main/java/org/apache/calcite/sql/SqlCall.java

...l/bodosql-calcite-application/src/main/java/org/apache/calcite/sql/SqlIntervalQualifier.java

njriasan · 2025-01-15T17:54:11Z

...odosql-calcite-application/src/main/java/org/apache/calcite/sql/fun/SqlStdOperatorTable.java

+  /**
+   * <code>BITOR</code> scalar function.
+   */
+  public static final SqlFunction BITOR =


Let's remove the bit function from our numeric operator table. Let's also open a backlog github issue to support the binary case for these.

Could you elaborate on this? All of the bit functions or just BITAND, BITOR, BITXOR, and BITNOT or the bitshift operations too? What do you mean by the binary case?

...l/bodosql-calcite-application/src/main/java/org/apache/calcite/sql/type/BodoSqlTypeUtil.java

njriasan · 2025-01-15T18:00:07Z

...calcite-application/src/main/java/org/apache/calcite/sql/validate/implicit/TypeCoercion.java

Let's see if we can remove coalesceCoercion.

njriasan · 2025-01-15T18:03:02Z

.../bodosql-calcite-application/src/main/java/org/apache/calcite/sql/validate/SqlValidator.java

+     * <p>Possible behavior differences include ignoring the {@code ORDER BY}
+     * clause of an embedded query, or converting measure expressions of a
+     * non-embedded query into values. */
+    @Value.Default default boolean embeddedQuery() {


Let's file an issue to explore this. We might have a simpler/better view expansion process now.

njriasan · 2025-01-15T18:04:05Z

...te_sql/bodosql-calcite-application/src/main/java/org/apache/calcite/sql/util/SqlShuttle.java

Let's add the bodo change here for fixing merge into.

njriasan · 2025-01-15T20:06:01Z

...bodosql-calcite-application/src/main/java/org/apache/calcite/sql/fun/SqlExtractFunction.java

It would be great if you could fix the formatting on this file because there are actually only a small number of changes.

njriasan · 2025-01-15T20:13:39Z

...l/bodosql-calcite-application/src/main/java/org/apache/calcite/sql/SqlIntervalQualifier.java

@@ -461,6 +462,53 @@ public int getIntervalSign(String value) {
    return sign;
  }

+  public static TimeUnit stringToDatePartTimeUnit(String stringValue) {


How is this used? We may need to intercept the usage to ensure we have nanosecond handling.

njriasan · 2025-01-15T20:14:28Z

...l/bodosql-calcite-application/src/main/java/org/apache/calcite/sql/SqlIntervalQualifier.java

@@ -720,7 +767,8 @@ private int[] evaluateIntervalLiteralAsQuarter(
      BigDecimal month = quarter.multiply(MONTHS_IN_QUARTER);


Delete the Bodo Change. Its dead code now.

njriasan · 2025-01-15T20:20:52Z

...alcite_sql/bodosql-calcite-application/src/main/java/org/apache/calcite/rex/RexSimplify.java

@@ -405,7 +408,7 @@ private RexNode simplifyGenericNode(RexCall e) {
            return e;
        }
        // Bodo Change: Avoid passing the output type to allow deriving it.
-        return rexBuilder.makeCall(e.getOperator(), operands);
+        return rexBuilder.makeCall(e.getParserPosition(), e.getType(), e.getOperator(), operands);


This violates the Bodo change. I would revert passing e.getType()

njriasan · 2025-01-15T20:22:41Z

...alcite_sql/bodosql-calcite-application/src/main/java/org/apache/calcite/rex/RexSimplify.java

-                            final RexLiteral literal = comparison.literal;
-                            final RexLiteral prevLiteral =
-                                    equalityConstantTerms.put(comparison.ref, literal);
-                            // Bodo Change: literal.equals(prevLiteral) is not reliable because


If this merged into Calcite this seems good to me.

njriasan · 2025-01-15T20:23:21Z

...alcite_sql/bodosql-calcite-application/src/main/java/org/apache/calcite/rex/RexSimplify.java

-                // which is not enough to exclude the literal. To be conservative we keep
-                // both checks and only convert to False if both fail.
-                if (!literal1.equals(literal2) && !literal1.getValue().equals(literal2.getValue())) {
+                if (literal1.getType().equals(literal2.getType()) && !literal1.equals(literal2)) {


njriasan · 2025-01-15T20:30:53Z

Regarding Questions:

I would keep the RexSimplify change. It looks good as shown.
I think the RexLiteral changes are fine but we may need to check how it impacts our tests.
For SqlValidatorImpl I'm not sure/I don't think so. We can hop on a call to ensure I know exactly which line matches.

codecov · 2025-01-16T01:45:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@b585341). Learn more about missing BASE report.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #148   +/-   ##
=======================================
  Coverage        ?   77.78%           
=======================================
  Files           ?      169           
  Lines           ?    61952           
  Branches        ?     8671           
=======================================
  Hits            ?    48191           
  Misses          ?    11663           
  Partials        ?     2098

knassre-bodo · 2025-01-16T15:54:45Z

BodoSQL/calcite_sql/bodosql-calcite-application/src/main/codegen/includes/parserImpls.ftl

@@ -1098,6 +1098,33 @@ SqlDrop SqlDropSchema(Span s) :
 //     AddExpression2b(ExprContext.ACCEPT_SUB_QUERY, list)
 // }

+//SqlNode DatePartFunctionCall() :


If you haven't already, I advise seeing if this branch causes anything in the test repo to fail / get changed (e.g. planner adjustments), and if so to open a parallel PR to update those.

IsaacWarren added 5 commits January 13, 2025 17:59

All changes through rex

2950784

Update runtime and sql

a860b32

All but parser

9d78343

Update parser

b521500

[Run CI]

4aa7949

IsaacWarren requested review from njriasan and knassre-bodo January 15, 2025 17:05

IsaacWarren added 4 commits January 15, 2025 11:17

fix semicolons

1452399

[Run CI]

34ba286

Fix signatures

e2f2999

[Run CI]

2dde8f5

njriasan reviewed Jan 15, 2025

View reviewed changes

IsaacWarren added 6 commits January 15, 2025 13:18

Use updated function name [run ci]

d752720

add bodo change

e7e3524

fix formatting

95b533c

Add bodo change

efae251

[Run CI]

54df081

[Run CI]

ef239e0

njriasan approved these changes Jan 15, 2025

View reviewed changes

IsaacWarren added 7 commits January 15, 2025 14:44

Format SqlExtractFunction

22a7e94

Review comments

394af7b

Add nanosecond

20699c9

Merge branch 'main' into isaac/calcite_1.38

86506fb

Merge remote-tracking branch 'origin/main' into isaac/calcite_1.38

2f38d93

remove deleted files

a152fe8

[Run CI]

63ea21c

knassre-bodo approved these changes Jan 16, 2025

View reviewed changes

Revert calcite getFunctionKind() change

7a6e5cc

IsaacWarren added 17 commits January 16, 2025 10:31

Remove redundant POSITION definition

f4c6b9f

fix typo

eefe22f

Readd missed bodo change

72540df

Fix variant casting

7089d21

Bugfixes

fd48e28

[Run CI]

d2b1d8e

Fix exactNumeric type coercion

3532951

Don't change int - int comparison to decimal

28bfbbe

Merge remote-tracking branch 'origin/main' into isaac/calcite_1.38

627ad85

Support streaming ANY_VALUE if upper bound is current row

1964b07

[Run CI]

d12944c

Fix bounds for windows without orderby

a5fce37

[Run CI]

e62928d

Merge remote-tracking branch 'origin/main' into isaac/calcite_1.38

751ed5e

[Run CI]

10fbaff

Merge remote-tracking branch 'origin/main' into isaac/calcite_1.38

f1573a9

[Run CI]

963538b

IsaacWarren merged commit daf98a2 into main Jan 27, 2025
22 of 23 checks passed

IsaacWarren deleted the isaac/calcite_1.38 branch January 27, 2025 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BSE-4460: Calcite 1.38 #148

BSE-4460: Calcite 1.38 #148

IsaacWarren commented Jan 15, 2025

njriasan Jan 15, 2025

njriasan Jan 15, 2025

njriasan Jan 15, 2025

IsaacWarren Jan 15, 2025

njriasan Jan 15, 2025

njriasan Jan 15, 2025

njriasan Jan 15, 2025

njriasan Jan 15, 2025

njriasan Jan 15, 2025

njriasan Jan 15, 2025

njriasan Jan 15, 2025

IsaacWarren Jan 15, 2025

njriasan Jan 15, 2025

njriasan Jan 15, 2025

njriasan commented Jan 15, 2025

codecov bot commented Jan 16, 2025 •

edited

Loading

knassre-bodo Jan 16, 2025

		@@ -720,7 +767,8 @@ private int[] evaluateIntervalLiteralAsQuarter(
		BigDecimal month = quarter.multiply(MONTHS_IN_QUARTER);

BSE-4460: Calcite 1.38 #148

BSE-4460: Calcite 1.38 #148

Conversation

IsaacWarren commented Jan 15, 2025

Changes included in this PR

Questions/request for close review

Testing strategy

User facing changes

Checklist

Comments

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

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

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

njriasan commented Jan 15, 2025

codecov bot commented Jan 16, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

codecov bot commented Jan 16, 2025 •

edited

Loading