feat(isthmus): configurable unquoted identifier casing via the ConverterProvider builder - #1037
Merged
Merged
Conversation
This was referenced Jul 24, 2026
nielspardon
added a commit
that referenced
this pull request
Jul 31, 2026
## Summary Add `ConverterProvider.builder()` and a nested `Builder` — a readable, forward-compatible way to configure a `ConverterProvider`, most importantly to supply a full Calcite `SqlParser.Config` now that the parsers consume the provider's config (#1035). `ConverterProvider.DEFAULT_SQL_PARSER_CONFIG` (`SqlParser.Config.DEFAULT` + `TO_UPPER` + `SqlDdlParserImpl.FACTORY` + `LENIENT`) is exposed as the base for customized configs, e.g. `DEFAULT_SQL_PARSER_CONFIG.withUnquotedCasing(Casing.UNCHANGED)`. ## Sole construction path + subclassing seam All public `ConverterProvider` constructors are `@Deprecated` in favour of `builder()`, and `DEFAULT` is `builder().build()`. Subclassing routes through a new `protected ConverterProvider(Builder)` seam — the single place instances are created — so adding a component later touches only the builder and this constructor, never a subclass `super(...)` call. `DynamicConverterProvider` and `AutomaticDynamicFunctionMappingConverterProvider` construct via `super(builder)`. Now that the provider is fully configurable, call sites that still used global defaults are routed through it: the builder's `typeConverter` reaches the scalar/aggregate/window converters, and the CREATE-statement catalog reader uses the provider's type factory and connection config. ## Compatibility Source-compatible — no public API removed, deprecated constructors still delegate to the builder, and behavior is unchanged for the default provider. ## Stack (#983 split, merge bottom-up) 1. #1035 — inject `ConverterProvider` into the SQL statement parsers (merged) 2. **#1036 — this PR** 3. #1037 — configurable unquoted identifier casing via the builder
…builder Add Builder.unquotedCasing(Casing) as a convenience for the common case of overriding only the unquoted-identifier casing, layered on top of the current parser configuration. Route the isthmus CLI's --unquotedcasing option through it, use it in the FromSql example (Casing.UNCHANGED, matching the lower-case identifiers in the example's CREATE TABLE statements), and add UnquotedCasingTest covering the default casing, the convenience for each Casing, the full sqlParserConfig path (retaining LENIENT conformance), and end-to-end NamedScan naming (EMPLOYEES under TO_UPPER, employees under UNCHANGED).
nielspardon
force-pushed
the
feat/builder-unquoted-casing
branch
from
July 31, 2026 08:50
3cda7ed to
130e81f
Compare
Member
Author
|
Rebased onto The one real conflict was positional: #1036 gained review changes that regrouped the |
vbarua
reviewed
Jul 31, 2026
…erConfig The convenience mutated the builder's parser config eagerly, so the result depended on call order: sqlParserConfig(y).unquotedCasing(x) produced y with x applied, while unquotedCasing(x).sqlParserConfig(y) silently discarded x. The Javadoc documented that ordering requirement rather than removing it. Hold the casing as an Optional<Casing> instead and apply it over the parser config in the primary constructor. Both orders now yield the same provider, and the rule is simply that the casing override wins over whatever casing the supplied config carries.
vbarua
reviewed
Aug 4, 2026
| builder | ||
| .unquotedCasing | ||
| .map(builder.sqlParserConfig::withUnquotedCasing) | ||
| .orElse(builder.sqlParserConfig); |
Member
There was a problem hiding this comment.
I think this is a reasonable solution to the problem of setting both.
vbarua
approved these changes
Aug 4, 2026
Address review feedback on the unquoted-casing convenience. Docs: drop the parser-config callouts from `builder()` and the hand-derivation warning from `unquotedCasing(...)`, and mention the convenience from `getSqlParserConfig()` in a paragraph of its own rather than parenthetically. Tests: fold the builder-level assertions into `ConverterProviderBuilderTest` as a nested `UnquotedCasing` section so they read as ConverterProvider behaviour rather than general casing functionality, and collapse the redundant cases. The end-to-end check moves to `SqlToSubstraitTest`, parameterized over the casings so it states the property directly: the `NamedScan` name follows whatever casing the provider carries.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add
ConverterProvider.Builder.unquotedCasing(Casing)— a convenience for the common case ofoverriding only the unquoted-identifier casing, layered on top of the current parser configuration
(equivalent to
sqlParserConfig(current.withUnquotedCasing(casing))).Motivation
TO_UPPERis the ANSI/Calcite default, but a large class of engines fold unquoted identifiers tolower-case or preserve them (Presto/Trino, Spark, DuckDB, Postgres), so a plan generated from
SELECT … FROM orderscomes out with aNamedScannamedORDERSthat those engines can't resolve.This gives library and CLI users a one-liner to control it, while the full
sqlParserConfig(...)path (previous PR) remains for anything else.
Changes
ConverterProvider.Builder.unquotedCasing(Casing)convenience.IsthmusEntryPoint(CLI):--unquotedcasingnow maps tobuilder().unquotedCasing(...).FromSqlexample: usesbuilder().unquotedCasing(Casing.UNCHANGED)(matching the lower-caseidentifiers in its
CREATE TABLEstatements), replacing the deprecatedconvert(sql, catalog, SqlDialect)call.Split of #983
Per @vbarua's request, #983 was split into three stacked PRs (review/merge bottom-up):
ConverterProviderinto the SQL statement parsers (merged)ConverterProviderbuilder (merged)Both dependencies have merged, so this PR is now rebased onto
mainand is the last commit in thestack — no longer blocked on anything.
🤖 Generated with AI