feat: add rollup table base table spec for catalog wiring by clintropolis · Pull Request #20452 · apache/druid

clintropolis · 2026-09-29T22:37:01Z

Description

Follow-up to #19830 and #20377, this adds a rollup table BaseTableProjectionSpec, the API side companion of the internal v10 segment metadata RollupTableProjectionSchema that we store internally for rollup tables. This PR rounds stuff out so that all of the physical layouts of V10 segments can be expressed as base table specs, and also with DDL in the catalog, where a rollup table is expressed as a group by query base table projection:

CREATE TABLE "druid"."visits" (
  user_id VARCHAR,
  __time TIMESTAMP,
  total BIGINT,
  ...
  PROJECTION __base AS (
    SELECT user_id, TIME_FLOOR(__time, 'PT1H') AS __time, SUM(total) AS total
    GROUP BY 1, 2
  )
)
PARTITIONED BY DAY

FrankChen021

🟡 Changes recommended

Reviewed all 15 of 15 current changed files in the full merge-base diff, including the new rollup base-table specs and metadata, DataSchema wiring, planner translation, embedded/catalog tests, and unit/SQL tests. I also inspected the surrounding ingestion, MSQ compaction, aggregator-combining, and incremental-index selector paths.

Validation run: git diff --check 443688fea12b1fee730f38f3aef87462573b475e af48f436cbf82f5bdf01d493cc83c66eb48b38f8 passed. No builds, tests, dependency installs, or formatters were run, per the requested static-review scope.

Severity	Findings
P0	0
P1	1
P2	1
P3	0
Total	2

This is an automated review by Codex GPT-5.6-Luna(max)

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

FrankChen021 · 2026-09-30T12:40:42Z

+      final String source = sources.getColumnName(i);
+      final AggregatorFactory aggregator = aggregatorsByInternalName.get(source);
+      if (aggregator != null) {
+        aggregators.add(aggregator.withName(declared));


[P1] Use combining aggregators for rollup reaggregation

Finding: This keeps the planner's raw-ingestion factory instead of the combining factory. For an accepted body such as COUNT(*) AS rows, the new rollup base-table spec is fed directly into MSQ compaction, whose GroupBy uses dataSchema.getAggregators() on already-stored rows, so each collapsed row contributes 1 rather than its stored count. The same issue affects sketch-build aggregators and any aggregator whose input field differs from its output, silently changing results on compaction or re-rollup.

Suggestion: Make the compaction/reaggregation schema use each aggregator's combining form, or reject and normalize rollup definitions whose factory is not self-combining; add a COUNT/sketch compaction round-trip test.

FrankChen021 · 2026-09-30T12:40:42Z

+    for (DimensionSchema column : groupingColumns) {
+      names.add(column.getName());
+    }
+    for (AggregatorFactory aggregator : aggregators) {


[P2] Reject metric names that collide with the granularity carrier

Finding: The shared declared-column validation is applied only to groupingColumns, while this loop accepts an aggregator named __virtualGranularity. Once query granularity is present, the spec contains both that metric and the carrier virtual column; IncrementalIndexColumnSelectorFactory resolves virtual columns before metrics, so a metric selector is shadowed by the time-floor expression, while catalog INSERTs reject the target as computed. This lets an invalid and potentially corrupting rollup schema pass validation.

Suggestion: Reject aggregator names equal to Granularities.GRANULARITY_VIRTUAL_COLUMN_NAME when constructing the rollup spec, with catalog and ingestion regression coverage.

FrankChen021

🟡 Changes recommended

Reviewed all 15 of 15 current changed files in the full merge-base diff. For this updated_since_review pass, I first reviewed all 8 files touched by the incremental patch from af48f43 to 54d3718, then rechecked the complete current diff and surrounding ingestion, compaction, incremental-index, catalog-metadata, planner, and test paths. The current validation and tests for self-combining aggregators and the reserved granularity-carrier name address those incremental-risk areas; the remaining grouping-set correctness issue is reported inline.

Validation: git diff --check 443688fea12b1fee730f38f3aef87462573b475e 54d3718de32b58074e38b2954e99e1f07705b224 passed. No builds, tests, dependency installs, or formatters were run, per the requested static-review scope.

Severity	Findings
P0	0
P1	1
P2	0
P3	0
Total	1

This is an automated review by Codex GPT-5.6-Luna(max)

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

FrankChen021 · 2026-10-01T12:49:55Z

-          groupBy.getVirtualColumns(),
-          groupBy.getDimFilter(),
-          groupBy.getIntervals()
+      return new PlannedAggregation(


[P1] Reject grouping sets in rollup base definitions

Finding: When a __base body uses GROUPING SETS, the projection engine permits that syntax and the planned GroupByQuery retains subtotal groups, but plannedAggregation copies only dimensions and aggregators. liftRollupBaseTable then accepts the definition as an ordinary rollup spec, so ingestion groups every row by the full dimension tuple instead of materializing the subtotal rows described by the DDL body.

Suggestion: Reject nontrivial grouping subtotals on the rollup-base path, or preserve them in a representation the base-table spec can execute, and add a regression test for GROUPING SETS.

…ection-spec

capistrant

Overall seems cool. I think the way it reads in your head with the grouping column ordering --> segment ordering and use of group by by said grouping columns all checks out. @FrankChen021 seems to have snuffed out the biggest land mines in the initial impl and I see those addressed accordingly

capistrant · 2026-10-01T19:09:19Z

+    for (AggregatorFactory aggregator : aggregators) {
+      if (aggregator == null) {
+        throw InvalidInput.exception("aggregators must not contain null entries");
+      }
+      if (aggregatorsByName.put(aggregator.getName(), aggregator) != null) {
+        throw InvalidInput.exception("aggregators contains duplicate entries for column [%s]", aggregator.getName());
+      }
+    }


why find out about these invalid bits now and not at construction? just cuz all the other validation stuff has to happen at runtime since columns influences?

this is still called as part of DatasourceDefn.validate so these checks are done whenever making any catalog write, it is just that base table spec defined in the catalog need the table level column definition list to be able to validate since they are allowed to be partial declarations (column schemas are optional and only need to provide explicit dimension schemas when there is some customization that needs done during indexing) and this also ensures that the table level column lists and projection column lists agree. Doing it all here ensures that if a bad spec does make it into the catalog somehow it is still fixable (because the validation belongs to write).

capistrant · 2026-10-01T21:19:57Z

Hopefully merging latest master now that it is not completely borked will turn all that CI green

…ection-spec

FrankChen021

🟢 Approval recommended

Reviewed 15 of 15 changed files. I started with the incremental diff, separated the upstream merge content from the PR-owned changes, then rechecked the full current diff at head 5aa35a8.

Coverage included the new rollup projection/spec and catalog metadata, DataSchema and ingestion/compaction integration, SQL projection translation and grouping-set rejection, documentation, and the focused unit/integration tests. The prior review findings are resolved in the current code: rollup aggregators must be self-combining, metric names cannot collide with the granularity carrier, and GROUPING SETS/ROLLUP/CUBE definitions are rejected. No additional PR-caused correctness issues were found.

Narrow validation: git diff --check e855cd9c3316710361c7f2cf0b1b8331141c20ad..HEAD passed. Builds and test suites were not run per the static-review scope.

This is an automated review by Codex GPT-5.6-Luna(max)

gianm

I only read the user-facing docs in catalog.md. The approach looks nice to me, so the comments are about clarity of docs.

gianm · 2026-10-07T01:12:59Z

+PARTITIONED BY DAY
+```
+
+This is the default base table spec, so may be omitted unless flooring `__time` values is desired.


This reads strangely to me. The example spec does floor __time. So, is it the default or isn't it? I would guess it isn't?

gianm · 2026-10-07T01:14:11Z

+
+This is the default base table spec, so may be omitted unless flooring `__time` values is desired.
+
+A clustered table stores the rows of each segment internally partitioned by the clustering columns, which must be the


I would put this closer to the bottom, at least underneath rollup tables. Generally I expect rollup tables to be more popular than clustered tables. The order in which we present things should reflect how popular we expect them to be.

gianm · 2026-10-07T04:52:00Z

+clustered table materializes computed columns.
+
+A rollup table aggregates rows with identical grouping values into a single row at ingest time, expressed as a
+`GROUP BY` query. Grouping columns must reference declared columns in order (or the `TIME_FLOOR` of `__time`, declaring


The sentence starting with "Grouping columns must reference..." reads strangely to me. Maybe it's the fact that it's long and has both a parenthetical and a bunch of comma separated clauses. Please try rewording it. Splitting it into more than one sentence may be a good idea.

gianm · 2026-10-07T04:54:56Z

+The metric columns must be declared after the grouping columns, because the declared order is the physical order. A
+rollup body accepts the same
+[aggregation functions that are supported for rollup at ingestion time](../../multi-stage-query/concepts.md#rollup)
+and, like an aggregate projection, cannot compute expressions over aggregates. Because the same aggregators serve both


The sentence starting with "Because the same aggregators" is too long.

gianm · 2026-10-07T04:55:31Z

+rollup body accepts the same
+[aggregation functions that are supported for rollup at ingestion time](../../multi-stage-query/concepts.md#rollup)
+and, like an aggregate projection, cannot compute expressions over aggregates. Because the same aggregators serve both
+ingestion and re-aggregation of stored rows, each aggregate must combine its own output: an aggregate that does not,


I don't think "must combine its own output" will necessarily make sense to a user that reads this. It seems like more of a developer detail.

…ection-spec

FrankChen021

🟢 Approval recommended

No actionable issues found in this review. The rollup layout consistently carries grouping order, query granularity, and self-combining metrics through catalog translation and ingestion; the revised documentation matches the implemented constraints.

Reviewed 15 of 15 changed files at head 2aba135. Started with the incremental diff from 5aa35a8, separated upstream merge changes from the PR-owned documentation edits, and rechecked the full current merge-base diff, focused tests, and surrounding ingestion/compaction paths. The earlier findings remain resolved: non-self-combining aggregators and granularity-carrier metric names are rejected, and GROUPING SETS/ROLLUP/CUBE definitions are rejected.

Validation: git diff --check 3cedc8a HEAD passed. This was a static review; builds and test suites were not run.

This is an automated review by Codex GPT-5.6-Luna(max)

feat: add rollup table base table spec for catalog wiring

af48f43

github-actions Bot added Area - Documentation Area - Querying labels Sep 29, 2026

FrankChen021 reviewed Sep 30, 2026

View reviewed changes

better

54d3718

clintropolis mentioned this pull request Sep 30, 2026

feat: catalog compaction support for msq and base table spec #20379

Open

FrankChen021 reviewed Oct 1, 2026

View reviewed changes

clintropolis added 2 commits October 1, 2026 11:48

reject grouping sets

d5a0a55

Merge remote-tracking branch 'upstream/master' into rollup-table-proj…

20ad934

…ection-spec

capistrant approved these changes Oct 1, 2026

View reviewed changes

Merge remote-tracking branch 'upstream/master' into rollup-table-proj…

5aa35a8

…ection-spec

FrankChen021 reviewed Oct 2, 2026

View reviewed changes

gianm reviewed Oct 7, 2026

View reviewed changes

clintropolis added 2 commits October 8, 2026 21:25

Merge remote-tracking branch 'upstream/master' into rollup-table-proj…

c76483c

…ection-spec

adjust docs

2aba135

FrankChen021 reviewed Oct 9, 2026

View reviewed changes

clintropolis merged commit 98bbcb4 into apache:master Oct 9, 2026
27 checks passed

clintropolis deleted the rollup-table-projection-spec branch October 9, 2026 22:36

github-actions Bot added this to the 39.0.0 milestone Oct 9, 2026


		This is the default base table spec, so may be omitted unless flooring `__time` values is desired.

		A clustered table stores the rows of each segment internally partitioned by the clustering columns, which must be the

Conversation

clintropolis commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

capistrant 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!

capistrant commented Oct 1, 2026

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

gianm 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

clintropolis commented Sep 29, 2026 •

edited

Loading