refactor(isthmus): derive CREATE-statement table types from the provider - #1050
Conversation
| assertSame( | ||
| fieldType, | ||
| typeFactory.canonizeType(fieldType), | ||
| "column types should be interned in the provider's type factory"); |
There was a problem hiding this comment.
Nice targeted coverage around both entry points. Non-blocking test thought: Calcite uses a static interner here, so canonizing a field type cannot distinguish the provider factory from the global factory. Recording createSqlType calls on RecordingTypeFactory, or asserting a custom type-system result, would make this check protect the validator path too.
There was a problem hiding this comment.
You are right, and thanks — I checked and RelDataTypeFactoryImpl.canonize(RelDataType) in Calcite 1.42.0 compiles to getstatic DATATYPE_CACHE on a private static final Interner, so interning is JVM-wide and the comparison held regardless of which factory produced the type. That assertion was decorative: the test only failed without the fix because its size assertion duplicated the row-type check the other two tests already cover, so the validator path was effectively untested.
Fixed in c58bea6 by recording the requested SqlTypeNames and asserting the declared column types are among them.
Worth noting for anyone reading later: matching the specific names is what makes it work. A plain "was createSqlType called" flag would have been a false positive — with the fix reverted, the provider factory still sees [BOOLEAN, BOOLEAN, CHAR, NULL, ...] from catalog setup, and never BIGINT/VARCHAR/DECIMAL. So the assertion now fails for the right reason.
Also rebased onto current main while I was in there, since the branch had fallen behind.
There was a problem hiding this comment.
Filed #1057 for the other open question from this PR — EMPTY_CATALOG and VALIDATOR now have no callers anywhere, including inside their own class, so whether they stay public is worth deciding separately.
bvolpato
left a comment
bvolpato
left a comment
There was a problem hiding this comment.
LGTM! Clean provider-config propagation through CREATE parsing, with focused coverage for both entry points and defaults. Left one non-blocking test-strengthening note inline.
The builder work threaded the ConverterProvider into the CalciteCatalogReader construction, but the type derivation underneath still used global defaults on that same injected path: - createSubstraitTable built the row type with SubstraitTypeSystem.TYPE_FACTORY instead of the provider's type factory. - Column types came from col.dataType.deriveType(VALIDATOR), where VALIDATOR and EMPTY_CATALOG are static finals built on the global type factory and SqlConverterBase.CONNECTION_CONFIG. So a builder-configured typeFactory produced a catalog reader that used it while the tables inside were typed by the global one. Harmless today, since getCalciteConnectionConfig() returns the same case-insensitive config, but a custom type factory carrying a different type system was silently bypassed. Add createEmptyCatalog(ConverterProvider) and createValidator(ConverterProvider) helpers, build the validator once per call, and pass the provider's type factory and that validator into createSubstraitTable. EMPTY_CATALOG and VALIDATOR are retained and now defined via the same helpers against ConverterProvider.DEFAULT, so their values are unchanged. The validator is also built with the provider's SqlOperatorTable rather than a hardcoded SubstraitOperatorTable.INSTANCE, matching how the query path already sources its operator table. Identical for the default provider. CreateStatementParserProviderConfigTest covers both entry points; 3 of its 4 tests fail without this change.
…tory The column-derivation check compared a field type against canonize(...) on the provider's type factory, which proves nothing: RelDataTypeFactoryImpl.canonize interns through a private static Interner shared by every factory instance, so the comparison holds no matter which factory created the type. That test only failed without the fix because its size assertion duplicated the row-type check already covered by the other two tests. Record the SqlTypeNames requested from the factory instead and assert the declared column types are among them, which does exercise the validator path. Matching on the specific names matters: the factory is asked for BOOLEAN, CHAR and NULL while the catalog is set up regardless, so a plain "createSqlType was called" flag would pass even with a global validator.
d79c262 to
c58bea6
Compare
2 participants
Summary
Finishes the deferred half of @vbarua's comment on #1035 —
"I wonder if it would make sense to get the RelTypeFactory and SqlConverterBase from the
ConverterProvider as well". #1036 threaded the provider into the
CalciteCatalogReaderconstruction; this covers the type derivation underneath it.
The gap
On the provider-injected path (
processCreateStatementsToCatalog(provider, …)→processCreateStatementsToSchema(provider, …)→createSubstraitTable(…)):createSubstraitTablebuilt the row type withSubstraitTypeSystem.TYPE_FACTORY, not theprovider's type factory.
col.dataType.deriveType(VALIDATOR), whereVALIDATORandEMPTY_CATALOGarestatic final, built on the global type factory plusSqlConverterBase.CONNECTION_CONFIG.So a builder-configured
typeFactoryproduced a catalog reader that used it while the tablesinside were typed by the global one. That is harmless today —
getCalciteConnectionConfig()returns the same case-insensitive config as
SqlConverterBase.CONNECTION_CONFIG— but a customtype factory carrying a different type system (different decimal precision limits, say) was
silently bypassed for table column types.
Changes
createEmptyCatalog(ConverterProvider)andcreateValidator(ConverterProvider)helpers. Because
SubstraitSqlValidatortakes its type factory from the catalog reader, fixingthe empty catalog fixes column derivation as well.
createSubstraitTable.EMPTY_CATALOGandVALIDATORare kept and now defined via the same helpers againstConverterProvider.DEFAULT, so their values are unchanged and nothing public was removed.Note on scope
Two things worth calling out explicitly, both easy to drop if you would rather they were separate:
converterProvider.getSqlOperatorTable()instead of ahardcoded
SubstraitOperatorTable.INSTANCE, matching how feat(isthmus)!: inject ConverterProvider into the SQL statement parsers #1035 made the query path source itsoperator table. Identical for the default provider; it only matters for a provider that adds
operators.
EMPTY_CATALOGandVALIDATORnow have no callers inside the class either (each entry pointderives its own from the provider). They are still
public, so they are retained — but if youwant them deprecated in favour of the provider-derived path, that is a one-line follow-up.
Independent of #1037 — no overlapping files, so the two can merge in either order.
🤖 Generated with AI