You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "clintropolis (via GitHub)" <gi...@apache.org> on 2025/08/20 17:30:04 UTC
Re: [PR] Consolidate the conversion between Granularity and VirtualColumn, and improve the mapping of granularity usage in projections. (druid)
clintropolis commented on code in PR #18403:
URL: https://github.com/apache/druid/pull/18403#discussion_r2277941363
##########
processing/src/main/java/org/apache/druid/segment/AggregateProjectionMetadata.java:
##########
@@ -213,16 +214,19 @@ public Schema(
int foundTimePosition = -1;
this.orderingWithTimeSubstitution = Lists.newArrayListWithCapacity(ordering.size());
- Granularity granularity = null;
+ Granularity granularity = Granularities.ALL;
for (int i = 0; i < ordering.size(); i++) {
OrderBy orderBy = ordering.get(i);
if (orderBy.getColumnName().equals(timeColumnName)) {
orderingWithTimeSubstitution.add(new OrderBy(ColumnHolder.TIME_COLUMN_NAME, orderBy.getOrder()));
+ if (foundTimePosition != -1) {
+ throw DruidException.defensive("projection[%s] has multiple time columns in ordering: %s", name, ordering);
+ }
foundTimePosition = i;
timeColumnName = groupingColumns.get(foundTimePosition);
final VirtualColumn vc = this.virtualColumns.getVirtualColumn(groupingColumns.get(foundTimePosition));
if (vc != null) {
- granularity = Granularities.fromVirtualColumn(vc);
+ granularity = MoreObjects.firstNonNull(Granularities.fromVirtualColumn(vc), Granularities.ALL);
Review Comment:
this seems less clear to me than just initializing to null and letting it naturally fill in like it was doing previously
##########
processing/src/main/java/org/apache/druid/java/util/common/granularity/Granularities.java:
##########
@@ -194,14 +192,24 @@ public static ExpressionVirtualColumn toVirtualColumn(Granularity granularity, S
public static Granularity fromVirtualColumn(VirtualColumn virtualColumn)
{
if (virtualColumn instanceof ExpressionVirtualColumn) {
- final ExpressionVirtualColumn expressionVirtualColumn = (ExpressionVirtualColumn) virtualColumn;
- final Expr expr = expressionVirtualColumn.getParsedExpression().get();
- if (expr instanceof TimestampFloorExprMacro.TimestampFloorExpr) {
- final TimestampFloorExprMacro.TimestampFloorExpr gran = (TimestampFloorExprMacro.TimestampFloorExpr) expr;
- if (gran.getArg().getBindingIfIdentifier() != null) {
- return gran.getGranularity();
- }
- }
+ return fromExpr(((ExpressionVirtualColumn) virtualColumn).getParsedExpression().get());
+ }
+ return null;
+ }
+
+ @Nullable
+ private static Granularity fromExpr(Expr expr)
+ {
+ String identifier = expr.getIdentifierIfIdentifier();
Review Comment:
wny switch to `getIdentifierIfIdentifier`? It doesn't matter really here probably, but `getBindingIfIdentifier` is more correct since the binding refers to the underlying column input instead of the variable name in the expression
##########
processing/src/main/java/org/apache/druid/java/util/common/granularity/PeriodGranularity.java:
##########
@@ -216,6 +226,139 @@ public String toString()
'}';
}
+ /**
+ * Returns true if this granularity can be mapped to the target granularity. For example:
+ * <li>Period('PT1H') in UTC can be mapped to Period('P1D') in UTC</li>
+ * <li>Period('PT1H') in America/Los_Angeles can be mapped to Period('PT1H') in UTC</li>
+ * <li>Period('P1D') in America/Los_Angeles cannot be mapped to Period('P1D') in UTC</li>
+ */
+ public boolean canBeMappedTo(PeriodGranularity target)
Review Comment:
admittedly i'm still digesting how exactly this method works, but it seems kind of expensive to do to match for every projection we consider of every segment when the timezones don't match (at least `getUtcMappablePeriodSecondsOrThrow` seems expensive). Perhaps we should make a cache of these conversions so we can re-use the work we've done since its likely going to be a lot of the same checks over and over?
##########
processing/src/main/java/org/apache/druid/java/util/common/granularity/PeriodGranularity.java:
##########
@@ -216,6 +226,139 @@ public String toString()
'}';
}
+ /**
+ * Returns true if this granularity can be mapped to the target granularity. For example:
+ * <li>Period('PT1H') in UTC can be mapped to Period('P1D') in UTC</li>
+ * <li>Period('PT1H') in America/Los_Angeles can be mapped to Period('PT1H') in UTC</li>
+ * <li>Period('P1D') in America/Los_Angeles cannot be mapped to Period('P1D') in UTC</li>
+ */
+ public boolean canBeMappedTo(PeriodGranularity target)
+ {
+ if (hasOrigin || target.hasOrigin) {
+ return false;
+ }
+
+ if (getTimeZone().equals(target.getTimeZone())) {
+ int periodMonths = period.getYears() * 12 + period.getMonths();
+ int targetMonths = target.period.getYears() * 12 + target.period.getMonths();
+ if (targetMonths == 0 && periodMonths != 0) {
+ return false;
+ }
+
+ long periodStandardSeconds = getStandardSecondsOrThrow(period.withYears(0).withMonths(0));
+ long targetStandardSeconds = getStandardSecondsOrThrow(target.period.withYears(0).withMonths(0));
+ if (targetMonths == 0 && periodMonths == 0) {
+ // both periods have zero months
+ return targetStandardSeconds % periodStandardSeconds == 0;
+ }
+ // if we reach here, targetMonths != 0
+ if (periodMonths == 0) {
+ // can map if 1.target not have week/day/hour/minute/second, and 2.period can be mapped to day
+ // this is for simplicity, it's possible some period can be mapped to gran, but we don't support it
+ return targetStandardSeconds == 0 && (3600 * 24) % periodStandardSeconds == 0;
+ } else {
+ // can map if 1.target&period not have week/day/hour/minute/second, and 2.period month can be mapped to target month
+ return targetMonths % periodMonths == 0
+ && targetStandardSeconds == 0
+ && periodStandardSeconds == 0;
+ }
+ }
+
+ // Timezone doesn't match, we'd require periods to be in whole seconds, i.e. no years, months, or milliseconds.
+ if (getStandardSeconds(period).isEmpty()) {
+ return false;
+ }
+ long standardSeconds = getStandardSecondsOrThrow(period);
+ if (standardSeconds != getUtcMappablePeriodSecondsOrThrow()) {
+ // the period cannot be mapped to UTC with the same period
+ return false;
+ }
+ if (target.period.getYears() == 0 && target.period.getMonths() == 0) {
+ return target.getUtcMappablePeriodSecondsOrThrow() % standardSeconds == 0;
+ } else {
+ return getStandardSecondsOrThrow(target.period.withYears(0).withMonths(0)) == 0
+ && (3600 * 24) % standardSeconds == 0;
+ }
+ }
+
+ /**
+ * Returns the maximum possible period seconds that this granularity can be mapped to UTC.
+ * <p>
+ * Throws {@link DruidException} if the period cannot be mapped to whole seconds, i.e. it has years or months, or milliseconds.
+ */
+ public long getUtcMappablePeriodSecondsOrThrow()
+ {
+ long periodSeconds = PeriodGranularity.getStandardSecondsOrThrow(period);
+
+ if (ISOChronology.getInstanceUTC().getZone().equals(getTimeZone())) {
+ return periodSeconds;
+ }
+ ZoneRules rules = ZoneId.of(getTimeZone().getID()).getRules();
+ Set<Integer> offsets = Stream.concat(
+ Stream.of(rules.getStandardOffset(Instant.now())),
+ rules.getTransitions()
+ .stream()
+ .filter(t -> t.getInstant().isAfter(Instant.EPOCH)) // timezone transitions before epoch are patchy
+ .map(ZoneOffsetTransition::getOffsetBefore)
+ ).map(ZoneOffset::getTotalSeconds).collect(Collectors.toSet());
+
+ if (offsets.isEmpty()) {
+ // no offsets
+ return periodSeconds;
+ }
+
+ if (offsets.stream().allMatch(o -> o % periodSeconds == 0)) {
+ // all offsets are multiples of the period
+ return periodSeconds;
+ } else if (periodSeconds % 3600 == 0 && offsets.stream().allMatch(o -> o % 3600 == 0)) {
+ // fall back to hour if period is a multiple of hour and all offsets are multiples of hour
+ return 3600;
+ } else if (periodSeconds % 60 == 0 && offsets.stream().allMatch(o -> o % 60 == 0)) {
+ // fall back to minute if period is a multiple of minute and all offsets are multiples of minute
+ return 60;
+ } else {
+ // default to second
+ return 1;
+ }
+ }
+
+ /**
+ * Returns the standard whole seconds for the given period.
+ * <p>
+ * Throws {@link DruidException} if the period cannot be mapped to whole seconds, i.e. one of the following applies:
+ * <ul>
+ * <li>it has years or months
+ * <li>it has milliseconds
+ */
+ public static Long getStandardSecondsOrThrow(Period period)
+ {
+ Optional<Long> s = getStandardSeconds(period);
+ if (s.isPresent()) {
+ return s.get();
+ } else {
+ throw DruidException.defensive("Period[%s] cannot be converted to standard whole seconds", period);
Review Comment:
this seems expensive to throw an exception, if there are multiple projections to match against a query which has a timezone, this method is going to be called for every projection we try to match of every segment
##########
processing/src/main/java/org/apache/druid/java/util/common/granularity/Granularities.java:
##########
@@ -194,14 +192,24 @@ public static ExpressionVirtualColumn toVirtualColumn(Granularity granularity, S
public static Granularity fromVirtualColumn(VirtualColumn virtualColumn)
{
if (virtualColumn instanceof ExpressionVirtualColumn) {
- final ExpressionVirtualColumn expressionVirtualColumn = (ExpressionVirtualColumn) virtualColumn;
- final Expr expr = expressionVirtualColumn.getParsedExpression().get();
- if (expr instanceof TimestampFloorExprMacro.TimestampFloorExpr) {
- final TimestampFloorExprMacro.TimestampFloorExpr gran = (TimestampFloorExprMacro.TimestampFloorExpr) expr;
- if (gran.getArg().getBindingIfIdentifier() != null) {
- return gran.getGranularity();
- }
- }
+ return fromExpr(((ExpressionVirtualColumn) virtualColumn).getParsedExpression().get());
+ }
+ return null;
+ }
+
+ @Nullable
+ private static Granularity fromExpr(Expr expr)
+ {
+ String identifier = expr.getIdentifierIfIdentifier();
+ if (identifier != null) {
+ return identifier.equals(ColumnHolder.TIME_COLUMN_NAME)
+ ? Granularities.NONE
+ : Granularities.ALL;
Review Comment:
ALL i guess because this isn't a time grouping? maybe worth a comment
##########
processing/src/main/java/org/apache/druid/data/input/impl/AggregateProjectionSpec.java:
##########
@@ -230,10 +230,15 @@ 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) {
+ if (maybeGranularity == null || maybeGranularity.equals(Granularities.ALL)) {
+ // no __time in inputs or not supported, skip
+ continue;
+ }
+ if (granularity == null) {
granularity = maybeGranularity;
timeColumnName = dimension.getName();
- } else if (granularity != null && maybeGranularity != null && maybeGranularity.isFinerThan(granularity)) {
+ } else if (maybeGranularity.isFinerThan(granularity)) {
Review Comment:
i think this check needs to make sure there is no timezone on the granularity to be considered as the time column since i think __time is assumed to be UTC and so any column on the projection being considered as the substitute __time column must also be utc. This seems worth adding a test for
##########
processing/src/main/java/org/apache/druid/segment/AggregateProjectionMetadata.java:
##########
@@ -213,16 +214,19 @@ public Schema(
int foundTimePosition = -1;
this.orderingWithTimeSubstitution = Lists.newArrayListWithCapacity(ordering.size());
- Granularity granularity = null;
+ Granularity granularity = Granularities.ALL;
for (int i = 0; i < ordering.size(); i++) {
OrderBy orderBy = ordering.get(i);
if (orderBy.getColumnName().equals(timeColumnName)) {
orderingWithTimeSubstitution.add(new OrderBy(ColumnHolder.TIME_COLUMN_NAME, orderBy.getOrder()));
+ if (foundTimePosition != -1) {
+ throw DruidException.defensive("projection[%s] has multiple time columns in ordering: %s", name, ordering);
+ }
Review Comment:
I guess this shouldn't be able to happen right now since we generate the ordering so the same entry shouldn't ever show up
##########
processing/src/main/java/org/apache/druid/java/util/common/granularity/PeriodGranularity.java:
##########
@@ -216,6 +226,139 @@ public String toString()
'}';
}
+ /**
+ * Returns true if this granularity can be mapped to the target granularity. For example:
+ * <li>Period('PT1H') in UTC can be mapped to Period('P1D') in UTC</li>
+ * <li>Period('PT1H') in America/Los_Angeles can be mapped to Period('PT1H') in UTC</li>
+ * <li>Period('P1D') in America/Los_Angeles cannot be mapped to Period('P1D') in UTC</li>
+ */
+ public boolean canBeMappedTo(PeriodGranularity target)
Review Comment:
also these methods feel like they could use some additional comments to make it clearer what is going on and why this works
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org