gianm commented on a change in pull request #11895:
URL: https://github.com/apache/druid/pull/11895#discussion_r745211540
##########
File path:
processing/src/main/java/org/apache/druid/query/metadata/metadata/ColumnAnalysis.java
##########
@@ -73,6 +78,12 @@ public String getType()
return type;
}
+ @JsonProperty
+ public ColumnType getTypeSignature()
Review comment:
Consider placing this getter above `getType`, which I think will put it
earlier in the JSON and make it seem more "official".
##########
File path:
processing/src/main/java/org/apache/druid/query/metadata/metadata/ColumnAnalysis.java
##########
@@ -136,7 +147,19 @@ public ColumnAnalysis fold(ColumnAnalysis rhs)
}
if (!type.equals(rhs.getType())) {
- return ColumnAnalysis.error("cannot_merge_diff_types");
+ return ColumnAnalysis.error(
+ StringUtils.format("cannot_merge_diff_types: [%s] and [%s]", type,
rhs.getType())
+ );
+ }
+
+ if (!typeSignature.equals(rhs.getTypeSignature())) {
Review comment:
This'll NPE if `typeSignature` is null — i.e. if it came from a server
that is on an older version. I think this is OK, since people are supposed to
update Brokers after segment-serving-servers, but I'd still feel better if this
code was resilient to null `typeSignature`.
Which reminds me: I think we'll also need a trivial change in the cache key
to make sure SegmentAnalysis objects from shared caches are not re-used.
##########
File path: docs/querying/segmentmetadataquery.md
##########
@@ -87,9 +87,11 @@ The format of the result is:
} ]
```
-Dimension columns will have type `STRING`, `FLOAT`, `DOUBLE`, or `LONG`.
-Metric columns will have type `FLOAT`, `DOUBLE`, or `LONG`, or the name of the
underlying complex type such as `hyperUnique` in case of COMPLEX metric.
-Timestamp column will have type `LONG`.
+All columns will contain a `typeSignature` which is the Druid internal
representation of the type information for this column, is what is show in
[`INFORMATION_SCHEMA.COLUMNS`](../querying/sql.md#columns-table) table in SQL,
and is typically the value used to supply Druid with JSON type information at
query or ingest time. This value will be `STRING`, `FLOAT`, `DOUBLE`, `LONG`,
or `COMPLEX<typeName>` (e.g. `COMPLEX<hyperUnique>`).
+
+Additionally, columns will have a legacy friendly `type` name. This might
match `typeSignature` for some column types (`STRING`, `FLOAT`, `DOUBLE`, or
`LONG`) but for COMPLEX columns will only contain the name of the underlying
complex type such as `hyperUnique`.
Review comment:
Consider adding:
> New applications should use `typeSignature`, not `type`.
##########
File path: docs/querying/segmentmetadataquery.md
##########
@@ -87,9 +87,11 @@ The format of the result is:
} ]
```
-Dimension columns will have type `STRING`, `FLOAT`, `DOUBLE`, or `LONG`.
-Metric columns will have type `FLOAT`, `DOUBLE`, or `LONG`, or the name of the
underlying complex type such as `hyperUnique` in case of COMPLEX metric.
-Timestamp column will have type `LONG`.
+All columns will contain a `typeSignature` which is the Druid internal
representation of the type information for this column, is what is show in
[`INFORMATION_SCHEMA.COLUMNS`](../querying/sql.md#columns-table) table in SQL,
and is typically the value used to supply Druid with JSON type information at
query or ingest time. This value will be `STRING`, `FLOAT`, `DOUBLE`, `LONG`,
or `COMPLEX<typeName>` (e.g. `COMPLEX<hyperUnique>`).
Review comment:
show -> shown
##########
File path:
processing/src/main/java/org/apache/druid/query/metadata/metadata/ColumnAnalysis.java
##########
@@ -181,6 +205,7 @@ public String toString()
{
return "ColumnAnalysis{" +
"type='" + type + '\'' +
+ ", columnType=" + typeSignature +
Review comment:
Why not call this typeSignature?
--
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]
