cecemei commented on code in PR #18403:
URL: https://github.com/apache/druid/pull/18403#discussion_r2335253054
##########
processing/src/main/java/org/apache/druid/data/input/impl/AggregateProjectionSpec.java:
##########
@@ -230,12 +234,40 @@ private static ProjectionOrdering
computeOrdering(VirtualColumns virtualColumns,
} else {
final VirtualColumn vc =
virtualColumns.getVirtualColumn(dimension.getName());
final Granularity maybeGranularity =
Granularities.fromVirtualColumn(vc);
- if (granularity == null && maybeGranularity != null) {
- granularity = maybeGranularity;
- timeColumnName = dimension.getName();
- } else if (granularity != null && maybeGranularity != null &&
maybeGranularity.isFinerThan(granularity)) {
- granularity = maybeGranularity;
- timeColumnName = dimension.getName();
+ if (maybeGranularity == null ||
maybeGranularity.equals(Granularities.ALL)) {
+ // no __time in inputs or not supported, skip
+ continue;
+ }
+ if (granularity == null) {
+ if (maybeGranularity.getClass().equals(PeriodGranularity.class)
+ && maybeGranularity.getTimeZone().equals(DateTimeZone.UTC)
+ && ((PeriodGranularity) maybeGranularity).getOrigin() == null) {
+ granularity = maybeGranularity;
+ timeColumnName = dimension.getName();
+ continue;
+ } else {
+ // we don't allow:
+ // 1. virtual column on __time if there's no grouping on __time
+ // 2. non-UTC or non-epoch origin period granularity
+ throw InvalidInput.exception(
+ "cannot use granularity[%s] on column[%s] as projection time
column",
+ maybeGranularity,
+ dimension.getName()
+ );
Review Comment:
ah i was thinking about some cases when there's a time-like virtual column
(non-utc) and another time-like virtual column (utc, can be converted to gran),
e.x. `PT1H` in Pacific and `P1D` in UTC, if we just ignore, we would just use
floor(__time, `P1D`) as the time column, and the virtual column `PT1H` in
Pacific would just be computed based on floor(__time, `P1D`), that would be
incorrect.
it seems simple and safe to just assume the first time-like grouping column
must be gran, and the following must be coarser, e.x. `PT1H` in UTC and `P1D`
in Pacific is an acceptable config. Another case is `PT2H`, `PT3H`, `PT1H`, it
seems a bit more complex (or calculated) if we don't have the FIRST time-like
must be gran assumption.
--
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]
