You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "clintropolis (via GitHub)" <gi...@apache.org> on 2026/07/31 01:19:53 UTC
[PR] fix: clustered segment catalog spec complex handling (druid)
clintropolis opened a new pull request, #19842:
URL: https://github.com/apache/druid/pull/19842
### Description
`ClusteredValueGroupsBaseTableMetadata` now checks for a complex `DimensionHandler` from a new method `DimensionHandlerUtils#getComplexDimensionSchema` which checks `DIMENSION_HANDLER_PROVIDERS` directly (instead of using the existing `DimensionHandlerUtils#getHandlerFromCapabilities` which now shares the underlying logic with this new method). `DimensionSchemaUtils` has also been updated to use this new method as it was already only calling `getHandlerFromCapabilities` for COMPLEX types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: clustered segment catalog spec complex handling (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #19842:
URL: https://github.com/apache/druid/pull/19842#discussion_r3706929676
##########
server/src/main/java/org/apache/druid/catalog/model/ClusteredValueGroupsBaseTableMetadata.java:
##########
@@ -212,16 +215,54 @@ private DimensionSchema toDimensionSchema(ColumnSpec column, @Nullable Dimension
if (ColumnType.NESTED_DATA.equals(druidType)) {
return new NestedDataColumnSchema(column.name(), NestedDataColumnSchema.DEFAULT_FORMAT_VERSION);
}
- // 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)) {
Review Comment:
Can we remove the special case for `NESTED_DATA`? That would help prove this system works and potentially get some extra test coverage.
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java:
##########
@@ -130,17 +131,44 @@ private DimensionHandlerUtils()
}
if (capabilities.is(ValueType.COMPLEX) && capabilities.getComplexTypeName() != null) {
- DimensionHandlerProvider provider = DIMENSION_HANDLER_PROVIDERS.get(capabilities.getComplexTypeName());
- if (provider == null) {
- throw new ISE("Can't find DimensionHandlerProvider for typeName [%s]", capabilities.getComplexTypeName());
- }
- return provider.get(dimensionName);
+ return getHandlerForComplexType(dimensionName, capabilities.getComplexTypeName());
}
// Return a StringDimensionHandler by default (null columns will be treated as String typed)
return new StringDimensionHandler(dimensionName, multiValueHandling, true, false);
}
+ /**
+ * The {@link DimensionHandler} registered for a complex type. Complex columns are stored by type-specific handlers,
+ * so a type contributed by an extension becomes storable as soon as that extension registers one.
+ *
+ * @throws ISE if no handler is registered for the type, which usually means the extension defining it is not loaded
+ */
+ public static DimensionHandler<?, ?, ?> getHandlerForComplexType(String dimensionName, String complexTypeName)
+ {
+ final DimensionHandlerProvider provider = DIMENSION_HANDLER_PROVIDERS.get(complexTypeName);
+ if (provider == null) {
+ throw new ISE("Can't find DimensionHandlerProvider for typeName [%s]", complexTypeName);
+ }
+ return provider.get(dimensionName);
+ }
+
+ /**
+ * The {@link DimensionSchema} to use when storing a complex column of the given type, for callers that have a
+ * declared type rather than an existing column. Handlers are free to consult the {@link ColumnCapabilities} they
+ * are given, so a default set describing the type is supplied on the caller's behalf.
+ *
+ * @throws ISE if no handler is registered for the type, which usually means the extension defining it is not loaded
+ */
+ 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);
Review Comment:
Consider using `InvalidInput`. Please include `dimensionName` in the error message.
##########
server/src/main/java/org/apache/druid/catalog/model/ClusteredValueGroupsBaseTableMetadata.java:
##########
@@ -212,16 +215,54 @@ private DimensionSchema toDimensionSchema(ColumnSpec column, @Nullable Dimension
if (ColumnType.NESTED_DATA.equals(druidType)) {
return new NestedDataColumnSchema(column.name(), NestedDataColumnSchema.DEFAULT_FORMAT_VERSION);
}
- // 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);
+ }
throw InvalidInput.exception(
- "column [%s] has unsupported type [%s] for a clustered base table; supported types are primitive, primitive"
- + " array, and COMPLEX<json> columns",
+ "column [%s] has unsupported type [%s] for a clustered base table",
column.name(),
druidType
);
}
+ /**
+ * Resolve a complex column through its registered {@link org.apache.druid.segment.DimensionHandler}, so that any
+ * complex type which can be stored as a dimension may be declared, including types contributed by extensions. The
+ * handler is looked up by the complex type name, so the schema it returns is specific to the declared type.
+ * <p>
+ * The returned schema is checked against the declared type before it is accepted. A schema selects its own handler
+ * at ingest time (via {@link DimensionSchema#getDimensionHandler()}, which reads
+ * {@link DimensionSchema#getColumnType()}), so a schema of some other type would quietly store the column as that
+ * type instead, contradicting the declared schema that queries are validated and coerced against.
+ */
+ private static DimensionSchema complexDimensionSchema(String name, ColumnType druidType)
+ {
+ final DimensionSchema schema;
+ try {
+ schema = DimensionHandlerUtils.getComplexDimensionSchema(name, druidType);
+ }
+ 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(
+ "column [%s] has type [%s], which cannot be stored as a dimension of a clustered base table; if this type"
+ + " comes from an extension, check that the extension is loaded",
+ name,
+ druidType
+ );
+ }
+ if (!druidType.equals(schema.getColumnType())) {
+ throw InvalidInput.exception(
Review Comment:
Push this check up to `DimensionHandlerUtils`?
##########
server/src/main/java/org/apache/druid/catalog/model/ClusteredValueGroupsBaseTableMetadata.java:
##########
@@ -212,16 +215,54 @@ private DimensionSchema toDimensionSchema(ColumnSpec column, @Nullable Dimension
if (ColumnType.NESTED_DATA.equals(druidType)) {
return new NestedDataColumnSchema(column.name(), NestedDataColumnSchema.DEFAULT_FORMAT_VERSION);
}
- // 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);
+ }
throw InvalidInput.exception(
- "column [%s] has unsupported type [%s] for a clustered base table; supported types are primitive, primitive"
- + " array, and COMPLEX<json> columns",
+ "column [%s] has unsupported type [%s] for a clustered base table",
column.name(),
druidType
);
}
+ /**
+ * Resolve a complex column through its registered {@link org.apache.druid.segment.DimensionHandler}, so that any
+ * complex type which can be stored as a dimension may be declared, including types contributed by extensions. The
+ * handler is looked up by the complex type name, so the schema it returns is specific to the declared type.
+ * <p>
+ * The returned schema is checked against the declared type before it is accepted. A schema selects its own handler
+ * at ingest time (via {@link DimensionSchema#getDimensionHandler()}, which reads
+ * {@link DimensionSchema#getColumnType()}), so a schema of some other type would quietly store the column as that
+ * type instead, contradicting the declared schema that queries are validated and coerced against.
+ */
+ private static DimensionSchema complexDimensionSchema(String name, ColumnType druidType)
+ {
+ final DimensionSchema schema;
+ try {
+ schema = DimensionHandlerUtils.getComplexDimensionSchema(name, druidType);
+ }
+ 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(
Review Comment:
If the errors in DimensionHandlerUtils are made more friendly then this catch + rethrow won't be needed.
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java:
##########
@@ -130,17 +131,44 @@ private DimensionHandlerUtils()
}
if (capabilities.is(ValueType.COMPLEX) && capabilities.getComplexTypeName() != null) {
- DimensionHandlerProvider provider = DIMENSION_HANDLER_PROVIDERS.get(capabilities.getComplexTypeName());
- if (provider == null) {
- throw new ISE("Can't find DimensionHandlerProvider for typeName [%s]", capabilities.getComplexTypeName());
- }
- return provider.get(dimensionName);
+ return getHandlerForComplexType(dimensionName, capabilities.getComplexTypeName());
}
// Return a StringDimensionHandler by default (null columns will be treated as String typed)
return new StringDimensionHandler(dimensionName, multiValueHandling, true, false);
}
+ /**
+ * The {@link DimensionHandler} registered for a complex type. Complex columns are stored by type-specific handlers,
+ * so a type contributed by an extension becomes storable as soon as that extension registers one.
+ *
+ * @throws ISE if no handler is registered for the type, which usually means the extension defining it is not loaded
+ */
+ public static DimensionHandler<?, ?, ?> getHandlerForComplexType(String dimensionName, String complexTypeName)
+ {
+ final DimensionHandlerProvider provider = DIMENSION_HANDLER_PROVIDERS.get(complexTypeName);
+ if (provider == null) {
+ throw new ISE("Can't find DimensionHandlerProvider for typeName [%s]", complexTypeName);
Review 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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: clustered segment catalog spec complex handling (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis merged PR #19842:
URL: https://github.com/apache/druid/pull/19842
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: clustered segment catalog spec complex handling (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19842:
URL: https://github.com/apache/druid/pull/19842#discussion_r3695810847
##########
server/src/main/java/org/apache/druid/catalog/model/ClusteredValueGroupsBaseTableMetadata.java:
##########
@@ -212,16 +215,54 @@ private DimensionSchema toDimensionSchema(ColumnSpec column, @Nullable Dimension
if (ColumnType.NESTED_DATA.equals(druidType)) {
return new NestedDataColumnSchema(column.name(), NestedDataColumnSchema.DEFAULT_FORMAT_VERSION);
}
- // 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);
+ }
throw InvalidInput.exception(
- "column [%s] has unsupported type [%s] for a clustered base table; supported types are primitive, primitive"
- + " array, and COMPLEX<json> columns",
+ "column [%s] has unsupported type [%s] for a clustered base table",
column.name(),
druidType
);
}
+ /**
+ * Resolve a complex column through its registered {@link org.apache.druid.segment.DimensionHandler}, so that any
+ * complex type which can be stored as a dimension may be declared, including types contributed by extensions. The
+ * handler is looked up by the complex type name, so the schema it returns is specific to the declared type.
+ * <p>
+ * The returned schema is checked against the declared type before it is accepted. A schema selects its own handler
+ * at ingest time (via {@link DimensionSchema#getDimensionHandler()}, which reads
+ * {@link DimensionSchema#getColumnType()}), so a schema of some other type would quietly store the column as that
+ * type instead, contradicting the declared schema that queries are validated and coerced against.
+ */
+ private static DimensionSchema complexDimensionSchema(String name, ColumnType druidType)
+ {
+ final DimensionSchema schema;
+ try {
+ schema = DimensionHandlerUtils.getComplexDimensionSchema(name, druidType);
+ }
+ 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(
+ "column [%s] has type [%s], which cannot be stored as a dimension of a clustered base table; if this type"
+ + " comes from an extension, check that the extension is loaded",
+ name,
+ druidType
+ );
+ }
+ if (!druidType.equals(schema.getColumnType())) {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: clustered segment catalog spec complex handling (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19842:
URL: https://github.com/apache/druid/pull/19842#discussion_r3690441937
##########
server/src/main/java/org/apache/druid/catalog/model/ClusteredValueGroupsBaseTableMetadata.java:
##########
@@ -212,16 +215,38 @@ private DimensionSchema toDimensionSchema(ColumnSpec column, @Nullable Dimension
if (ColumnType.NESTED_DATA.equals(druidType)) {
return new NestedDataColumnSchema(column.name(), NestedDataColumnSchema.DEFAULT_FORMAT_VERSION);
}
- // 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);
Review 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<clusteredBaseTableTestType> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org