Skip to content

refactor: Rationalize SQL dialect and statement code, to simplify adding new dialects - #2235

Open
ferenc-csaky wants to merge 2 commits into
mainfrom
refactor/sql-dialects
Open

refactor: Rationalize SQL dialect and statement code, to simplify adding new dialects#2235
ferenc-csaky wants to merge 2 commits into
mainfrom
refactor/sql-dialects

Conversation

@ferenc-csaky

@ferenc-csaky ferenc-csaky commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Key Changes

  • Centralized dialect SQL translation discovery and dispatch.
  • Made translation maps immutable and locale-safe.
  • Consolidated relational-plan and SQL-string conversion into SqlConverters.
  • Replaced duplicated dialect converter pairs with shared conversion logic.
  • Added service-based lookup for dialect SQL converters.
  • Updated JDBC statement factories to use the consolidated converter.

Snapshot Diffs

  1. I turned back on escaping in Snowflake
  2. Some postgres ocnverters were instantiated multiple times before, that causes the generated index diff

@ferenc-csaky
ferenc-csaky force-pushed the refactor/sql-dialects branch from dfff128 to e589ac2 Compare July 27, 2026 13:28
@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 31.08108% with 51 lines in your changes missing coverage. Please review.
✅ Project coverage is 17.10%. Comparing base (98a02a8) to head (e589ac2).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...sqrl/calcite/dialect/SqlTranslationDispatcher.java 0.00% 17 Missing ⚠️
...atasqrl/calcite/convert/AbstractSqlConverters.java 7.14% 13 Missing ⚠️
...abase/relational/AbstractJdbcStatementFactory.java 0.00% 8 Missing ⚠️
...tasqrl/calcite/convert/SnowflakeSqlConverters.java 50.00% 2 Missing ⚠️
...tasqrl/calcite/dialect/BasePostgresSqlDialect.java 0.00% 2 Missing ⚠️
...database/relational/SnowflakeStatementFactory.java 0.00% 2 Missing ⚠️
.../java/com/datasqrl/calcite/SqrlConfigurations.java 0.00% 1 Missing ⚠️
...m/datasqrl/calcite/convert/FlinkSqlConverters.java 50.00% 1 Missing ⚠️
...com/datasqrl/calcite/dialect/DuckDbSqlDialect.java 83.33% 1 Missing ⚠️
...rl/calcite/dialect/ExtendedPostgresSqlDialect.java 87.50% 1 Missing ⚠️
... and 3 more
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2235      +/-   ##
============================================
+ Coverage     16.91%   17.10%   +0.18%     
- Complexity     1040     1045       +5     
============================================
  Files           613      611       -2     
  Lines         17738    17693      -45     
  Branches       2155     2155              
============================================
+ Hits           3001     3026      +25     
+ Misses        14425    14355      -70     
  Partials        312      312              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@mbroecheler mbroecheler left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I did not validate the snapshot changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants