fix: clustered segment catalog spec complex handling - #19842
fix: clustered segment catalog spec complex handling#19842clintropolis wants to merge 2 commits into
Conversation
github-actions
Bot
added
Area - Batch Ingestion
Area - Segment Format and Ser/De
Area - MSQ
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
Reviewed 5 of 5 changed files.
This is an automated review by Codex GPT-5.6-Sol
| // Other complex types cannot be ingested into a clustered base table: there is no dimension handler for them, | ||
| // and clustered base tables have no aggregators to produce them. | ||
| if (druidType.is(ValueType.COMPLEX)) { | ||
| return complexDimensionSchema(column.name(), druidType); |
There was a problem hiding this comment.
[P2] Reject handler schemas that change the declared type
This path accepts the provider's DimensionSchema without checking that schema.getColumnType() equals the catalog's declared complex type. The new test demonstrates the problem by declaring COMPLEX but producing a DoubleDimensionSchema. During ingestion, DimensionSchema.getDimensionHandler() then selects the double handler from that returned schema, so complex values fail conversion or are stored as a type that contradicts the sealed catalog schema. Apply the same type-consistency check used for custom schemas before accepting the provider result.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
Reviewed 5 of 5 changed files.
This is an automated review by Codex GPT-5.6-Sol
| druidType | ||
| ); | ||
| } | ||
| if (!druidType.equals(schema.getColumnType())) { |
There was a problem hiding this comment.
[P2] Reject handler schemas that rename the column
The new guard validates only the returned schema's type. A provider can still return the correct complex type under a different column name, which createSpec accepts verbatim. Downstream clustered ingestion then reads row.getRaw(schema.getName()), silently storing nulls for the declared catalog column and exposing the provider-chosen name instead. Validate name.equals(schema.getName()) alongside the type.
| } | ||
| // Other complex types cannot be ingested into a clustered base table: there is no dimension handler for them, | ||
| // and clustered base tables have no aggregators to produce them. | ||
| if (druidType.is(ValueType.COMPLEX)) { |
There was a problem hiding this comment.
Can we remove the special case for NESTED_DATA? That would help prove this system works and potentially get some extra test coverage.
| public static DimensionSchema getComplexDimensionSchema(String dimensionName, ColumnType type) | ||
| { | ||
| if (!type.is(ValueType.COMPLEX) || type.getComplexTypeName() == null) { | ||
| throw new IAE("Type [%s] is not a named complex type", type); |
There was a problem hiding this comment.
Consider using InvalidInput. Please include dimensionName in the error message.
| { | ||
| final DimensionHandlerProvider provider = DIMENSION_HANDLER_PROVIDERS.get(complexTypeName); | ||
| if (provider == null) { | ||
| throw new ISE("Can't find DimensionHandlerProvider for typeName [%s]", complexTypeName); |
There was a problem hiding this comment.
I see this error was pre-existing, but still, it's a funny error for someone to get if they provide an invalid complex type. Consider rewording it to include dimensionName, to be an InvalidInput, and to say something more user friendly like Complex type[%s] for dimension[%s] is not a valid type.
| ); | ||
| } | ||
| if (!druidType.equals(schema.getColumnType())) { | ||
| throw InvalidInput.exception( |
There was a problem hiding this comment.
Push this check up to DimensionHandlerUtils?
| catch (ISE e) { | ||
| // No handler is registered for this complex type, which usually means the extension that defines it is not | ||
| // loaded on whichever service is validating the spec. | ||
| throw InvalidInput.exception( |
There was a problem hiding this comment.
If the errors in DimensionHandlerUtils are made more friendly then this catch + rethrow won't be needed.
3 participants
Description
ClusteredValueGroupsBaseTableMetadatanow checks for a complexDimensionHandlerfrom a new methodDimensionHandlerUtils#getComplexDimensionSchemawhich checksDIMENSION_HANDLER_PROVIDERSdirectly (instead of using the existingDimensionHandlerUtils#getHandlerFromCapabilitieswhich now shares the underlying logic with this new method).DimensionSchemaUtilshas also been updated to use this new method as it was already only callinggetHandlerFromCapabilitiesfor COMPLEX types.