Consolidate the conversion between Granularity and VirtualColumn, and improve the mapping of granularity usage in projections. by cecemei · Pull Request #18403 · apache/druid

cecemei · 2025-08-14T06:55:55Z

Description

Consolidate the conversion between Granularity and VirtualColumn, and improve the mapping of granularity usage in projections.

Before this PR, projection has issues handling granularity with time zones, examples:

TimeseriesQuery with Pacific time hourly granularity can't use UTC hourly projection.
GroupbyQuery with Indian time hourly granularity is matched to UTC hourly projection falsely (it's 5h30m offset)

After this PR, the granularity matches as following:

TimeseriesQuery with Pacific time hourly granularity can use UTC hourly projection.
GroupbyQuery with Indian time hourly granularity can't use UTC hourly projection.

This is done through:

VirtualColumn -> Granularity should be able to handle all TimestampFloorExpr regardless of the time zone.
Granularity -> VirtualColumn should always be able to convert PeriodGranularity to TimestampFloorExpr, regardless of the time zone.
PeriodGranularity has a new canBeMappedTo function, in a simple way, a finer gran can be mapped to a coarser gran. It also considers timezone, origin, month/year, week, day, hour compatible stuff. E.x.
- same zone hour can be mapped to day, 2 hour can be mapped to 4 hour, but can not be mapped to 3 hour.
- different zone sometimes hour can also be mapped to day, like pacific time, but indian time can not
- week can not be mapped to month, we simplify this by not allowing week/day/hour period if there's month/year
query will almost always have a __gran VirtualColumn, unless Granularities.ALL.
projections would check its __gran canBeMappedTo to query __gan, this is more restrictive than isFinerThan.

Additional restrictions on projection granularity:

it has to be in UTC time zone and null origin. This is consistent with the __time column. Without this requirement, we can't simply map timeColumnName in projection to __time.

Key changed/added classes in this PR

Projections
Granularities
PeriodGranularity
PeriodGranularityTest
CursorFactoryProjectionTest

This PR has:

clintropolis · 2025-08-20T17:19:04Z

+   * <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)


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?

also these methods feel like they could use some additional comments to make it clearer what is going on and why this works

added some additional comments. also added PERIOD_GRAN_CACHE in Projections.java.

clintropolis · 2025-09-09T06:10:52Z

+            // 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()
+            );


I don't think this needs to be an exception, we just need to not consider it as a time column, the same way as a coarser granularity would not be __time. These columns can still be substituted for query virtual columns, so there is value in being able to pre-compute them, they just cannot serve as a replacement for __time (or time floor on __time) in queries.

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.

actually realized non-UTC vc would just be treated as regular grouping column, for PT2H and PT3H, it'd just choose the finer one PT2H.

clintropolis · 2025-09-09T06:13:46Z

+    // determine the granularity and time column name for the projection, based on the first time-like grouping column.
+    // if there are multiple time-like grouping columns, they must be "coarser" than the first time-like grouping column.


this is too restrictive, the __time column does not need to be first, we just need to replace any candidate __time column with the finer granularity column, since what we are looking for is the finest granularity column we can use as a substitute for __time in any queries against the base table

finer is not completely safe, we're looking for the most compatible gran, e.x. between PT2H and PT3H, we can find PT1H as compatible (but this is not currently supported, i'm just using this as an example).

and deciding on the most compatible is tricky since we don't want non-UTC time to sneak in, e.x. PT30M in Pacific time would sort of invalidate our PT1H gran in UTC, handling this just makes things more complicated, thus choose to handle in a strict way here

actually the compatible is PT6H. i realized projection need to find coarse ones as compatible

updated this logic to handle the gran, it should never throw exception now, skips non-utc and non-mappable vc

this comment is stale now since it no longer requires time to be first and additional time groupings to be coarser

updated comment

clintropolis · 2025-09-09T06:24:32Z

                             .addReferencedPhysicalColumn(ColumnHolder.TIME_COLUMN_NAME);
+        } else if (virtualGranularity.equals(Granularities.NONE)
+                   || projection.getEffectiveGranularity().equals(Granularities.ALL)) {
+          return null;


what is this check for none and all separately for? Are there other query virtual column granularities that are ok with an all granularity projection?

if query granularity is NONE, and projection gran is not (would already been caught by the first if statement), we just can't map, e.x. format(__time, 'YYYY-MM-DD HH:mm:ss') query can't use hourly projection. This exits early.

but i also reformatted this part to be more readable, so return null should be at bottom now

i should be more specific, previous this is using a simple isFinerThan comparison, which is not safe, because PT2H is finer than PT3H but the former can't be mapped to the latter. now we're switching to canBeMappedTo, but it only applies to period gran, so adding this special handling for Granularities.NONE and Granularities.ALL.

clintropolis · 2025-09-09T06:25:10Z


 public class Projections
 {
+  private static final Map<byte[], Boolean> PERIOD_GRAN_CACHE = new HashMap<>();


this needs to be a concurrent map or use a lock or something

updated to use ConcurrentHashMap, no lock should be fine since we're only using computeIfAbsent

clintropolis

overall lgtm 👍

clintropolis · 2025-09-11T18:26:12Z

+    // determine the granularity and time column name for the projection, based on the first time-like grouping column.
+    // if there are multiple time-like grouping columns, they must be "coarser" than the first time-like grouping column.


this comment is stale now since it no longer requires time to be first and additional time groupings to be coarser

clintropolis · 2025-09-11T18:31:57Z

+          PeriodGranularity projectionGran = (PeriodGranularity) projection.getEffectiveGranularity();
+          byte[] combinedKey = StringUtils.toUtf8(projectionGran + "->" + virtualGran);


it might be better to use getCacheKey() from the granularities instead of toString conversion, this whole bit could be

byte[] combinedKey = new CacheKeyBuilder(0).appendCacheable(projectionGran).appendCacheable(virtualGran).build()

some

4841a88

github-actions Bot added the Area - Segment Format and Ser/De label Aug 14, 2025

cecemei added 5 commits August 14, 2025 03:02

style

3d11506

more-gran

1e752ea

better-gran

e94520d

forbidden

2eeda26

revert-expr

b56df8b

cecemei marked this pull request as ready for review August 14, 2025 23:08

cecemei changed the title ~~some changes on gran and expr and projection~~ Consolidate the conversion between Granularity and VirtualColumn, and improve the mapping of granularity usage in projections. Aug 14, 2025

cecemei requested a review from clintropolis August 14, 2025 23:09

Merge branch 'master' into gran

21b1c90

clintropolis reviewed Aug 20, 2025

View reviewed changes

cecemei added 8 commits September 4, 2025 13:59

review-comment

f540831

Merge branch 'master' into gran

8073537

more-gran

9cb7336

gran-with-cache

9047a1a

Merge branch 'master' into gran

93f0d93

fix-build

d23a46f

fix-test

e12070e

format

4bdc5b9

cecemei requested a review from clintropolis September 9, 2025 00:10

clintropolis reviewed Sep 9, 2025

View reviewed changes

cecemei added 4 commits September 9, 2025 18:49

format

302ce7f

Merge branch 'master' into gran

dc75548

concurrent

c4d64e0

gran

3a9c8a9

cecemei requested a review from clintropolis September 10, 2025 05:05

cecemei added 2 commits September 10, 2025 08:44

finer-gran

962f7c5

format

fdb69c7

clintropolis reviewed Sep 11, 2025

View reviewed changes

comment

16e23b5

cecemei requested a review from clintropolis September 12, 2025 00:13

clintropolis approved these changes Sep 12, 2025

View reviewed changes

cecemei merged commit f55de8f into apache:master Sep 12, 2025
62 checks passed

cecemei added this to the 35.0.0 milestone Oct 21, 2025

		// determine the granularity and time column name for the projection, based on the first time-like grouping column.
		// if there are multiple time-like grouping columns, they must be "coarser" than the first time-like grouping column.

		PeriodGranularity projectionGran = (PeriodGranularity) projection.getEffectiveGranularity();
		byte[] combinedKey = StringUtils.toUtf8(projectionGran + "->" + virtualGran);

Conversation

cecemei commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Key changed/added classes in this PR

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cecemei Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cecemei commented Aug 14, 2025 •

edited

Loading

cecemei Sep 10, 2025 •

edited

Loading