Migrate built-in coercions from kAllowedCoercions to CastRulesRegistry#17017
Open
natashasehgal wants to merge 3 commits intofacebookincubator:mainfrom
Open
Migrate built-in coercions from kAllowedCoercions to CastRulesRegistry#17017natashasehgal wants to merge 3 commits intofacebookincubator:mainfrom
natashasehgal wants to merge 3 commits intofacebookincubator:mainfrom
Conversation
✅ Deploy Preview for meta-velox canceled.
|
|
@natashasehgal has exported this pull request. If you are a Meta employee, you can view the originating Diff in D99197524. |
Build Impact AnalysisSelective Build Targets (building these covers all 505 affected)Total affected: 505/557 targets Affected targets (505)Directly changed (431)
Transitively affected (74)
Fast path • Graph from main@472701f4a4828b995dcaa9f0dc44f00b20a2bf4c |
… types
Summary:
Register CastRulesRegistry rules for all Presto custom types that support casts: BigintEnum, VarcharEnum, BingTile, IPAddress, IPPrefix, Uuid, Json, P4HyperLogLog, and Spark TimestampNTZ.
Add a `coerceTypeBase(TypePtr, TypePtr)` overload that checks the CastRulesRegistry directly without reconstructing the type from a name. The existing string overload calls `getCustomType(name, {})` which throws for parametric custom types like BigintEnum whose factories require parameters. Internal call sites in `coercible()` and `leastCommonSuperType()` now use the safe TypePtr overload. The string overload is retained for SignatureBinder (which only has a type name), with a `typeSignature.parameters().empty()` guard to prevent parametric type names from reaching `getCustomType`.
Also fix CastRegistry.cpp to try rule lookup before checking for custom cast existence (enabling cast rules for custom types that extend RowType like IPPREFIX), and remove a duplicate assertion in IPPrefixTypeTest.
Differential Revision: D99173066
…perator methods Summary: CastRulesRegistry is now the sole source of truth for all custom type cast validation. CastExpr::applyPeeled() checks CastRulesRegistry::canCast() instead of per-operator isSupportedFromType()/isSupportedToType() methods. Changes: 1. **CastExpr.cpp**: Replace the two-step validation (registry → operator fallback) with a single `CastRulesRegistry::canCast()` check. If a CastOperator exists but the registry says the cast is unsupported, fail immediately. 2. **CastExpr.h**: Change `isSupportedFromType`/`isSupportedToType` from pure virtual to default-true and mark as deprecated. Subclasses no longer need to override them. 3. **All non-JSON CastOperator subclasses**: Remove `isSupportedFromType`/`isSupportedToType` overrides from BigintEnum, BingTile, IPAddress, IPPrefix, P4HyperLogLog, TimestampWithTimezone, TimeWithTimezone, UUID, VarcharEnum, TimestampNTZ. 4. **JsonCastOperator**: Remove recursive `isSupportedFromType`/`isSupportedToType` methods and the `isSupportedBasicType` helper. Register ARRAY/MAP/ROW ↔ JSON cast rules in CastRulesRegistry with recursive validator callbacks that check element/child type compatibility. Add missing DATE → JSON and DECIMAL → JSON (short decimal only) rules. Before: CastExpr checked the registry first, fell back to per-operator methods. After: CastExpr uses only the registry. The operator methods are deprecated dead code (default-true). Differential Revision: D99232117
Summary: `TypeCoercer::coerceTypeBase` maintained a separate static map (`kAllowedCoercions`) for built-in implicit coercions (TINYINT→SMALLINT, INTEGER→BIGINT, etc.) alongside the `CastRulesRegistry` used for custom types. This meant two lookup paths for the same concept — one lock-free static map for built-ins, one registry for custom types. This diff makes `CastRulesRegistry` the single source of truth for all coercion rules: 1. **Register built-in coercions in the registry.** Replace `kAllowedCoercions` with `registerBuiltInCoercions()`, called once on first use via a function-local static. Same type pairs, same costs — just stored in the registry instead of a separate map. 2. **Add `getScalarType(name)`.** A non-throwing type resolver for built-in scalar singletons (BOOLEAN, INTEGER, VARCHAR, etc.) and custom types (JSON, UUID, etc.). Skips parametric types (ARRAY, MAP, ROW) since they aren't valid leaf coercion targets. Replaces `getCustomType()` in TypeCoercer, which returned nullptr for built-in types and silently dropped registry lookups for built-in pairs. 3. **Relax `registerCastRules()` validation.** Previously required a CastOperator on at least one side, blocking built-in→built-in rules. Now also accepts rules where both types are known built-in types, since Velox's native cast machinery in CastExpr handles execution. Before: `coerceTypeBase` checked `kAllowedCoercions` first, then fell back to `CastRulesRegistry` (which couldn't see built-in types anyway). After: `coerceTypeBase` uses only `CastRulesRegistry` for all coercion lookups — built-in and custom. Differential Revision: D99197524
a1c1344 to
9ecf3cc
Compare
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
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:
TypeCoercer::coerceTypeBasemaintained a separate static map (kAllowedCoercions) for built-in implicit coercions (TINYINT→SMALLINT, INTEGER→BIGINT, etc.) alongside theCastRulesRegistryused for custom types. This meant two lookup paths for the same concept — one lock-free static map for built-ins, one registry for custom types.This diff makes
CastRulesRegistrythe single source of truth for all coercion rules:Register built-in coercions in the registry. Replace
kAllowedCoercionswithregisterBuiltInCoercions(), called once on first use via a function-local static. Same type pairs, same costs — just stored in the registry instead of a separate map.Add
getScalarType(name). A non-throwing type resolver for built-in scalar singletons (BOOLEAN, INTEGER, VARCHAR, etc.) and custom types (JSON, UUID, etc.). Skips parametric types (ARRAY, MAP, ROW) since they aren't valid leaf coercion targets. ReplacesgetCustomType()in TypeCoercer, which returned nullptr for built-in types and silently dropped registry lookups for built-in pairs.Relax
registerCastRules()validation. Previously required a CastOperator on at least one side, blocking built-in→built-in rules. Now also accepts rules where both types are known built-in types, since Velox's native cast machinery in CastExpr handles execution.Before:
coerceTypeBasecheckedkAllowedCoercionsfirst, then fell back toCastRulesRegistry(which couldn't see built-in types anyway).After:
coerceTypeBaseuses onlyCastRulesRegistryfor all coercion lookups — built-in and custom.Differential Revision: D99197524