feat: DH-21493: pushdown support for sorted regions by lbooker42 · Pull Request #7955 · deephaven/deephaven-core

lbooker42 · 2026-04-27T22:32:13Z

This adds support for match and range filters to push down into binary search over sorted regions.

…delegate to the ColumnRegion for predicate pushdown.

…timizations.

…nderneath Unioned table to activate Column Region optimizations.

…ents.

…21493-sorted-pushdown

…nversion.

…t. Documentation.

…ation' into nightly/DH-21522-parquettablelocation # Conflicts: # engine/table/src/main/java/io/deephaven/engine/table/impl/PushdownResult.java

…lumnSourceManager to store table definition instead of List<ColumnDefinition>

…ation' into nightly/DH-21493-sorted-pushdown # Conflicts: # engine/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedPushdownFilterLocationContext.java # extensions/parquet/table/src/main/java/io/deephaven/parquet/table/region/ParquetColumnRegionByte.java # extensions/parquet/table/src/main/java/io/deephaven/parquet/table/region/ParquetColumnRegionChar.java # extensions/parquet/table/src/main/java/io/deephaven/parquet/table/region/ParquetColumnRegionDouble.java # extensions/parquet/table/src/main/java/io/deephaven/parquet/table/region/ParquetColumnRegionFloat.java # extensions/parquet/table/src/main/java/io/deephaven/parquet/table/region/ParquetColumnRegionInt.java # extensions/parquet/table/src/main/java/io/deephaven/parquet/table/region/ParquetColumnRegionLong.java # extensions/parquet/table/src/main/java/io/deephaven/parquet/table/region/ParquetColumnRegionObject.java # extensions/parquet/table/src/main/java/io/deephaven/parquet/table/region/ParquetColumnRegionShort.java

# Conflicts: # engine/table/src/main/java/io/deephaven/engine/table/impl/BasePushdownFilterContext.java # engine/table/src/main/java/io/deephaven/engine/table/impl/BasePushdownFilterContextImpl.java # engine/table/src/main/java/io/deephaven/engine/table/impl/PushdownFilterMatcher.java # engine/table/src/main/java/io/deephaven/engine/table/impl/dataindex/DataIndexPushdownManager.java # engine/table/src/main/java/io/deephaven/engine/table/impl/locations/impl/AbstractTableLocation.java # engine/table/src/main/java/io/deephaven/engine/table/impl/sources/UnionSourceManager.java # engine/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/ColumnRegion.java # engine/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/ColumnRegionByte.java # engine/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/ColumnRegionChar.java # engine/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/ColumnRegionDouble.java # engine/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/ColumnRegionFloat.java # engine/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/ColumnRegionInt.java # engine/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/ColumnRegionLong.java # engine/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/ColumnRegionObject.java # engine/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/ColumnRegionShort.java # engine/table/src/main/java/io/deephaven/engine/table/impl/sources/regioned/RegionedPushdownAction.java # engine/table/src/test/java/io/deephaven/engine/table/impl/TestPartitionAwareSourceTableNoMocks.java # extensions/parquet/table/src/main/java/io/deephaven/parquet/table/location/ParquetTableLocation.java # extensions/parquet/table/src/main/java/io/deephaven/parquet/table/region/ParquetColumnRegionByte.java # extensions/parquet/table/src/main/java/io/deephaven/parquet/table/region/ParquetColumnRegionChar.java # extensions/parquet/table/src/main/java/io/deephaven/parquet/table/region/ParquetColumnRegionDouble.java # extensions/parquet/table/src/main/java/io/deephaven/parquet/table/region/ParquetColumnRegionFloat.java # extensions/parquet/table/src/main/java/io/deephaven/parquet/table/region/ParquetColumnRegionInt.java # extensions/parquet/table/src/main/java/io/deephaven/parquet/table/region/ParquetColumnRegionLong.java # extensions/parquet/table/src/main/java/io/deephaven/parquet/table/region/ParquetColumnRegionObject.java # extensions/parquet/table/src/main/java/io/deephaven/parquet/table/region/ParquetColumnRegionShort.java # extensions/parquet/table/src/test/java/io/deephaven/parquet/table/ParquetTableFilterTest.java # extensions/parquet/table/src/test/java/io/deephaven/parquet/table/ParquetTableReadWriteTest.java

github-actions · 2026-04-27T22:32:21Z

No docs changes detected for `d8abd52`

lbooker42 · 2026-04-27T23:08:03Z

-
        executedFilterCost = 0;
+        // Extract the effective filter and use it for populating the context.
+        final WhereFilter effectiveFilter = WhereFilterDelegating.maybeUnwrapFilter(filter);


This change is pretty significant, allowing wrapped/delegating filters to still be processed efficiently. Unfortunately, this means RowCapturingFilters row tracking is also often getting bypassed so a lot of tests had to be updated to show this.

We'll may also need to update the Core+ tests once this merges; because staging/gplus may break because of the new found transparency of the RowSetCapturingFilter. It is worth running a local pTML and unit test from Core+ so we have that ready to go if necessary.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 39 out of 39 changed files in this pull request and generated 20 comments.

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

lbooker42 · 2026-04-29T16:34:07Z

An earlier PR (#7666) merged a change to the Page Stores that forces pushdown to the sub-regions of the store. This is good in many scenarios (e.g. dictionary) but not ideal for this scenario. It results in each sorted sub-region being searched even though they might have been skipped if we looked at the page store overall.

Do we need a hint (maybe in the RegionedPushDownAction.Region) about whether we should apply the pushdown to sub-regions or to the top-level page-store. This might be the only case and it might not matter too much. We might someday have sorted subregions as part of a unsorted page-store so might this is over-optimizing?

cpwright · 2026-04-29T19:53:56Z

+    WhereFilter getWrappedFilter();
+
+    /**
+     * Returns the wrapped filter if and only if the wrapped filter is functionally identical to this filter. Filters


This needs more explanation. I think things like barriers could be considered function; so maybe something more like "if the wrapped filter accepts the same rows as this filter." Then we can say e.g. a barrier does not change the rows something accepts as an example.

cpwright · 2026-04-29T19:55:35Z


+    @Override
+    public WhereFilter maybeUnwrapFilter() {
+        if (filter instanceof WhereFilterDelegating) {


Can we use the static method we defined on filter to avoid the duplication?

cpwright · 2026-05-01T13:03:42Z

    /**
-     * Performs a binary search on a given column region to find the positions (row keys) of specified sorted keys. The
-     * method returns the RowSet containing the matched row keys.
+     * Performs a binary search on a given column region to find the positions (row keys) of specified keys. The method


So we've changed the contract to be looser (though the existing kernel didn't take advantage of this with firstKey and lastKey being set). This makes sense, because we are sorting the unboxed array; so there is no need to have the sorted requirement in the contract.

However, we are not taking full advantage of the sorting. The findStartIndexAscending is returning -1; so we have no information to update firstKey when something isn't found. If we returned an Arrays.binarySearch style position of where the thing would be we could actually even short circuit the remaining to-find values if we are past the end of the known values.

cpwright · 2026-05-01T13:06:39Z

+        final int end;
+
+        if (sortColumn.isAscending()) {
+            start = findStartIndexAscending(region, firstKey, lastKey, min, minInc);


The -1 return for not found seems at odds with our match results in the binarySearchMatch case.

If I'm right, I want to make sure we have test coverage for any bugs that would be present because of that.

cpwright · 2026-05-01T13:11:23Z

        if (sortColumn.isAscending()) {
            try (final ObjectTimsortKernel.ObjectSortKernelContext<Any> context =
                    ObjectTimsortKernel.createContext(searchValues.length)) {
                context.sort(WritableObjectChunk.writableChunkWrap(searchValues));


We're going to be changing searchValues. This should be documented or we should make a copy of it.

cpwright · 2026-05-01T16:58:27Z

+                return input.copy();
+            }
+
+            // We will use the effective filter from the context, which may bypass row tracking but provides the


Probably more important to explicate why the barriers, etc. don't matter at this level rather than our testing framework.

cpwright · 2026-05-01T17:01:07Z

+                try (final RowSet ignored = matches) {
+                    return PushdownResult.of(
+                            selection,
+                            selection.subSetByKeyRange(matches.firstRowKey(), matches.lastRowKey()),


seems strange we would use the subsetBykeyRange here and intersect above. I think I would prefer intersect; and we should hope intersect does a good job, like what happens if a region decided to be non-contiguous in its rowset [of course we shouldn't have that in selection]. So maybe it doesn't matter, but I think we should pick one.

cpwright · 2026-05-01T17:04:16Z

+
+            if (ctx.isRangeFilter() && effectiveFilter instanceof RangeFilter
+                    && ((RangeFilter) effectiveFilter).getRealFilter() instanceof SingleSidedComparableRangeFilter) {
+                final SingleSidedComparableRangeFilter rangeFilter =


Why not ComparableRangeFilter?

cpwright · 2026-05-01T17:06:33Z

+            }
+            missingValues.add(value);
+        }
+        return missingValues;


if we hit maxFailures should we throw or be otherwise unhappy?

cpwright · 2026-05-01T17:07:40Z

+                    missingValue,
+                    missingValue, false,
+                    false)) {
+                Assert.assertTrue(valuesFound.isEmpty());


This test isn't great. We should test the missing value as one side of this.

"Apple", "Carrot", "Dragonfruit"

"Banana" is missing, but you should still return something in MinMax if you had "Banana", "Eggplant"

Because you have (A, A) as your range and neither are inclusive, you could never find anything anyway.

lbooker42 added 30 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.

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

Final co-pilot suggestion.

fda03ee

Use createNull for new null regions.

4c8f2de

Minor changes from co-pilot.

49dcf57

Consolidation of repeated code.

3464b4a

More cleanup

ece9334

WIP

bcb14f0

Lots of self-review.

7358204

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

Correct test and verify coverage.

7a6c143

Final copilot PR comments addressed.

0ef8684

Changes to improve coverage.

5117682

Spotless :(

62c13dc

PT comments addressed.

c7d9fd8

Re-worked predicate pushdown costs, added config for customer adjustm…

6d61d59

…ents.

Initial commit of sorted match and range filter pushdown.

7f08ff4

Cleanup and improvement for range filtering row set intersection.

533288e

lbooker42 added 15 commits April 15, 2026 09:42

Updated pushdown costs, re-ordered and renamed.

bfa3430

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

0d63333

…21493-sorted-pushdown

Updated to match latest changes in parent repo, handle match filter i…

bb96bc9

…nversion.

Added columnLocation retrieval to column region as optional

c968f0c

Using region column location instead of the location-providing context.

ae4537d

Using region column location instead of the location-providing contex…

2cf70b4

…t. Documentation.

Cleanup and documentation improvements.

425649a

Some PR comments addressed.

7050ce3

Merge remote-tracking branch 'origin/nightly/DH-21522-parquettableloc…

76b2d7e

…ation' into nightly/DH-21522-parquettablelocation # Conflicts: # engine/table/src/main/java/io/deephaven/engine/table/impl/PushdownResult.java

Refactor RegionedPushdownAction, small fixes.

428d962

Add pushdown to sub-regions to static page stores, updated RegionedCo…

400e18f

…lumnSourceManager to store table definition instead of List<ColumnDefinition>

Cleanup, handle renames.

bce97a6

Can't binsearch when filter case-insensitive. Corrected many tests.

dcfe983

lbooker42 self-assigned this Apr 27, 2026

lbooker42 added NoDocumentationNeeded ReleaseNotesNeeded Release notes are needed labels Apr 27, 2026

Cleanup

b770d9a

lbooker42 commented Apr 27, 2026

View reviewed changes

lbooker42 requested a review from Copilot April 27, 2026 23:08

Copilot started reviewing on behalf of lbooker42 April 27, 2026 23:20 View session

Copilot AI reviewed Apr 27, 2026

View reviewed changes

lbooker42 requested a review from Copilot April 27, 2026 23:48

Copilot started reviewing on behalf of lbooker42 April 27, 2026 23:55 View session

Copilot AI reviewed Apr 28, 2026

View reviewed changes

Addressing PR comments, improving code coverage.

d8abd52

cpwright reviewed May 1, 2026

View reviewed changes

Conversation

lbooker42 commented Apr 27, 2026

Uh oh!

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

Uh oh!

No docs changes detected for d8abd52

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

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!

lbooker42 commented Apr 29, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Apr 27, 2026 •

edited

Loading

No docs changes detected for `d8abd52`