revert ColumnAnalysis type, add typeSignature and use it for DruidSchema #11895

clintropolis · 2021-11-09T01:01:09Z

Description

#11713 changed the segment metadata query column analysis type field, but this might not be very friendly to actual users (e.g. not DruidSchema) who are calling this query directly because, merging query results of mixed versions of Druid would result in an error due to mismatched types.

This PR reverts type to its original behavior, and adds a new typeSignature which is used by DruidSchema. Error messaging when merging ColumnAnalysis has also been improved to include the mismatched types involved in the merge.

Key changed/added classes in this PR

ColumnAnalysis
SegmentAnalyzer

This PR has:

been self-reviewed.
added documentation for new or modified features or behaviors.
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
been tested in a test Druid cluster.

gianm · 2021-11-09T01:05:23Z

docs/querying/segmentmetadataquery.md

-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>`).


show -> shown

gianm · 2021-11-09T01:06:08Z

docs/querying/segmentmetadataquery.md

-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`.


Consider adding:

New applications should use typeSignature, not type.

gianm · 2021-11-09T01:08:40Z

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()


Consider placing this getter above getType, which I think will put it earlier in the JSON and make it seem more "official".

gianm · 2021-11-09T01:09:09Z

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 +


Why not call this typeSignature?

gianm · 2021-11-09T01:17:53Z

processing/src/main/java/org/apache/druid/query/metadata/metadata/ColumnAnalysis.java

+      );
+    }
+
+    if (!typeSignature.equals(rhs.getTypeSignature())) {


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.

techdocsmith · 2021-11-09T02:00:23Z

docs/querying/segmentmetadataquery.md

-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>`).


Suggested change

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>`).

All columns contain a `typeSignature` that Druid uses to represent the column type information internally. The `typeSignature` is typically the same value used to identify the JSON type information at query or ingest time. One of: STRING`, `FLOAT`, `DOUBLE`, `LONG`, or `COMPLEX<typeName>`, e.g. `COMPLEX<hyperUnique>`.

techdocsmith · 2021-11-09T02:01:43Z

docs/querying/segmentmetadataquery.md

+
+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`.
+
+The timestamp column will always have `typeSignature` and `type` as `LONG`.


Suggested change

The timestamp column will always have `typeSignature` and `type` as `LONG`.

The `typeSignature` and `type` for the timestamp column is always `LONG`.

techdocsmith · 2021-11-09T02:03:47Z

docs/querying/segmentmetadataquery.md

-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`.


Suggested change

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`.

Druid retains the legacy friendly `type` name. For some column types, the value may match the `typeSignature` (`STRING`, `FLOAT`, `DOUBLE`, or `LONG`). For `COMPLEX` columns, the `type` only contains the name of the underlying complex type such as `hyperUnique`.

New applications should use `typeSignature`, not `type`.

lgtm-com · 2021-11-09T02:18:25Z

This pull request introduces 2 alerts when merging bff926e into a5bd0b8 - view on LGTM.com

new alerts:

2 for Dereferenced variable may be null

lgtm-com · 2021-11-09T03:36:48Z

This pull request introduces 2 alerts when merging be1c2ba into a5bd0b8 - view on LGTM.com

new alerts:

2 for Dereferenced variable may be null

abhishekagarwal87 · 2021-11-09T08:55:50Z

processing/src/main/java/org/apache/druid/query/metadata/metadata/ColumnAnalysis.java

@@ -47,6 +50,7 @@ public static ColumnAnalysis error(String reason)

  @JsonCreator
  public ColumnAnalysis(
+      @JsonProperty("typeSignature") ColumnType typeSignature,


can this change cause a problem during upgrade? E.g. an historical is on older version while broker is on newer version so the broker gets old json from the historical when running segment metadata queries?

the supported upgrade order is brokers last, but it would just be null, which shouldn't be a problem i think

https://github.com/apache/druid/pull/11895/files#diff-5aa1494dc5a13e82654e42ce8cdb3cb628c95233f8828312b3ede40dac1b8544R156 - shall this be modified to pick the one if the other is null instead of throwing an error. I am throwing darts in the dark here so please ignore if this is not really a problem :)

ah, if we supported updating the broker before data servers then we might want to consider that, but we don't (so that new query types, filters, aggregators, etc cannot be issued before data servers can understand them)

ah yes. you are right. thanks for clarifying.

docs/querying/segmentmetadataquery.md

techdocsmith

small nit. Otherwise LGTM.

docs/querying/segmentmetadataquery.md

Co-authored-by: Charles Smith <techdocsmith@gmail.com>

…ype-stuffs

lgtm-com · 2021-11-10T01:25:58Z

This pull request introduces 2 alerts when merging 9055810 into 6c196a5 - view on LGTM.com

new alerts:

2 for Dereferenced variable may be null

revert ColumnAnalysis type, add typeSignature and use it for DruidSchema

bff926e

gianm reviewed Nov 9, 2021

View reviewed changes

techdocsmith requested changes Nov 9, 2021

View reviewed changes

review stuffs

be1c2ba

clintropolis added Area - Querying Compatibility labels Nov 9, 2021

clintropolis added 2 commits November 8, 2021 19:44

maybe null

3cf5a81

better maybe null

d5a32c6

abhishekagarwal87 reviewed Nov 9, 2021

View reviewed changes

abhishekagarwal87 approved these changes Nov 9, 2021

View reviewed changes

gianm approved these changes Nov 9, 2021

View reviewed changes

clintropolis commented Nov 9, 2021

View reviewed changes

docs/querying/segmentmetadataquery.md Outdated Show resolved Hide resolved

Update docs/querying/segmentmetadataquery.md

17fd3dc

techdocsmith approved these changes Nov 9, 2021

View reviewed changes

docs/querying/segmentmetadataquery.md Outdated Show resolved Hide resolved

clintropolis and others added 2 commits November 9, 2021 10:57

Update docs/querying/segmentmetadataquery.md

aba20be

Co-authored-by: Charles Smith <techdocsmith@gmail.com>

Merge remote-tracking branch 'upstream/master' into column-analysis-t…

9055810

…ype-stuffs

clintropolis added 4 commits November 9, 2021 17:49

fix null right

a2214b2

sad

eca1c1c

oops

870fb3b

Update batch_hadoop_queries.json

945fed6

suneet-s merged commit 5baa221 into apache:master Nov 11, 2021

clintropolis deleted the column-analysis-type-stuffs branch November 12, 2021 19:05

This was referenced Dec 1, 2021

better types #11713

Merged

allow DruidSchema to fallback to segment metadata 'type' if 'typeSignature' is null #12016

Merged

abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

revert ColumnAnalysis type, add typeSignature and use it for DruidSchema #11895

revert ColumnAnalysis type, add typeSignature and use it for DruidSchema #11895

clintropolis commented Nov 9, 2021 •

edited

Loading

gianm Nov 9, 2021

gianm Nov 9, 2021

gianm Nov 9, 2021

gianm Nov 9, 2021

clintropolis Nov 9, 2021

gianm Nov 9, 2021

techdocsmith Nov 9, 2021

techdocsmith Nov 9, 2021

techdocsmith Nov 9, 2021

lgtm-com bot commented Nov 9, 2021

lgtm-com bot commented Nov 9, 2021

abhishekagarwal87 Nov 9, 2021

clintropolis Nov 9, 2021

abhishekagarwal87 Nov 9, 2021

clintropolis Nov 9, 2021 •

edited

Loading

abhishekagarwal87 Nov 9, 2021

techdocsmith left a comment

lgtm-com bot commented Nov 10, 2021


		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`.

		The timestamp column will always have `typeSignature` and `type` as `LONG`.

	The timestamp column will always have `typeSignature` and `type` as `LONG`.
	The `typeSignature` and `type` for the timestamp column is always `LONG`.

-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`.
+Druid retains the legacy friendly `type` name. For some column types, the value may match the `typeSignature`  (`STRING`, `FLOAT`, `DOUBLE`, or `LONG`). For `COMPLEX` columns, the `type` only contains the name of the underlying complex type such as `hyperUnique`.
+New applications should use `typeSignature`, not `type`.

revert ColumnAnalysis type, add typeSignature and use it for DruidSchema #11895

revert ColumnAnalysis type, add typeSignature and use it for DruidSchema #11895

Conversation

clintropolis commented Nov 9, 2021 • edited Loading

Description

Key changed/added classes in this PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Nov 9, 2021

lgtm-com bot commented Nov 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintropolis Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

techdocsmith left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Nov 10, 2021

clintropolis commented Nov 9, 2021 •

edited

Loading

clintropolis Nov 9, 2021 •

edited

Loading