feat(isthmus)!: inject ConverterProvider into the SQL statement parsers#1035
Merged
Conversation
Thread a single ConverterProvider through both the CREATE-statement parsing that builds the Calcite catalog and the query parsing, so one parser configuration (the provider's getSqlParserConfig()) is applied consistently end-to-end. Add a shared ConverterProvider.DEFAULT instance for the no-arg parsing entry points. The SqlParser.Config-based parsing entry points (public since v0.94.0) are removed in favour of passing a ConverterProvider: - SubstraitSqlStatementParser.parseStatements(String, SqlParser.Config) - SubstraitSqlToCalcite.convertQueries(..., SqlParser.Config) overloads SqlToSubstrait.convert(String, CatalogReader, SqlDialect) is deprecated: the SqlDialect argument no longer influences parsing and it delegates to convert(String, CatalogReader). Parser configuration is customised by subclassing ConverterProvider and overriding getSqlParserConfig().
vbarua
reviewed
Jul 24, 2026
| ConverterProvider converterProvider, | ||
| SqlOperatorTable operatorTable) | ||
| throws SqlParseException { | ||
| SqlValidator validator = new SubstraitSqlValidator(catalogReader, operatorTable); |
Member
There was a problem hiding this comment.
Now that we're injecting the ConverterProvider, should we use the operatorTable available in ConverterProvider#getSqlOperator here?
Member
Author
There was a problem hiding this comment.
good catch. pushed a commit that refactors this part
vbarua
reviewed
Jul 24, 2026
vbarua
reviewed
Jul 24, 2026
| * @return list of {@link SubstraitTable}s generated from the CREATE statements | ||
| * @throws SqlParseException if parsing fails or statements are invalid | ||
| */ | ||
| public static List<SubstraitTable> processCreateStatements( |
Member
There was a problem hiding this comment.
minor: would it make sense to move this under the original processCreateStatement from which it was extracted? Would also have made the diff easier to review.
Member
Author
There was a problem hiding this comment.
pushed a commit that moves the method as suggested
vbarua
reviewed
Jul 24, 2026
| rootSchema, | ||
| defaultSchema, | ||
| SubstraitTypeSystem.TYPE_FACTORY, | ||
| SqlConverterBase.CONNECTION_CONFIG); |
Member
There was a problem hiding this comment.
(not for this PR) I wonder if it would make sense to get the RelTypeFactory and SqlConverterBase from the ConverterProvider as well. The former is already configured in the ConverterProvider.
Member
Author
There was a problem hiding this comment.
can probably include that in the builder PR
…ibling Relocate processCreateStatements(ConverterProvider, String) to sit directly beneath the single-argument processCreateStatements(String) that delegates to it, grouping the two overloads together. Pure code move; no behavior change.
The SQL statement converters now derive the operator table from the injected ConverterProvider rather than accepting it as a separate parameter, so one provider configures both parsing and the valid operators end-to-end: - convertQuery/convertQueries build the validator with converterProvider.getSqlOperatorTable(), dropping the redundant convertQueries(ConverterProvider, SqlOperatorTable) overload and the SqlToSubstrait.operatorTable field that fed it. - Add convertQuery(String, CatalogReader, ConverterProvider) mirroring the plural overload, and migrate callers off passing the operator table by hand. - SqlExpressionToSubstrait's extended-expression validator now honors the provider's operator table too, matching the query path. - Memoize DynamicConverterProvider.getSqlOperatorTable() so reusing a converter across conversions no longer rebuilds the dynamic operator table each call. - Deprecate the raw-SqlOperatorTable convertQuery/convertQueries overloads in favour of the ConverterProvider ones.
vbarua
approved these changes
Jul 24, 2026
vbarua
left a comment
vbarua
left a comment
Member
There was a problem hiding this comment.
LGTM
I'll leave the merging to you. The PR message is a bit of mouthful for a commit message, especially because we'll try and generate the release notes from it as well.
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
Thread a single
ConverterProviderthrough both the CREATE-statement parsing that builds theCalcite catalog and the query parsing, so one parser configuration (the provider's
getSqlParserConfig()) is applied consistently end-to-end. Previously the CREATE-statement path andthe query path could apply different parser settings, so the identifier casing used to build a
NamedScanname could disagree with the casing used to resolve it in a query.Also adds a shared
ConverterProvider.DEFAULTinstance, used as the default for the no-arg parsingentry points.
Breaking changes
The
SqlParser.Config-based parsing entry points (public sincev0.94.0) are removed in favourof passing a
ConverterProvider:SubstraitSqlStatementParser.parseStatements(String, SqlParser.Config)SubstraitSqlToCalcite.convertQueries(String, CatalogReader, SqlParser.Config)and the5-arg
SqlParser.ConfigoverloadSqlToSubstrait.convert(String, CatalogReader, SqlDialect)is deprecated: theSqlDialectargument no longer influences parsing and it delegates to
convert(String, CatalogReader). Until thebuilder arrives (next PR in the stack), parser configuration is customised by subclassing
ConverterProviderand overridinggetSqlParserConfig().Notable changes
SubstraitSqlStatementParserparseStatements(String, ConverterProvider)owns theSqlParserinstantiation;SqlParser.Configoverload removedSubstraitSqlToCalciteconvertQueries(sql, catalog, ConverterProvider[, operatorTable]); internal overloads route throughConverterProvider;SqlParser.Configoverloads removedSubstraitCreateStatementParserprocessCreateStatements(ConverterProvider, …)/processCreateStatementsToCatalog(ConverterProvider, …)overloads — same parser config as query parsingSqlToSubstraitconvert(sql, catalog)uses theConverterProviderpath;convert(sql, catalog, SqlDialect)@Deprecated(arg ignored)SqlExpressionToSubstraitConverterProviderDEFAULTinstanceIsthmusEntryPoint(CLI)Split of #983
Per @vbarua's request, #983 is split into three stacked PRs (review/merge bottom-up):
ConverterProviderinto the SQL statement parsers (this PR)ConverterProviderbuilderReplaces #983.
🤖 Generated with AI