capistrant commented on code in PR #19579:
URL: https://github.com/apache/druid/pull/19579#discussion_r3424171847
##########
processing/src/main/java/org/apache/druid/data/input/impl/ClusteredValueGroupsBaseTableProjectionSpec.java:
##########
@@ -193,40 +206,45 @@ private static void validate(List<DimensionSchema>
columns, List<String> cluster
}
int timeIndex = -1;
- boolean bothPresent = false;
for (int i = 0; i < columns.size(); i++) {
final String name = columns.get(i).getName();
- if (ColumnHolder.TIME_COLUMN_NAME.equals(name) ||
Granularities.GRANULARITY_VIRTUAL_COLUMN_NAME.equals(name)) {
- if (timeIndex >= 0) {
- bothPresent = true;
- }
+ // The query-granularity virtual column is a granularity carrier in
virtualColumns (it floors the stored __time
+ // column); it is not itself a stored column, so it must not be declared
in 'columns'.
+ if (Granularities.GRANULARITY_VIRTUAL_COLUMN_NAME.equals(name)) {
+ throw InvalidInput.exception(
+ "[%s] is the query-granularity virtual column, not a stored
column; declare it in 'virtualColumns' and use"
+ + " [%s] as the time column in 'columns'",
+ Granularities.GRANULARITY_VIRTUAL_COLUMN_NAME,
+ ColumnHolder.TIME_COLUMN_NAME
+ );
+ }
+ if (ColumnHolder.TIME_COLUMN_NAME.equals(name)) {
timeIndex = i;
}
}
if (timeIndex < 0) {
throw InvalidInput.exception(
- "clustered base table must include %s (or the query-granularity
column [%s]) in 'columns' to define the"
- + " time position",
- ColumnHolder.TIME_COLUMN_NAME,
- Granularities.GRANULARITY_VIRTUAL_COLUMN_NAME
- );
- }
- if (bothPresent) {
- throw InvalidInput.exception(
- "clustered base table must include exactly one of %s / %s in
'columns' to define the time position",
- ColumnHolder.TIME_COLUMN_NAME,
- Granularities.GRANULARITY_VIRTUAL_COLUMN_NAME
+ "clustered base table must include [%s] in 'columns' to define the
time position",
+ ColumnHolder.TIME_COLUMN_NAME
);
}
if (timeIndex < clusteringColumns.size()) {
throw InvalidInput.exception(
- "clustering by %s / %s is not yet supported; the time column must be
a non-clustering column",
- ColumnHolder.TIME_COLUMN_NAME,
- Granularities.GRANULARITY_VIRTUAL_COLUMN_NAME
+ "clustering by [%s] is not yet supported; the time column must be a
non-clustering column",
+ ColumnHolder.TIME_COLUMN_NAME
);
}
}
+ private static boolean isSupportedClusteringType(ColumnType type)
Review Comment:
`Projections.isAllowedClusteringType` is an existing public utility doing
the same. I think consolidating would be helpful to make future changes to
supported clustering types (if any) require less fixups
--
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]
