fix: Fix PartitioningColumnDataIndex: region RowSet provenance and in…

…dex RowSet tracking (deephaven#5942)
rcaudy · Aug 15, 2024 · 4d0b82f · 4d0b82f
1 parent 57d0188
commit 4d0b82f
Show file tree

Hide file tree

Showing 2 changed files with 11 additions and 7 deletions.
diff --git a/...ain/java/io/deephaven/engine/table/impl/sources/regioned/PartitioningColumnDataIndex.java b/...ain/java/io/deephaven/engine/table/impl/sources/regioned/PartitioningColumnDataIndex.java
@@ -216,7 +216,7 @@ private void handleKey(
             indexKeySource.set(addedKeyPos, locationKey);
 
             indexRowSetSource.ensureCapacity(addedKeyPos + 1);
-            indexRowSetSource.set(addedKeyPos, regionRowSet.shift(regionFirstRowKey));
+            indexRowSetSource.set(addedKeyPos, regionRowSet.shift(regionFirstRowKey).toTracking());
         } else {
             // noinspection DataFlowIssue
             final WritableRowSet existingRowSet = indexRowSetSource.get(pos).writableCast();

diff --git a/...ain/java/io/deephaven/engine/table/impl/sources/regioned/RegionedColumnSourceManager.java b/...ain/java/io/deephaven/engine/table/impl/sources/regioned/RegionedColumnSourceManager.java
@@ -291,11 +291,10 @@ private WritableRowSet update(final boolean initializing) {
             if (entry.pollUpdates(addedRowSetBuilder)) {
                 // Changes were detected, update the row set in the table and mark the row/column as modified.
                 /*
-                 * Since TableLocationState.getRowSet() returns a copy(), we should consider adding an UpdateCommitter
-                 * to close() the previous row sets for modified locations. This is not important for current
-                 * implementations, since they always allocate new, flat RowSets.
+                 * Since TableLocationState.getRowSet() returns a copy(), we own entry.rowSetAtLastUpdate and can
+                 * propagate it without making another copy().
                  */
-                rowSetSource.set(entry.regionIndex, entry.location.getRowSet());
+                rowSetSource.set(entry.regionIndex, entry.rowSetAtLastUpdate);
                 if (modifiedRegionBuilder != null) {
                     modifiedRegionBuilder.appendKey(entry.regionIndex);
                 }
@@ -346,7 +345,7 @@ private WritableRowSet update(final boolean initializing) {
                                 wcs.set(entry.regionIndex, entry.location.getKey().getPartitionValue(key)));
                 // @formatter:on
                 locationSource.set(entry.regionIndex, entry.location);
-                rowSetSource.set(entry.regionIndex, entry.location.getRowSet());
+                rowSetSource.set(entry.regionIndex, entry.rowSetAtLastUpdate);
             });
         }
 
@@ -574,7 +573,12 @@ private boolean pollUpdates(final RowSetBuilderSequential addedRowSetBuilder) {
                             .appendRange(regionFirstKey + subRegionFirstKey, regionFirstKey + subRegionLastKey));
                 }
             } finally {
-                rowSetAtLastUpdate.close();
+                /*
+                 * Since we record rowSetAtLastUpdate in the RowSet column of our includedLocationsTable, we must not
+                 * close() the old rowSetAtLastUpdate here. We should instead consider adding an UpdateCommitter to
+                 * close() the previous RowSets for modified locations, but this is not important for current
+                 * implementations since they always allocate new, flat RowSets.
+                 */
                 rowSetAtLastUpdate = updateRowSet;
             }
             // There was a change to the row set.