[opt](exec) Skip null maps for predicate batches without nulls - #66824
[opt](exec) Skip null maps for predicate batches without nulls#66824Mryange wants to merge 1 commit into
Conversation
### What problem does this PR solve?
Issue Number: N/A
Problem Summary: Nullable predicate columns always process their null maps even when a scanned batch is known to contain no NULL values. Track the batch null state so predicate evaluation can use the nested column and selector copying can rebuild an all-zero null map only for the nullable output.
### Release note
None
### Check List (For Author)
- Test: Unit Test added but not run
- Added BE unit tests for NO_NULLS, HAS_NULLS, and UNKNOWN states
- clang-format-16 and git diff --check passed
- Behavior changed: No
- Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Review status: changes requested for one test-coverage gap. After two full review rounds, no production correctness defect was found in the null-state optimization itself, but the added tests bypass the production state producer and therefore do not prove the new fast path or its NULL-containing fallback.
Critical checkpoints:
- Goal, scope, and performance: the change is small and focused, and the implementation avoids nullable predicate/null-map work only after a batch is proven to contain no top-level NULLs. Conservative UNKNOWN/HAS_NULLS paths preserve existing behavior.
- Data correctness and parallel paths: continuous and chunked reads, rowid fallbacks, index/count placeholders, scalar/default/virtual/complex/Variant iterators, dictionary conversion, vectorized and short-circuit predicates, selector copying, casts, limits, reverse reads, and lazy materialization were traced. No reachable false NO_NULLS, stale state, dropped NULL, or column-size mismatch was found.
- Concurrency and lifecycle: SegmentIterator state is thread-confined, initialized once, and reset for every predicate-column batch. No new locking, deadlock, static-initialization, ownership, or release concern applies.
- Configuration and compatibility: no configuration, FE-BE protocol, storage format, persistence, transaction, data-write, or rolling-upgrade behavior changes.
- Error handling and observability: existing Status propagation and scan/predicate timers remain appropriate; no new metric or log is required for this local hot path.
- Testing: the new helper tests are registered, but the commit states they were not run locally and all three inject PredicateColumnNullState directly. The live macOS check failed earlier in unrelated CMake/base integration with MAKE_TEST=OFF, so it neither compiled nor ran this test. A production-path nullable SegmentIterator test is still required.
- User focus: no additional review focus was provided.
Review completion: converged in Round 2. Both normal full-review reruns and the separate risk-focused audit returned NO_NEW_VALUABLE_FINDINGS with the accepted comment duplicate-fenced.
| SchemaSPtr _read_schema; | ||
| }; | ||
|
|
||
| TEST_F(SegmentIteratorPredicateNullStateTest, UsesNestedColumnOnlyWhenNoNullsAreKnown) { |
There was a problem hiding this comment.
Please exercise the production state producer in this test file. All three tests assign _predicate_column_null_states directly, so a regression in _read_columns_by_index—for example, a NULL-containing later chunk incorrectly remaining NO_NULLS, or every real no-NULL batch remaining conservative—would still pass even though that state controls predicate evaluation and null-map elision. Add a SegmentIterator test backed by real nullable segment data that covers no-NULL and NULL-containing batches plus continuous and rowid-fallback/discontinuous reads, and asserts returned rows/null maps for representative vectorized and short-circuit predicates.
|
run buildall |
…count probe (FE) (#66831) ### What Two independent breakages that each make **current master fail to build** — one in BE configure, one in FE compile. They share a shape: a pair of PRs that never conflict textually, merge cleanly, and only break once combined, so each PR's own pipeline was green. | | Breakage | Colliding PRs | |---|---|---| | BE | `cmake` configure aborts | #66052 moved a file, #66789 made a dangling unity-skip entry fail loud | | FE | `fe-connector-iceberg` does not compile | #66778 deleted `getCountFromSnapshot()`, #66413 added a caller for it | CI merges each PR into the latest master before building, so **every PR pipeline that picks up current master is red** on one or both. --- ## 1. BE — stale `STORAGE_UNITY_SKIP` entry for a moved file Remove the stale `STORAGE_UNITY_SKIP` entry (and its comment block) for `compaction/collection_statistics.cpp`, which no longer exists. ### Why — master configure is currently broken #66052 moved `storage/compaction/collection_statistics.{cpp,h}` to `storage/index/inverted/similarity/` (rewritten), but left behind the unity-skip entry that #66789 had added for the old path. The fail-loud validation introduced by #66789 turns a dangling skip entry into a configure-time error — which is exactly what it is designed to catch (a skip list rotting after a file move), so BE configure on current master fails immediately: ``` CMake Error at CMakeLists.txt:1002 (message): unity skip entry does not exist (renamed or moved?): .../be/src/storage/compaction/collection_statistics.cpp ``` #66826, #66824, #66819, #66820 were the first hits — same error on multiple independent agents. ### Why deletion (not a path update) is correct The old entry existed because the old `collection_statistics_test` `#include`d the `.cpp` into a second TU (unity batching would then produce a duplicate definition at link time). The rewritten file at the new location is not `#include`d by any test (`grep -rn 'collection_statistics.cpp' be/test/` is empty on master), so the new path needs no skip entry. ### Verification - Full BE build from a clean tree at master + this change (clang20 / macOS arm64, unity=ON, PCH=ON): configure passes the skip-list validation and the build compiles. (The same tree without this change fails configure with the error above.) - Timeline note: #66052's last green CI round presumably predates #66789's validation landing (2026-08-16), which is how the dangling entry slipped through. --- ## 2. FE — the metadata-only COUNT(\*) probe calls a deleted method #66778 replaced the snapshot-summary COUNT(\*) pushdown with a manifest-derived count and deleted `getCountFromSnapshot()`. `canServeMetadataOnlyCount()`, added by #66413, still calls it, so FE compilation fails: ``` [ERROR] .../connector/iceberg/IcebergScanPlanProvider.java:[505,16] cannot find symbol [ERROR] symbol: method getCountFromSnapshot(org.apache.iceberg.TableScan,org.apache.doris.connector.spi.ConnectorSession) [ERROR] location: class org.apache.doris.connector.iceberg.IcebergScanPlanProvider ``` ### Why re-express the probe instead of restoring the method Bringing `getCountFromSnapshot()` back would reintroduce precisely what #66778 removed: a query result derived from optional, writer-provided snapshot summary fields. The probe is rebuilt in #66778's own terms instead. It reuses that PR's delete gate and additionally requires the data manifests to carry aggregate row counters, so the answer is proved from the manifest list alone — `O(manifests)`, no data-file enumeration, which is what a probe running before planning can afford. Manifest lists that omit those aggregates now answer `false`, where count planning still serves them through its bounded per-file fallback. A capability probe should under-promise rather than over-promise; the planner itself is untouched. ### Verification - `mvn package` over the full FE reactor (74 modules): all green. This also confirms no second semantic break is hiding behind the first — CI's maven stops at `fe-connector-iceberg` and never reaches the rest. - `IcebergScanPlanProviderTest`: 153 tests, 0 failures, including a new case pinning that the probe follows the same delete gate as count planning. - FE checkstyle: 0 violations. ### Release note None (both fixes only restore a buildable master; no user-visible behavior change). ### Check List (For Author) - Test - [x] Regression test — not applicable: neither change alters query behavior - [x] Unit test: `IcebergScanPlanProviderTest` (153 tests, 0 failures) - [x] Manual test: full BE configure/build and full FE reactor build - Behavior changed: No - Does this need documentation: No --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Nullable predicate columns always carry their null maps through predicate evaluation and selector copying, even when a scanned batch is known to contain no NULL values. This adds unnecessary work to a common scan path. This change tracks each predicate column's batch null state, evaluates proven no-NULL batches through the nested column, and rebuilds an all-zero null map only when producing nullable output. Unknown or NULL-containing batches continue to use the existing nullable path.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)