Conversation
Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
There was a problem hiding this comment.
Pull request overview
This pull request advances the Calcite version to pick up recent upstream commits that fix a type inference issue with the CONTAINS operator. The PR also removes a complex workaround for a Calcite bug related to TUMBLE and HOP window functions, reverting changes from PR #5576 now that the fix has been upstreamed.
Changes:
- Updates Calcite commit hash from
ea08b43db6bc83ea76678bd77873e110083c0edctoe7e3925cf1bcd8d71b664fe295705c1180f89c3ain calcite_version.env - Removes custom implementations of window table functions (
FelderaSqlWindowTableFunction,FelderaSqlTumbleTableFunction,FelderaSqlHopTableFunction) now that Calcite properly handles unlimited precision timestamps - Updates test for issue #5637 to verify that incompatible types in CONTAINS operator are properly rejected by Calcite
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| sql-to-dbsp-compiler/calcite_version.env | Updates Calcite commit hash to pick up upstream fixes |
| sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/simple/Regression2Tests.java | Updates issue5637 test to verify proper error message for incompatible types in CONTAINS operator |
| sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/calciteCompiler/SqlToRelCompiler.java | Removes ReplaceTumbleHop shuttle class that was workaround for Calcite bug |
| sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/calciteCompiler/FelderaSqlWindowTableFunction.java | Deletes custom window table function base class (no longer needed) |
| sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/calciteCompiler/FelderaSqlTumbleTableFunction.java | Deletes custom TUMBLE implementation (no longer needed) |
| sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/calciteCompiler/FelderaSqlHopTableFunction.java | Deletes custom HOP implementation (no longer needed) |
| sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/calciteCompiler/CustomFunctions.java | Removes registration of custom window functions |
| sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/calciteCompiler/ConvertletTable.java | Changes parameter type from FelderaSqlWindowTableFunction to SqlWindowTableFunction |
| sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/CalciteToDBSPCompiler.java | Simplifies function name matching using switch statement, removes custom function name checks |
| sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/DBSPCompiler.java | Removes error message rewriting for custom window functions |
...er/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/calciteCompiler/ConvertletTable.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
mythical-fred
left a comment
There was a problem hiding this comment.
Net -476 lines. This is the best kind of PR.
Upstreaming the timestamp precision fix to Calcite and reverting the workaround is exactly the right move — the custom FelderaSqlTumbleTableFunction/FelderaSqlHopTableFunction classes, the ReplaceTumbleHop AST shuttle, and the error-message rewriting for feldera_hop/feldera_tumble were all there to paper over a Calcite bug. Now that the bug is fixed upstream, none of it needs to exist.
The issue5637 test going from @Ignore to a proper statementsFailingInCompilation assertion is the right signal — the fix is now verified, not assumed.
Advance Calcite version to pick up recent commits.
Fixes #5637
I have upstreamed a small fix which enables us to undo a complicated workaround a Calcite bug for
TUMBLEandHOPfunctions. A second commit thus essentially reverts #5576Describe Manual Test Plan
Ran all Java tests