fix: DH-23289: Propagate shifts properly in group-by aggregations, including rollups by lbooker42 · Pull Request #8324 · deephaven/deephaven-core

lbooker42 · 2026-08-05T18:53:58Z

This branch fixes three distinct defects that surface when building a rollup over an Instant column and when a rollup sits above a table that shifts, plus the SegmentedSortedMultiSet (SSM) equality/hashing bugs those paths exposed.

Instant re-aggregation.

Since #8099, rollup re-aggregation for AggUnique reads each constituent's value column rather than its SSM, but passed that source through raw. For an Instant aggregation the child level exposes a LongAsInstantColumnSource while the selected operator is LongRollupUniqueOperator, which casts to LongChunk — so any rollup with AggUnique over an Instant column threw ClassCastException. AggregationProcessor now reinterprets the input as long while still passing the raw type to makeUniqueOperator, so operator selection and the Instant result type are unchanged. The reinterpret is deliberately Instant-only rather than the general ReinterpretUtils.maybeConvertToPrimitive, which would also convert Boolean/ZonedDateTime and hand the wrong chunk type to ObjectRollupUniqueOperator. Relatedly, InstantSsmSourceWrapper.ValueWrapper had class-identity equals/hashCode inconsistent with the ObjectVector contract — an Instant AggDistinct result was never equal to an equivalent ObjectVectorDirect, including the one its own getDirect() produced; both now delegate to the ObjectVector helpers, and it gains an O(size) iterator override.

Shift reporting through group / re-aggregate

A shift relabels row keys without changing grouped values, so GroupByChunkedOperator reported nothing — but rollups above it cache an unshifted union of the child RowSets and so silently went stale. GroupByChunkedOperator now tracks shifts separately from adds/removes/modifies and dirties only the exposed RowSet column when one exists (nothing at all when it doesn't), so consumers such as FormulaMultiColumnChunkedOperator are not forced to recompute over unchanged group vectors. GroupByReaggregateOperator mirrors this one level up: it never sees shiftChunk, so a shift below arrives as a modify, and it distinguishes "the union's contents changed" from "only its keys moved" by whether the level below dirtied its value columns, propagating either all output columns or just the RowSet column accordingly.

SSM equality and hashing

equals compared leaf layout structurally against another SSM, but two SSMs can hold identical values in different layouts (leaves need not be full, node sizes need not agree), so equal contents could compare unequal; hashCode mixed in size/leafCount and used Objects.hash, so it disagreed with the Vector hash of the same contents. All eight generated SSMs now compare element-wise against any Vector via XComparisons.eq (fixing -0.0/NaN handling for float/double) and hash exactly as XVector.hashCode does, with the leaf walk kept inline for speed and pinned against the helper by a test. Each also gains an O(size) iterator override — the inherited positional one made traversal O(size * leafCount), which matters because hashing and equality run per row per cycle when an SSM-valued column is used as an aggregation key. Both replicators were updated (fixupObjectIterator, reworked fixupObjectCompare) so the generated Object variant stays correct.

Testing

TestRollupTable had no Instant coverage at all; it gains static and incremental AggUnique tests, rollup-vs-zero-key comparisons across every aggregation that accepts an Instant column, a value-type routing test, and four directed shift tests (group and formula, covering both correctness under shift and the shift-only "reports only the RowSet column / does not recompute" assertions). The directed unique test is parameterized so one transition sequence runs for both int and Instant. The single-key incremental tests now hold the table at a fixed 10k rows and sweep the per-cycle update size up to a full replacement — previously the int variant built a 100k-row table regardless of update size, spending 32s of the class's 42s while only covering deltas up to 1% and never a multi-chunk update. New per-type SSM tests cover the iterator, hash/helper agreement, and symmetric equality, plus new float/double special-value tests for -0.0 and NaN delta tracking.

…aggregation Since deephaven#8099 the rollup re-aggregation for AggUnique reads each constituent's value column instead of its SSM, but passed that column raw. For an Instant aggregation the child level exposes its result as a LongAsInstantColumnSource, while the operator selected for Instant is LongRollupUniqueOperator, which casts the chunk to LongChunk -- so building any rollup with AggUnique over an Instant column threw ClassCastException. Reinterpret the input as long, mirroring what addBasicOperators already does at the base level, while still passing the raw type to makeUniqueOperator so it keeps selecting the Instant operator and exposing an Instant result. The reinterpret is deliberately Instant-only rather than the general ReinterpretUtils.maybeConvertToPrimitive: the latter also converts Boolean and ZonedDateTime, which route to ObjectRollupUniqueOperator and would then receive the wrong chunk type. testRollupUniqueValueTypes pins this. Also fix InstantSsmSourceWrapper.ValueWrapper's equals/hashCode, which were class-identity based and inconsistent with the ObjectVector contract, so an Instant AggDistinct result was never equal to an equivalent ObjectVectorDirect -- including the one its own getDirect() produces. Both now delegate to the ObjectVector helpers, which is what ObjectVectorDirect uses. TestRollupTable had no Instant coverage at all, on the assumption that re-aggregation is type-independent; add directed static and incremental AggUnique tests, rollup-vs-zero-key comparisons across every aggregation that accepts an Instant column, and the value-type routing test above. The directed unique test is now parameterized so one transition sequence runs for both int and Instant. The single-key incremental tests hold their table at a fixed 10k rows and sweep the per-cycle update size up to a full replacement. Previously the int one built a 100k-row table regardless of update size, spending 32s of the class's 42s to cover only deltas up to 1% and never a multi-chunk update. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions · 2026-08-05T18:54:07Z

No docs changes detected for `90ebd46`

Copilot

Pull request overview

Fixes Instant-backed AggUnique re-aggregation in rollup tables.

Changes:

Reinterprets Instant result sources as primitive long sources during unique re-aggregation.
Aligns wrapped Instant vectors’ equality and hashing with other ObjectVector implementations.
Adds extensive static and incremental rollup coverage.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
`AggregationProcessor.java`	Uses reinterpreted `long` sources for `Instant` unique re-aggregation.
`InstantSsmSourceWrapper.java`	Corrects vector equality and hash-code behavior.
`TestRollupTable.java`	Adds `Instant` rollup and unique-transition tests.

…ched union Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…size)

lbooker42 · 2026-08-05T22:34:01Z

Note for previous commit:

Measured

Baseline is a Vector with the identical get body but no iterator override, so
it pays the same per-element dispatch the pre-fix code paid. leafSize 4096.
"floor" is the same element count hashed from one contiguous long[].

distinct	leaves	SSM before	SSM after		floor	Instant before	Instant after
4,096	1	0.005ms	0.005ms	1.0×	0.005ms	0.013ms	0.013ms	1.0×
25,000	7	0.119ms	0.066ms	1.8×	0.030ms	0.353ms	0.081ms	4.3×
100,000	25	0.768ms	0.263ms	2.9×	0.121ms	2.001ms	0.330ms	6.1×
400,000	98	8.171ms	1.061ms	7.7×	0.482ms	13.102ms	1.305ms	10.0×
1,000,000	245	54.971ms	2.684ms	20.5×	1.226ms	79.166ms	3.316ms	23.9×

Monotonic, no crossover; leafCount <= 1 is exactly unchanged. The harness
asserts the hash value is identical before and after at every size.

Caching the leaf array inside the cursor is worth a further 1.30×, measured
in isolation on the same layout in one run (positional / uncached walk / cached
walk, interleaved):

distinct	leaves	positional	uncached walk	cached walk	unc/pos	cch/unc
25,000	7	0.056ms	0.042ms	0.032ms	1.3×	1.31×
100,000	25	0.507ms	0.165ms	0.126ms	3.1×	1.31×
400,000	98	7.060ms	0.661ms	0.507ms	10.7×	1.31×
1,000,000	245	53.127ms	1.657ms	1.275ms	32.1×	1.30×

rcaudy · 2026-08-06T02:20:06Z

+        final int startLeaf = firstLeaf;
+        final int startOffset = (int) firstOffset;
+
+        return new ValueIteratorOfChar() {


There is a wrinkle, here. Vectors allow access to negative offsets, or offsets after their end, returning the appropriate null value. The column source wrapper implementations use methods like io.deephaven.engine.primitive.value.iterator.ValueIteratorOfChar#wrapWithNulls to help with this.

There are probably bugs in get, subVector, and subVectorByPositions as well.

We might consider whether SSMs as vectors need to be correct in this way. If we aren't exposing them to user code, we can get away with violating the rules (and it seems we already have been).

SSM is not a well behaved Vector, will throw in these cases.

AggDistinct result column hands the live SSM to formulas, so X[-1], X.subVector(...), X.get(size()) all land on SSM code. Probably should address this, will create a ticket.

rcaudy · 2026-08-06T02:29:53Z

-        }
-
-        return result;
+        return CharVector.hashCode(this);


I'm a bit suspicious about the implementation for equals, now. compareTo is inheriting the default.

Good suspicion, the equals() paths were using equality operations (==) instead of Comparsions.eq(). So Float/Double NaN matching was broken. Still have inaccuracy in equality vs ObjectVector, but these are not new and don't seem to be used currently.

rcaudy · 2026-08-06T21:27:14Z


+        // A child RowSet changed in place. We never see shiftChunk, so a shift below arrives here as a modify that
+        // dirtied only the RowSet column; dirty value columns instead mean rows genuinely came or went below us.
+        if (someValueColumnsModified) {


The below may be interesting for context, but I think the tl;dr is that we should revert this part of the change.

I'm not sure this follows.
If any group in the level below has adds or removes, all of the output aggregated columns will be in the MCS, not just some. Additionally, the exposed row set column will be in the MCS.
So, I wonder if we should be testing for containsAll (over input agg columns and input exposed row set column) instead of containsAny and calling the boolean allInputColumnsModified. Even there, we might be over-claiming adds/removes - we can't tell from here if they were all modified because of upstream value mods + upstream shifts, or if they were all modified because of adds or removes to groups.

Maybe, then, we just want to call these:
allOutputsModified
rowSetOnlyModified

rcaudy · 2026-08-06T21:32:24Z


    private RowSetBuilderRandom stepDestinationsModified;
+    /** Did any destination's union of child RowSets actually change contents this step? */
    private boolean rowsetsModified = false;


This should be a local variable in its current usage.

rcaudy · 2026-08-06T21:48:47Z

    @Override
    public boolean hasModifications(boolean columnsModified) {
-        return columnsModified || rowsetsModified;
+        // Deliberately excludes shifts; see GroupByChunkedOperator.hasModifications.


I think this change makes sense - formulas don't care if our row sets change due to shifts.

rcaudy · 2026-08-06T21:52:51Z

+            if (someKeyHasShifts) {
+                // Our union's keys moved but its contents did not; only the exposed RowSet column observes that.
+                updateModifiedColumnSet.setAll(rowSetModifiedColumnSet);
+            }


I think this is covered by the above transform. If so, I think you don't need "someKeyHasShifts" anymore.

rcaudy · 2026-08-06T21:54:30Z

        @Override
        public ModifiedColumnSet apply(@NotNull final ModifiedColumnSet upstreamModifiedColumnSet) {
-            if (rowsetsModified) {
+            if (someKeyHasAddsOrRemoves) {


~~We don't need this either, if my proposed change to the meaning makes sense. Then again, we never did with the way the transformer was written.~~

rcaudy · 2026-08-06T22:08:23Z

+    private boolean someValueColumnsModified;
+    private boolean someKeyHasAddsOrRemoves;
+    private boolean someKeyHasShifts;


I think we only want to keep someKeyHasAddsOrRemoves.

rcaudy · 2026-08-06T22:12:34Z

            stepDestinationsModified = null;
        }
+        // Neither classification survives a step in which no union actually changed.
+        someKeyHasAddsOrRemoves &= rowsetsModified;


I think someKeyHasShifts goes away. The open question is: do we still need rowsetsModified or can we trust someKeyHasAddsOrRemoves if it's only set by addChunk and removeChunk?

rcaudy · 2026-08-07T00:35:27Z

We'll be removing the SSM work from this PR, which is being merged under #8332. We should probably write a new Jira issue to cover the group-by work that's staying in this PR.

…ashCode changes Reverts three commits, leaving the group RowSet shift fixes in place: 36a5a0c fix: DH-23256: Reinterpret Instant value source for rollup unique re-aggregation 838eddc fix: DH-23256: Fix SSM hashCode contract and make Vector iteration O(size) c93822d fix: DH-23256: Fix SSM equals to compare contents, not leaf layout Every file the three touched is restored byte-for-byte to its pre-36a5a0c00 state, except TestRollupTable, where the revert of 36a5a0c's Instant coverage had to be resolved by hand against the two kept commits. That file keeps INCREMENTAL_TABLE_SIZE and its use by testRollupMultiKeyIncrementalInternal (afcc3e4), and the four shift tests -- testRollupGroupWithUpstreamShift (afcc3e4) plus testRollupGroupShiftOnlyReportsRowSetColumn, testRollupFormulaWithUpstreamShift, and testRollupFormulaShiftOnlyDoesNotRecompute (d2ed361) -- while dropping the Instant fixtures, the Instant rollup-vs-zero-key tests, testRollupUniqueValueTypes, and testRollupUniqueInstantStatic. The RollupCompareNugget columns-array constructor goes back to the two-argument form, since only the Instant tests needed it.

lbooker42 · 2026-08-07T06:02:39Z

This PR is frozen, these changes will be moved to a new JIRA ticket to address the specific GroupBy Rollup shift issue the outstanding code is attempting to address.

lbooker42 requested a balanced review from Copilot August 5, 2026 18:54

Copilot started reviewing on behalf of lbooker42 August 5, 2026 18:54 View session

lbooker42 self-assigned this Aug 5, 2026

lbooker42 added NoDocumentationNeeded ReleaseNotesNeeded Release notes are needed labels Aug 5, 2026

Copilot AI reviewed Aug 5, 2026

View reviewed changes

rcaudy self-requested a review August 5, 2026 18:58

rcaudy reviewed Aug 5, 2026

View reviewed changes

Comment thread engine/table/src/main/java/io/deephaven/engine/table/impl/by/AggregationProcessor.java Outdated

lbooker42 and others added 2 commits August 5, 2026 13:25

fix: DH-23256: Report group RowSet shifts so rollups rebuild their ca…

afcc3e4

…ched union Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix: DH-23256: Fix SSM hashCode contract and make Vector iteration O(…

838eddc

…size)

rcaudy reviewed Aug 6, 2026

View reviewed changes

lbooker42 added 2 commits August 6, 2026 10:52

fix: DH-23256: Fix SSM equals to compare contents, not leaf layout

c93822d

perf: DH-23256: Report only the group RowSet column on shift-only cycles

d2ed361

rcaudy reviewed Aug 6, 2026

View reviewed changes

rcaudy mentioned this pull request Aug 7, 2026

fix: DH-23256: AggUnique for instant columns fix #8332

Merged

lbooker42 marked this pull request as draft August 7, 2026 06:02

rcaudy changed the title ~~fix: DH-23256: Reinterpret Instant value source for rollup unique re-aggregation~~ fix: DH-23289: Propagate shifts properly in group-by aggregations, including rollups Aug 7, 2026

Uh oh!

Conversation

lbooker42 commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Instant re-aggregation.

Shift reporting through group / re-aggregate

SSM equality and hashing

Testing

Uh oh!

github-actions Bot commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

No docs changes detected for 90ebd46

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

lbooker42 commented Aug 5, 2026

Measured

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!

Uh oh!

Uh oh!

Uh oh!

rcaudy commented Aug 7, 2026

Uh oh!

lbooker42 commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lbooker42 commented Aug 5, 2026 •

edited

Loading

github-actions Bot commented Aug 5, 2026 •

edited

Loading

No docs changes detected for `90ebd46`