FrankChen021 commented on code in PR #19769:
URL: https://github.com/apache/druid/pull/19769#discussion_r3665410648
##########
server/src/main/java/org/apache/druid/catalog/model/table/DatasourceDefn.java:
##########
@@ -130,8 +131,21 @@ public void validate(ResolvedTable table)
protected void validateColumn(ColumnSpec spec)
{
super.validateColumn(spec);
- if (Columns.isTimeColumn(spec.name()) && spec.dataType() != null) {
- // Validate type in next PR
+ // A column declared without a type is legal (the type is resolved from
the physical schema or the ingestion
+ // query), but a declared type must parse to a Druid type. This runs at
catalog write time only: reads never
+ // validate, so tables stored before this rule keep resolving (though
editing them surfaces the invalid type).
+ // Columns.druidType maps __time to LONG regardless of the declared type,
which ColumnSpec.validate already
+ // restricts to LONG or untyped.
+ if (spec.dataType() != null && Columns.druidType(spec) == null) {
Review Comment:
[P1] Validate declared types for external catalog tables
The new type check is scoped to DatasourceDefn, while
ExternalTableDefn.validateColumn remains empty. Posting an external catalog
table with dataType `COMPLEX<json` (now parsed as null by this PR) or `FOO`
therefore succeeds; BaseInputSourceDefn later calls Columns.convertSignature,
which replaces the null type with STRING. This preserves the silent schema
substitution this PR intends to reject and can make external ingestion use a
different type than declared. Move this parse check into shared table/column
validation or add equivalent external-table validation.
##########
processing/src/main/java/org/apache/druid/segment/column/Types.java:
##########
@@ -57,14 +58,16 @@ public static <T extends TypeSignature<?>> T
fromString(TypeFactory<T> typeFacto
case "COMPLEX":
return typeFactory.ofComplex(null);
default:
- // we do not convert to uppercase here, because complex type name must
be preserved in original casing
- // array could be converted, but are not for no particular reason
other than less spooky magic
- if (typeString.startsWith(ARRAY_PREFIX)) {
+ // Prefix matching is case-insensitive, consistent with the scalar
handling above, but the type parameter is
+ // taken from the original string: complex type names are
case-sensitive registry keys which must be preserved
+ // in their original casing (array element types recurse through this
method, so any casing works for them).
+ // A parameterized type without the closing bracket is malformed, not
a truncated parameter name.
+ if (upperTypeString.startsWith(ARRAY_PREFIX) &&
upperTypeString.endsWith(">")) {
Review Comment:
[P2] Handle unknown array element types as invalid input
`ARRAY<FOO>` passes this prefix/bracket guard, but the recursive parse
returns null and Preconditions.checkNotNull throws NullPointerException.
DatasourceDefn now passes every declared catalog type through this parser,
while CatalogResource maps only IAE and DruidException to a 400 response, so a
datasource without base-table metadata submitted with this invalid array type
produces a 500 instead of the intended invalid-input response. Return null here
or convert this case to InvalidInput, and add coverage for unsupported array
element 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
