feat: DH-21522: allow column region optimizations in Predicate Pushdown filtering by lbooker42 · Pull Request #7666 · deephaven/deephaven-core

lbooker42 · 2026-02-10T18:02:02Z

This PR refactors predicate pushdown filtering for regioned column sources to support both table-location and per-column-region optimizations, enabling more granular (region-level) pushdown planning and execution.

Changes:

Introduces a new RegionedPushdownAction model (Location vs Region actions) and new regioned pushdown filter context types.
Updates regioned pushdown execution to run per-region and merge results, enabling column-region pushdown participation.
Refactors ParquetTableLocation pushdown logic from an internal enum to the new action-based API and updates the engine interfaces accordingly.

Code Coverage Summary:

ImmutableConstant[Type]Source
- makePushdownFilterContext / estimatePushdownFilterCost at 100%
- pushdownFilter effectively 100% (empty selection detection not tested, might not be reachable without dedicated test)

…delegate to the ColumnRegion for predicate pushdown.

…timizations.

github-actions · 2026-02-10T18:02:16Z

No docs changes detected for `62c13dc`

Copilot

Pull request overview

This PR refactors predicate pushdown filtering for regioned column sources to support both table-location and per-column-region optimizations, enabling more granular (region-level) pushdown planning and execution.

Changes:

Introduces a new RegionedPushdownAction model (Location vs Region actions) and new regioned pushdown filter context types.
Updates regioned pushdown execution to run per-region and merge results, enabling column-region pushdown participation.
Refactors ParquetTableLocation pushdown logic from an internal enum to the new action-based API and updates the engine interfaces accordingly.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
extensions/parquet/table/src/test/java/io/deephaven/parquet/table/location/ParquetTableLocationTest.java	Removes the prior unit test that validated pushdown mode cost ordering.
extensions/parquet/table/src/main/java/io/deephaven/parquet/table/location/ParquetTableLocation.java	Implements location-level supported actions and action contexts for parquet pushdown planning/execution.
engine/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedPushdownHelper.java	Adds shared utilities for region-thread context and combining per-region pushdown results.
engine/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedPushdownFilterMatcher.java	Introduces the regioned action-based pushdown matcher API.
engine/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedPushdownFilterContext.java	Adds a regioned pushdown context carrying column definitions + rename mappings.
engine/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedPushdownFilterLocationContext.java	Extends the regioned context with access to the current table location.
engine/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedPushdownAction.java	Defines the new pushdown action abstraction (Location/Region) and related context interfaces.
engine/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedColumnSourceManager.java	Refactors manager-level pushdown scheduling/merging and exposes internals needed for region pushdown.
engine/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedColumnSourceBase.java	Refactors regioned column source pushdown to operate directly on regions with location-aware contexts.
engine/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/GenericColumnRegionBase.java	Adds default region-level pushdown orchestration combining region + location actions by cost.
engine/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/ColumnRegion.java	Makes column regions pushdown-capable via `RegionedPushdownFilterMatcher`.
engine/table/src/main/java/io/deephaven/engine/table/impl/sources/UnionSourceManager.java	Updates to use the new `BasePushdownFilterContext#filter()` accessor.
engine/table/src/main/java/io/deephaven/engine/table/impl/locations/impl/AbstractTableLocation.java	Adds default action-based pushdown planning/execution for table locations.
engine/table/src/main/java/io/deephaven/engine/table/impl/locations/TableLocation.java	Switches `TableLocation` to the new `RegionedPushdownFilterMatcher` API.
engine/table/src/main/java/io/deephaven/engine/table/impl/PushdownResult.java	Adds a new cost constant for region-level single-value optimizations.
engine/table/src/main/java/io/deephaven/engine/table/impl/PushdownFilterMatcher.java	Provides default implementations for pushdown matcher methods.
engine/table/src/main/java/io/deephaven/engine/table/impl/BasePushdownFilterContext.java	Makes the base context abstract and introduces `filter()` accessor (encapsulation change).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

...le/src/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedPushdownAction.java

...ns/parquet/table/src/main/java/io/deephaven/parquet/table/location/ParquetTableLocation.java

...c/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedColumnSourceManager.java

.../src/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedColumnSourceBase.java

…nderneath Unioned table to activate Column Region optimizations.

Copilot

Pull request overview

Copilot reviewed 64 out of 64 changed files in this pull request and generated 11 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

...rc/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedColumnSourceObject.java

...ne/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/ColumnRegionByte.java

...src/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedColumnSourceShort.java

.../src/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedColumnSourceLong.java

...e/src/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedColumnSourceInt.java

extensions/parquet/table/src/test/java/io/deephaven/parquet/table/ParquetTableFilterTest.java

engine/table/src/main/java/io/deephaven/engine/table/impl/BasePushdownFilterContext.java

...rc/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedColumnSourceDouble.java

.../src/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedColumnSourceChar.java

.../src/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedColumnSourceByte.java

engine/test-utils/src/main/java/io/deephaven/engine/testutil/filters/RowSetCapturingFilter.java

cpwright

Is the new code fully covered by the tests?

cpwright · 2026-02-25T19:47:50Z

engine/chunk/src/main/java/io/deephaven/chunk/WritableBooleanChunk.java

 import io.deephaven.chunk.attributes.Any;
 import io.deephaven.chunk.util.pools.MultiChunkPool;

+import io.deephaven.function.ArraySort;


You've added imports without any actual code changes here and in WritableBooleanChunk.

These were auto-added when I executed the replication code. I'll look deeper and try to understand how/why

engine/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/ColumnRegion.java

...ne/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/ColumnRegionChar.java

cpwright · 2026-02-25T20:02:23Z

...ne/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/ColumnRegionChar.java

+        }
+
+        @Override
+        @MustBeInvokedByOverriders


Why the MustBeInvokedByOverrides annotation?

Here is my logic, maybe it's wrong:

Might be incorrect annotation, but if we implement an override of this class I think we should include parent supported actions in our list (since they would also be supported). So a Null or Constant Char region would still be optimized.

engine/table/src/main/java/io/deephaven/engine/table/impl/BasePushdownFilterContext.java

.../src/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedColumnSourceBase.java

extensions/parquet/table/src/test/java/io/deephaven/parquet/table/ParquetTableFilterTest.java

cpwright · 2026-02-25T22:16:19Z

...c/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedColumnSourceManager.java

-                if (localColumnName.equals(filterColumnName)) {
-                    continue;
+    /**
+     * Get (or create) a map from column source to column name.


bad c/p of javadoc

is there a reason we've decided to defer this?

This and columnSourceToName are only used in filtering. We were deferring creating columnSourceToName but not columnNameToDefinition. Would like to be consistent between these two (either deferring both or neither).

...le/src/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedPushdownAction.java

Copilot

Pull request overview

Copilot reviewed 74 out of 74 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

.../main/java/io/deephaven/engine/table/impl/sources/immutable/ImmutableConstantByteSource.java

...a/io/deephaven/engine/table/impl/sources/regioned/RegionedPushdownFilterLocationContext.java

.../table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/ColumnRegionObject.java

Copilot · 2026-02-27T01:19:25Z

.../src/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedColumnSourceBase.java

+                    try {
+                        ctx.shiftedRowSet = tle.subsetAndShiftIntoLocationSpace(selection);
+                        getRegion(regionIndex).estimatePushdownFilterCost(
+                                filter,
+                                ctx.shiftedRowSet,
+                                usePrev,
+                                newCtx,
+                                jobScheduler,
+                                regionCost -> {
+                                    minCost.updateAndGet(old -> Math.min(old, regionCost));
+                                    resume.run();
+                                    newCtx.close();
+                                },
+                                nec);
+                    } catch (final Exception e) {
+                        // In the case of an exception, clean up the temporary context.
+                        newCtx.close();
+                        throw e;
+                    }


Potential resource management issue: newCtx.close() is called both in the success callback (line 156) and in the catch block (line 161). If an exception occurs after the callback is invoked but before it completes, the context could be closed twice. Consider using try-with-resources or a more explicit cleanup pattern to avoid potential double-close scenarios.

Can't do try with resources because of the thread switch. This is the best we can do, release on error (although this context current holds no resources).

.../src/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedColumnSourceBase.java

lbooker42 added 2 commits February 5, 2026 14:48

Changed RegionedColumnSourceBase to handle single column filters and …

27978f0

…delegate to the ColumnRegion for predicate pushdown.

Lots more work, allowing column region and table locatiop pushdown op…

d44e00d

…timizations.

lbooker42 marked this pull request as draft February 10, 2026 18:02

lbooker42 changed the title ~~Nightly/dh 21522 parquettablelocation~~ DH-21522: allow column region optimizations in Predicate Pushdown filtering Feb 10, 2026

lbooker42 requested a review from Copilot February 10, 2026 18:03

lbooker42 self-assigned this Feb 10, 2026

Copilot started reviewing on behalf of lbooker42 February 10, 2026 18:04 View session

lbooker42 added NoDocumentationNeeded ReleaseNotesNeeded Release notes are needed labels Feb 10, 2026

Copilot AI reviewed Feb 10, 2026

View reviewed changes

lbooker42 added 9 commits February 10, 2026 10:39

Addressed copilot PR comments.

73adb07

Improved and consolidated RowKeyAgnosticChunkSource filtering.

3b67e34

Cleanup and spotless

163f4c7

Support for ColumnRegion Constant and Null regions and tests.

557eae2

Merge branch 'main' into nightly/DH-21522-parquettablelocation

43e895d

Updated the new test to use QueryTable disables rather than burying u…

8dc65e6

…nderneath Unioned table to activate Column Region optimizations.

Update test to reflect new reality.

0450746

Coordinated changes to allow Core+ optimizations.

e5e9e1f

Merge branch 'main' into nightly/DH-21522-parquettablelocation

5afc2ae

lbooker42 changed the title ~~DH-21522: allow column region optimizations in Predicate Pushdown filtering~~ feat: DH-21522: allow column region optimizations in Predicate Pushdown filtering Feb 24, 2026

lbooker42 marked this pull request as ready for review February 24, 2026 23:56

Final co-pilot suggestion.

fda03ee

lbooker42 requested review from Copilot and cpwright February 25, 2026 00:01

Copilot started reviewing on behalf of lbooker42 February 25, 2026 00:02 View session

Copilot AI reviewed Feb 25, 2026

View reviewed changes

lbooker42 added 3 commits February 24, 2026 16:15

Use createNull for new null regions.

4c8f2de

Minor changes from co-pilot.

49dcf57

Consolidation of repeated code.

3464b4a

lbooker42 added 2 commits February 24, 2026 17:24

More cleanup

ece9334

WIP

bcb14f0

lbooker42 commented Feb 25, 2026

View reviewed changes

engine/test-utils/src/main/java/io/deephaven/engine/testutil/filters/RowSetCapturingFilter.java Outdated Show resolved Hide resolved

Lots of self-review.

7358204

cpwright reviewed Feb 25, 2026

View reviewed changes

lbooker42 added 4 commits February 26, 2026 09:12

Changes to the column region classes

d9f08a8

Rework filter contexts.

76e8df6

Correct failing test, more cleanup in the filter contexts.

9eb02f7

Revert RCF to not include ExposesChunkFilter.

f94919f

lbooker42 requested review from Copilot and cpwright February 27, 2026 01:16

Copilot started reviewing on behalf of lbooker42 February 27, 2026 01:17 View session

Copilot AI reviewed Feb 27, 2026

View reviewed changes

lbooker42 added 4 commits February 27, 2026 13:26

Correct test and verify coverage.

7a6c143

Final copilot PR comments addressed.

0ef8684

Changes to improve coverage.

5117682

Spotless :(

62c13dc

Conversation

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

Uh oh!

Uh oh!

github-actions bot commented Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

No docs changes detected for 62c13dc

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpwright left a comment

Choose a reason for hiding this comment

Uh oh!

cpwright Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

lbooker42 Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpwright Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

lbooker42 Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpwright Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

lbooker42 Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

lbooker42 Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

lbooker42 commented Feb 10, 2026 •

edited

Loading

github-actions bot commented Feb 10, 2026 •

edited

Loading

No docs changes detected for `62c13dc`