Repository navigation
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review conclusion: request changes.
The PR correctly fixes the contiguous point-lookup accounting case and safely removes the dead runtime-predicate counter, but the new min/max seed is only a dense envelope. With disjoint key ranges or sparse cache/index survivors, RowsStatsFiltered and RowsBloomFilterFiltered still charge rows already removed by the earlier bitmap. The final row bitmap/output remains correct, so this is a profile-accounting correctness issue rather than a row-result issue.
Critical checkpoints:
- Goal: partially achieved; contiguous point lookup is fixed, but sparse/disjoint candidates are not accounted exactly.
- Scope/focus: the change is focused; no additional user focus was supplied.
- Concurrency/lifecycle: no new threads, shared mutable state, lock ordering, or special lifecycle were introduced.
- Configuration/compatibility: no config, FE-BE protocol, storage-format, transaction, or rolling-upgrade change. The removed profile counter had no live repository consumers and was always zero from the duplicated intersection.
- Parallel paths: segment-level and expression zone-map counters remain wired; the remaining issue is the per-segment condition-range path.
- Error/memory/data correctness: no new unchecked Status, allocation, delete-bitmap, version, or output-row defect was found.
- Tests: the added BE and regression tests cover a contiguous point lookup and full-scan zone-map pruning, but not disjoint key/cache/index survivor sets; the regression test also has a trailing DROP TABLE contrary to the repository debugging convention.
- Validation: static review only; builds and tests were not run per the review prompt.
Please address the inline sparse-accounting and regression-test cleanup comments, then add a disjoint-range counter test before re-review.
Member
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
mrhhsg
changed the title
mrhhsg
force-pushed
the
fix/scan-stats-counted-after-key-range
branch
from
September 22, 2026 02:12
6025c81 to
27896b5
Compare
mrhhsg
added a commit
to mrhhsg/doris
that referenced
this pull request
Sep 22, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#68279 Problem Summary: Correct filtered-row counters alone do not avoid page-index work on rows already removed by key ranges or other indexes. On an eight-page segment, an instrumented point lookup still evaluated all eight ZoneMaps. Select candidate pages from the surviving bitmap and current row ranges using each column's ordinal index, and share that selection across ordinary ZoneMap, expression ZoneMap and Bloom-filter pruning. Carry reduced ranges across columns, preserve synthetic readers without physical pages, and retain exact sparse-row accounting. The point/disjoint-key tests now evaluate one/two pages respectively. ### Release note Skip redundant ZoneMap and Bloom-filter page evaluations after key-range/index pruning and expose the actual page-index evaluation counts in query profiles. ### Check List (For Author) - Test: - Unit Test: 269 focused ASAN BE tests passed, including candidate-page oracle, sparse/empty rows, expression flags, corrupt metadata and synthetic readers. - Regression test: point_lookup_zone_map_profile (generated output and normal rerun), test_expr_zonemap_pruning, test_zone_map_delete, test_bloom_filter_is_not_null passed on an isolated local cluster. - Build: ./build.sh --be --fe -j24 (ASAN). - Formatting, build hygiene and git diff --check passed. - Static analysis: see PR body for existing base-header/toolchain blockers and the local-only validation overlay (zero changed-line diagnostics). No RELEASE timing benchmark. - Behavior changed: Yes (same query results and storage format; less page-index work and new profile counters). - Does this need documentation: No
Member
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Member
Author
Local pipeline review — ✅ PASSschema: doris-repo-review/v1
status: PASS
pr: apache/doris#68279
commit: c92b9b836e07a2691de61575d93a50be3c7eb20c
base: 14c957e048bfb3617f8cb23bc39db712089fb98f
reviewed_at: 2026-09-22T12:07+08:00
reviewer: mrhhsg
model: gpt-6-astra
effort: xhigh
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: trueNotes for maintainers
Reviewed locally with the |
mrhhsg
added a commit
to mrhhsg/doris
that referenced
this pull request
Sep 24, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#68279 Problem Summary: Correct filtered-row counters alone do not avoid page-index work on rows already removed by key ranges or other indexes. On an eight-page segment, an instrumented point lookup still evaluated all eight ZoneMaps. Select candidate pages from the surviving bitmap and current row ranges using each column's ordinal index, and share that selection across ordinary ZoneMap, expression ZoneMap and Bloom-filter pruning. Carry reduced ranges across columns, preserve synthetic readers without physical pages, and retain exact sparse-row accounting. The point/disjoint-key tests now evaluate one/two pages respectively. ### Release note Skip redundant ZoneMap and Bloom-filter page evaluations after key-range/index pruning and expose the actual page-index evaluation counts in query profiles. ### Check List (For Author) - Test: - Unit Test: 269 focused ASAN BE tests passed, including candidate-page oracle, sparse/empty rows, expression flags, corrupt metadata and synthetic readers. - Regression test: point_lookup_zone_map_profile (generated output and normal rerun), test_expr_zonemap_pruning, test_zone_map_delete, test_bloom_filter_is_not_null passed on an isolated local cluster. - Build: ./build.sh --be --fe -j24 (ASAN). - Formatting, build hygiene and git diff --check passed. - Static analysis: see PR body for existing base-header/toolchain blockers and the local-only validation overlay (zero changed-line diagnostics). No RELEASE timing benchmark. - Behavior changed: Yes (same query results and storage format; less page-index work and new profile counters). - Does this need documentation: No Claude-Session: https://claude.ai/code/session_018ZrdaZUYBhBJtmsmJwzxkQ
mrhhsg
force-pushed
the
fix/scan-stats-counted-after-key-range
branch
from
September 24, 2026 05:20
c92b9b8 to
3000ffc
Compare
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29115 ms |
Contributor
TPC-DS: Total hot run time: 151489 ms |
Contributor
ClickBench: Total hot run time: 24.59 s |
mrhhsg
force-pushed
the
fix/scan-stats-counted-after-key-range
branch
from
October 18, 2026 14:54
3000ffc to
725f2f6
Compare
mrhhsg
added a commit
to mrhhsg/doris
that referenced
this pull request
Oct 18, 2026
Issue Number: None Related PR: apache#68279 Problem Summary: Correct filtered-row counters alone do not avoid page-index work on rows already removed by key ranges or other indexes. On an eight-page segment, an instrumented point lookup still evaluated all eight ZoneMaps. Select candidate pages from the surviving bitmap and current row ranges using each column's ordinal index, and share that selection across ordinary ZoneMap, expression ZoneMap and Bloom-filter pruning. Carry reduced ranges across columns, preserve synthetic readers without physical pages, and retain exact sparse-row accounting. The point/disjoint-key tests now evaluate one/two pages respectively. Skip redundant ZoneMap and Bloom-filter page evaluations after key-range/index pruning and expose the actual page-index evaluation counts in query profiles. - Test: - Unit Test: 269 focused ASAN BE tests passed, including candidate-page oracle, sparse/empty rows, expression flags, corrupt metadata and synthetic readers. - Regression test: point_lookup_zone_map_profile (generated output and normal rerun), test_expr_zonemap_pruning, test_zone_map_delete, test_bloom_filter_is_not_null passed on an isolated local cluster. - Build: ./build.sh --be --fe -j24 (ASAN). - Formatting, build hygiene and git diff --check passed. - Static analysis: see PR body for existing base-header/toolchain blockers and the local-only validation overlay (zero changed-line diagnostics). No RELEASE timing benchmark. - Behavior changed: Yes (same query results and storage format; less page-index work and new profile counters). - Does this need documentation: No Claude-Session: https://claude.ai/code/session_018ZrdaZUYBhBJtmsmJwzxkQ
mrhhsg
added a commit
to mrhhsg/doris
that referenced
this pull request
Oct 18, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#68279 Problem Summary: Rebasing the page-index pruning change onto the current master required two test adaptations. `StorageReadOptions` now requires the statistics reference in its constructor instead of a default-constructed object, and `Segment::get_column_reader` was replaced by `get_column_reader_for_pruning`. The stub `ColumnIterator` in the candidate-pushdown test also follows the new `get_row_ranges_by_zone_map` signature that receives the surviving row bitmap. ### Release note None ### Check List (For Author) - Test: Unit Test - Behavior changed: No - Does this need documentation: No
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed all 15 changed paths at 725f2f6f97363329234a734bbd06c50abe825315 using the supplied aggregate diff. No new substantiated findings; no previously reported blocking finding remains applicable. Review converged in 1 round: both complete-scope reviewers and the additional sparse-integration reviewer returned NO_NEW_VALUABLE_FINDINGS, and all candidate/risk checks have evidence-backed dispositions.
Critical checkpoint conclusions:
- Goal and proof: candidate selection intersects the surviving bitmap with current ranges using each column's ordinal index. Ordinary ZoneMap, expression ZoneMap and Bloom filtering retain conservative page ranges and intersect them into the original bitmap. Alive-row cardinality prevents sparse gaps from inflating filtered-row counters. Unit tests instrument actual predicate calls and compare selection with an independent full-page oracle; SQL/profile tests cover the intended behavior. These tests were inspected, not executed here.
- Scope and reuse: the shared selector covers all three index paths, and the changes stay focused on pruning, profile statistics, API forwarding and tests.
- Concurrency and locking: traversal state is local; shared physical indexes retain existing call-once loading. Scanner statistics retain reader ownership and publish once through the existing close guard into atomic profile counters. No new lock order or blocking work under locks is introduced.
- Lifecycle and memory: readers and temporary iterators/vectors retain existing ownership and Status-exit cleanup. Variant forwarding preserves its reader owner. No new global/static initialization dependency or persistent owner cycle was found.
- Configuration: no configuration item is added. The existing expression-filter option still preserves input ranges when disabled.
- Compatibility: these are internal C++ API updates, with all in-tree callers and overrides updated. No storage-format, wire-protocol, or shipped dependency change is introduced; query result semantics are preserved by the intersection invariant. The unused runtime-predicate profile counter is removed and two page-evaluation counters are added.
- Thrift/protobuf: no generated schema or field/enum ID changes.
- Parallel paths: ordinary, expression and Bloom paths are covered, including constant readers, sparse Variant fallback and complex-column no-op paths. Shared cloud/local scanning uses the same implementation. Synthetic readers skip physical index access; this checkout's nested-group provider cannot populate flattened child readers.
- Conditional/error paths: empty candidates skip work; unsupported/disabled expression cases preserve their fallback. ZoneMap/ordinal count mismatch fails with Corruption before indexing. Changed load/parse calls propagate Status failures.
- Test coverage: point, disjoint, same-page and empty candidates, rejection, cross-column narrowing, expression enable/disable, synthetic fallback, corrupted metadata and randomized selection are covered in source. Additional cases for disjoint/rejecting Bloom filters and deliberately different column page layouts would strengthen regression coverage; the shared selector and subset-intersection logic were independently traced for those mechanisms.
- Expected results: all nine SQL golden sections agree with the deterministic queries, including the ordered disjoint-key projection. ProfileAction waits for complete profiles and requested counter names. Both tables are dropped before creation and preserved afterward. Golden-file generation provenance was not reproduced.
- Observability: new ZoneMap/Bloom page counters are initialized, incremented on the relevant candidate-page work and collected into the scan profile. Existing expression rejection counters remain available; no heavy INFO logging is added.
- Transactions and persistence: read pruning does not add writes, metadata mutations or EditLog changes. Existing version/delete restrictions remain ahead of candidate filtering.
- Data modification/atomicity: no write or commit path changes; no new crash-recovery operation or atomicity requirement.
- FE-BE variables: none added.
- Performance: sparse selection skips bitmap gaps instead of enumerating surviving rows or visiting every page; ascending page IDs avoid repeated range unions. Whole-index ZoneMap metadata loading remains. Author-reported build, test and benchmark results were not independently reproduced, and pruning microbenchmarks are not treated as end-to-end query speedups.
- Other issues: final checks of forward/reverse scans, count-shaped index bitmaps, delete predicates, synthetic readers, index alignment, removed counter references and updated virtual APIs found no additional substantiated issue.
Existing review context: discussion_r4057083753's sparse counter overcount is fixed by exact alive-row deltas and covered by DisjointKeyRangesOnlyCountAliveRows; discussion_r4057083754's trailing DROP is removed. Neither is reposted. The user focus file contains no additional requested focus.
Validation limits: static review only, as required by the review contract. No builds, tests, benchmarks, release-binary ABI audit or source edits were performed. External implementations absent from this checkout were not assessed. No unresolved review candidate or uncovered changed path remains.
Review converged after 1 round(s).
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29380 ms |
Member
Author
|
Re-checked the full review history at HEAD Previous inline findings — fixed and resolved:
Other top-level comments: the The current Validation at this HEAD (local): Could a maintainer take another look and clear the change request if the remaining concerns are satisfied? |
Contributor
TPC-DS: Total hot run time: 151961 ms |
Contributor
ClickBench: Total hot run time: 24.55 s |
Member
Author
|
Re-checked the full review history at HEAD
Validation at this HEAD (local): Could a maintainer take another look and clear the change request if the remaining concerns are satisfied? |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…ap filtered
Issue Number: None
Problem Summary: `SegmentIterator::_get_row_ranges_by_column_conditions()` builds
the condition row ranges from the whole segment, although `_row_bitmap` at that
point already carries the key-range and index pruning. The zone map / bloom
filter counters are therefore computed against rows that were pruned long
before. A point lookup shows it clearly: on a 200k row table,
`SELECT COUNT(*) FROM t WHERE k = 12345 AND v = 12345` reports
```
RowsKeyRangeFiltered: 199.999K (199999) -- one row left
RowsStatsFiltered: 183.616K (183616) -- but the zone map "filtered" this many
RowsConditionsFiltered: 0
```
Start the condition ranges from the rows that are still alive, so the counters
only report what the index really pruned; the same query now reports
`RowsStatsFiltered: 0` while a query without a key range still reports the full
`RowsStatsFiltered: 183616`. The final row bitmap is unchanged, since it is
intersected with the condition ranges either way.
The same block also intersected the condition ranges with the zone map ranges
twice and reported the (always empty) difference of the second, idempotent
intersection as `RowsZoneMapRuntimePredicateFiltered`. That counter lost its
source when the top-n filter was turned into a regular pushed-down conjunct in
keeping a profile row that can only ever be 0.
None
- Test:
- Unit Test: Yes, `SegmentIteratorKeyRangeStatsTest` covers a point lookup
whose key range already selects a single row (no rows counted as zone-map
filtered) and the same predicate without a key range (zone map still
prunes pages).
- Regression test: Yes, `point_lookup_zone_map_profile` asserts the counters
of the profile of such a point lookup.
- Behavior changed: Yes, profile only. `RowsStatsFiltered` / `RowsBloomFilterFiltered`
no longer count rows that the key range or an index already removed, and the
`RowsZoneMapRuntimePredicateFiltered` counter is removed.
- Does this need documentation: No
Claude-Session: https://claude.ai/code/session_01RZ36Pij3o8fGnYYg33PKnq
The min/max envelope of `_row_bitmap` is not the set of surviving rows: disjoint key ranges or sparse index results (e.g. `k IN (1, 5000)`) still charged the rows between them as zone-map / bloom filter filtered. Keep building the condition ranges from the whole segment, and instead count for each step only the removed rows that are still set in `_row_bitmap`. Add a disjoint key range test, and keep the regression table after the run. Claude-Session: https://claude.ai/code/session_01TcCqRsfwhu58PN1zNar5rD
Issue Number: None Related PR: apache#68279 Problem Summary: Correct filtered-row counters alone do not avoid page-index work on rows already removed by key ranges or other indexes. On an eight-page segment, an instrumented point lookup still evaluated all eight ZoneMaps. Select candidate pages from the surviving bitmap and current row ranges using each column's ordinal index, and share that selection across ordinary ZoneMap, expression ZoneMap and Bloom-filter pruning. Carry reduced ranges across columns, preserve synthetic readers without physical pages, and retain exact sparse-row accounting. The point/disjoint-key tests now evaluate one/two pages respectively. Skip redundant ZoneMap and Bloom-filter page evaluations after key-range/index pruning and expose the actual page-index evaluation counts in query profiles. - Test: - Unit Test: 269 focused ASAN BE tests passed, including candidate-page oracle, sparse/empty rows, expression flags, corrupt metadata and synthetic readers. - Regression test: point_lookup_zone_map_profile (generated output and normal rerun), test_expr_zonemap_pruning, test_zone_map_delete, test_bloom_filter_is_not_null passed on an isolated local cluster. - Build: ./build.sh --be --fe -j24 (ASAN). - Formatting, build hygiene and git diff --check passed. - Static analysis: see PR body for existing base-header/toolchain blockers and the local-only validation overlay (zero changed-line diagnostics). No RELEASE timing benchmark. - Behavior changed: Yes (same query results and storage format; less page-index work and new profile counters). - Does this need documentation: No Claude-Session: https://claude.ai/code/session_018ZrdaZUYBhBJtmsmJwzxkQ
### What problem does this PR solve? Issue Number: None Related PR: apache#68279 Problem Summary: Rebasing the page-index pruning change onto the current master required two test adaptations. `StorageReadOptions` now requires the statistics reference in its constructor instead of a default-constructed object, and `Segment::get_column_reader` was replaced by `get_column_reader_for_pruning`. The stub `ColumnIterator` in the candidate-pushdown test also follows the new `get_row_ranges_by_zone_map` signature that receives the surviving row bitmap. ### Release note None ### Check List (For Author) - Test: Unit Test - Behavior changed: No - Does this need documentation: No
…ndex pruning ### What problem does this PR solve? Issue Number: None Related PR: apache#68279 Problem Summary: The Bloom filter and zone map counters now charge only the rows that are still alive when each pruning step runs. In test_variant_predefine_types_with_indexes_profile the inverted indexes consume the probe predicates first, so by the time the Bloom filter runs there is no surviving row left for it to reject. The `bloom_filter_must_filter_data` debug point therefore fails with "Bloom filter did not filter the data", as seen in the non-concurrent regression pipeline. Disable the inverted index inside the Bloom filter probes so the filter is exercised on the whole segment. When the index is not applied the predicate stays in col_id_to_predicates and the Bloom filter still evaluates it; query results and all .out baselines are unchanged. ### Release note None ### Check List (For Author) - Test: Regression test - variant_p0.predefine.test_types_with_indexes_profile passed on a local cluster; without the change it reproduces the pipeline failure with "Bloom filter did not filter the data" - fault_injection_p0.test_variant_bloom_filter, fault_injection_p0.test_variant_typed_path_bloom_filter and query_profile.point_lookup_zone_map_profile passed on the same cluster - Behavior changed: No - Does this need documentation: No
…serting ### What problem does this PR solve? Issue Number: None Related PR: apache#68279 Problem Summary: point_lookup_zone_map_profile assumes the whole fixture is one segment, so the key range is applied before the zone map can prune anything. An INSERT ... SELECT may create several segments depending on sink parallelism: the non-concurrent pipeline produced 24 segments, the segment-level zone map in Segment::new_iterator pruned 23 of them and charged their rows to RowsStatsFiltered before the per-segment key range ran. The assertion that a point lookup must report RowsStatsFiltered == 0 therefore failed (RowsKeyRangeFiltered=36575, RowsStatsFiltered=163424). Run a full compaction after the insert so the fixture deterministically has one segment. The key range then narrows the scan to one row and the zone map has no surviving row to charge. Verified locally: a 24-segment fixture reproduces the pipeline failure (RowsStatsFiltered=191666), and the same fixture passes after the compaction; the plain fixture still passes as before. ### Release note None ### Check List (For Author) - Test: Regression test - query_profile.point_lookup_zone_map_profile passed on a local cluster with the compacted fixture - a 24-segment fixture reproduces the non-concurrent pipeline assertion failure without the compaction and passes with it - Behavior changed: No - Does this need documentation: No
…lookup ### What problem does this PR solve? Issue Number: None Related PR: apache#68279 Problem Summary: point_lookup_zone_map_profile asserts RowsStatsFiltered == 0 after a point lookup, which only holds when the tablet holds a single segment: with several segments the segment-level zone map charges whole segments before the per-segment key range runs. The previous fixture relied on a full compaction to establish that layout, but FullCompaction::pick_rowsets_to_compact skips the initial empty [0-1] rowset plus a single data rowset regardless of how many segments that rowset has. trigger_and_wait_compaction also treats a rejected compaction's failure-time update as completion, so the fixture could silently keep a multi-segment layout and fail in CI (24 segments, RowsStatsFiltered=163424). Split the data into two non-empty INSERT transactions so the compaction has more than one data rowset and actually rewrites, then assert through the tablet compaction status that the fixture holds exactly one segment instead of trusting the helper's return value. Verified locally: a single transaction forced to flush 9 segments fails the new invariant assertion and reproduces RowsStatsFiltered=126560 without it, while the two-transaction fixture compacts to one segment and reports RowsStatsFiltered=0 even when both transactions are forced to flush multiple segments. ### Release note None ### Check List (For Author) - Test: Regression test - query_profile.point_lookup_zone_map_profile passed on a local cluster; the fixture reports NumSegmentTotal=1 and RowsStatsFiltered=0 - variant_p0.predefine.test_variant_predefine_types_with_indexes_profile and the two fault_injection_p0 variant Bloom filter suites passed - Behavior changed: No - Does this need documentation: No
…probe ### What problem does this PR solve? Issue Number: None Related PR: apache#68279 Problem Summary: The new point_lookup_zone_map_profile suite probes the BE compaction status to assert that the fixture holds a single segment. The probe called the regression curl helper without credentials, while the compaction helper used just before it forwards feHttpUser/feHttpPassword. When the regression BE enables enable_all_http_auth, the probe would get 401 and the suite would fail before any profile assertion. Forward the same configured credentials to the probe. ### Release note None ### Check List (For Author) - Test: Regression test - query_profile.point_lookup_zone_map_profile passed on a local cluster; the log shows the probe now runs `curl -uroot: --max-time 10 -X GET ...` - Behavior changed: No - Does this need documentation: No
mrhhsg
force-pushed
the
fix/scan-stats-counted-after-key-range
branch
from
October 19, 2026 09:14
725f2f6 to
ac69ac7
Compare
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed all 16 changed paths at ac69ac763059df4b8d8b0d41cf1878021546ad40. No new actionable finding was verified. Three independent reviewers returned NO_NEW_VALUABLE_FINDINGS against the same final empty finding set; all initial risks have evidence-backed dispositions.
Both existing concerns are addressed: exact bitmap intersections now prevent sparse/disjoint candidates from inflating filtered-row counts (thread 4057083753), and the regression preserves its tables after execution (thread 4057083754). No existing P0/P1 remains applicable. The focus file contains no additional user concern.
The production flow checked was:
Segment-level pruning -> bounded bitmap and key ranges
-> index predicates and scan restrictions (existing ordering)
-> Bloom -> ordinary ZoneMap -> expression ZoneMap
-> exact bitmap intersection -> scanner-close profile collection
Critical checkpoint conclusions:
- Goal/correctness: all three page-index stages restrict evaluation to pages intersecting surviving rows and current ranges. Exact live-row deltas avoid charging earlier pruning again; final intersection retains exact candidates. Tests check output and actual evaluation work.
- Focus/abstraction: changes remain scoped to page pruning, statistics, wrapper signatures and their tests. The shared page selector removes duplicated selection logic.
- Concurrency/locks: candidate state and statistics remain iterator/scanner scoped. Existing once-only index loading and profile collection are preserved; no new thread, lock nesting or lock-order change.
- Lifecycle/memory/static initialization: temporary page lists and ranges have scoped ownership. Reader ownership remains intact, with no new asynchronous lifetime, ownership cycle, cache or cross-TU static dependency.
- Configuration: none added. The existing expression-filter gate retains disabled behavior.
- Compatibility: internal callers and overrides carry the new bitmap argument consistently. No persisted format or wire contract changes. Valid scalar writers already align ZoneMap/Bloom entries with ordinal data pages; inconsistent counts now return Corruption.
- Thrift/protobuf: no definitions changed; cross-branch field-ID checks are not applicable.
- Parallel paths: ordinary ZoneMap, Bloom, expressions, VARIANT forwarding, sparse paths, constant/default readers, unsafe casts and unsupported/multi-slot fallbacks were traced. Each column uses its own page boundaries.
- Conditions/errors: comments explain sparse seeks, repeated-page suppression, empty candidates, synthetic readers and metadata mismatch. New index/parsing Status values propagate; no new silent failure path found.
- Test coverage: instrumented unit tests cover point/disjoint/same-page/empty/rejecting candidates, Bloom work, successive columns, page boundaries, randomized independent page-selection oracles, index corruption and enabled/disabled/synthetic expressions. SQL profile assertions cover ordinary, expression and Bloom work.
- Changed results: the nine golden sections agree with the fixture and predicates; the multi-row query is ordered and other outputs are aggregates. Golden-file generation provenance was not independently reproduced.
- Observability: new evaluation counters distinguish actual page work from rows filtered, and are collected under the existing once-only scanner guard. Removed-counter references are updated. Profile level and completion handling support the regression parser.
- Transactions/persistence: version selection, delete-bitmap selection, edit logs and failover behavior are unchanged; no persistent-state mutation introduced.
- Writes/crash handling: no data-write or atomicity path changes. Temporary pruning state is released normally on checked error exits.
- FE/BE variables: none added or transmitted differently.
- Performance: bitmap seeks skip selected-page rows and sparse page gaps; sorted range append replaces repeated unions. Whole ZoneMap metadata loading remains unchanged. Full-candidate overhead and the author's benchmark figures were not measured in this review.
- Other risk sweep: iterator exhaustion, maximum ordinal bounds, aliasing intersections, pass-all metadata, dictionary rejection, candidate-pushdown ordering and special-reader reachability yielded no additional verified issue. The pinned CRoaring implementation supports the seek and half-open cardinality semantics used here.
Coverage is complete for the required static review. No builds, tests or benchmarks were run, and no product source was modified. Execution success, binary compatibility and performance measurements are not independently certified by this review.
Review converged after 1 round(s).
Member
Author
|
Re-checked the full review history at HEAD Previous inline findings — fixed and resolved:
Other top-level comments: the The current Validation at this HEAD (fresh local runs):
Could a maintainer take another look and clear the change request if the remaining concerns are satisfied? |
Member
Author
|
run buildall |
### What problem does this PR solve? Problem Summary: The branch is based on a master revision that removed JsonToVariantOptions::check_duplicate_json_path (upstream apache#68447), while four BE unit test files still pass that field in designated initializers. The BE UT pipeline builds the pull request head directly, so it fails to compile: core/data_type_serde/data_type_serde_arrow_test.cpp:960:36: error: no matching constructor for initialization of 'JsonStringToVariantEncoder' core/data_type_serde/data_type_variant_v2_serde_jsonb_test.cpp:115:32: error: no matching constructor for initialization of 'JsonStringToVariantEncoder' Remove the deleted field from the six affected initializers in data_type_serde_arrow_test.cpp, data_type_variant_v2_serde_jsonb_test.cpp, variant_v2_execution_test.cpp and vparquet_writer_test.cpp. This matches the upstream master fix (apache#69261) for the same fixtures. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh: full doris_be_test compile and link passed - ./run-be-ut.sh --run --filter='DataTypeSerDeArrowTest.*:DataTypeVariantV2SerdeJsonbTest.*:VariantV2ExecutionTest.*:VParquetWriterTest.*': 53 tests passed - Behavior changed: No - Does this need documentation: No
Contributor
TPC-H: Total hot run time: 28709 ms |
Member
Author
|
/review |
Contributor
TPC-DS: Total hot run time: 150450 ms |
Member
Author
|
Re-checked the full review history at HEAD Previous inline findings — fixed; both threads are resolved:
Latest review: the most recent Other top-level entries: The |
Contributor
ClickBench: Total hot run time: 24.33 s |
Contributor
There was a problem hiding this comment.
Static review of all 20 authoritative changed paths at ef01c98 found no new substantiated issue. Review converged after round 1: both complete-review specialists and the separate risk-focused reviewer returned NO_NEW_VALUABLE_FINDINGS, and all in-tree risks/candidates have evidence-backed dispositions.
The reviewed flow is:
bounded segment bitmap -> key/index/delete/split restrictions
-> Bloom -> ordinary ZoneMap -> expression ZoneMap
-> exact bitmap intersection -> materialization -> scanner profile
Each column selects pages using its own ordinal index and the surviving bitmap; ranges only narrow between stages. The pinned CRoaring iterator contract confirms the seek semantics used here.
Critical checkpoint conclusions:
- Goal and evidence: candidate-only page evaluation and exact sparse-row accounting are implemented. Ten new key-range/page-selection unit tests, four new expression tests and SQL/profile assertions cover the intended behavior; these were inspected statically.
- Scope, clarity and reuse: the shared candidate helper covers all three index paths. The four Variant test initializer changes adapt to the current options struct without adding product behavior.
- Concurrency and locks: pruning state is scanner-local; shared index loading retains existing call-once synchronization and profile aggregation uses existing atomic counters. No new threads, locks, lock ordering or work inside new critical sections.
- Lifecycle and static initialization: temporary page/range/evaluation objects retain lexical ownership, reader lifetimes remain valid, and no new ownership cycle or cross-translation-unit initialization dependency appears.
- Configuration: none added. Existing expression-filter enable/disable behavior remains intact.
- Compatibility: internal C++ filter callers and overrides are updated. No storage/wire-format or rolling-upgrade protocol change. Adding two page counters and removing the unused zero counter intentionally changes profile output.
- Thrift/protobuf: unchanged; field-id compatibility checks are not applicable.
- Parallel paths: ordinary ZoneMap, Bloom, expression ZoneMap, Variant forwarding, synthetic constants, sparse-only leaves and unsupported/disabled-expression fallbacks were checked. Local and cloud scans share the modified reader path.
- Conditions and errors: empty/same-page/synthetic branches explain concrete cases; inconsistent physical ZoneMap/ordinal page counts return Corruption. Status propagation remains intact.
- Test coverage: point, disjoint, same-page, empty and rejecting candidates, Bloom, cross-column pruning, randomized page oracle, malformed metadata and synthetic/disabled expressions are covered. New direct reader fixtures do not explicitly vary nullable/different-column page layouts; their ordinal invariants were traced through writers/readers.
- Expected results: all nine new golden outputs match the inserted values and SQL predicates; the multirow result has ORDER BY. Golden-generation provenance and runtime success were not independently verified.
- Observability: both evaluated-page counters are initialized and published once through scanner close; expression/ordinary counts intentionally measure column-page evaluations. Sparse filtered-row deltas count only still-live rows.
- Transactions, persistence and failover: no write, version-selection, EditLog or persisted-state changes. Existing rowset visibility and version-aligned delete-bitmap selection are preserved.
- Writes, atomicity and crash behavior: no data-write path changes; read-side temporaries retain cleanup on errors.
- FE-BE variables: none added; no sender/receiver protocol work is required.
- Performance: sparse seeks skip irrelevant pages and later stages reuse narrowed ranges; unique page vectors and incremental range construction remove redundant work. Whole-index ZoneMap metadata loading remains unchanged. Author benchmark numbers and runtime performance were not reproduced.
- Other invariants: nullable scalar ordinals include NULL rows, materialized Variant paths backfill row gaps, and synthetic readers skip physical page pruning. No additional visibility, memory-ownership, nullable-cast or error-handling defect was identified.
Existing review context: 4057083753 is fixed by alive-bitmap accounting and the disjoint-key test. 4057083754 is fixed by retaining regression tables after execution. Neither is an existing blocker; neither is reposted. The supplied user-focus file has no additional focus points.
Limits: static analysis only, with no Doris builds or tests run and no source edits. The optional out-of-tree nested-group provider is absent and was not assessed; its in-tree default is disabled. All requested in-tree paths and checkpoints are covered, with no unresolved in-tree candidate.
Review converged after 1 round(s).
Member
Author
|
Re-checked the full review history at HEAD Latest automated review: the Previous inline findings — fixed; both threads are resolved and outdated:
New top-level comments since the previous report: only a ClickBench benchmark payload on Validation at this HEAD (fresh local runs):
The current |
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28605 ms |
Member
Author
|
Re-checked the full review history at HEAD New top-level comment since the previous report: Previous inline findings — fixed; both threads are resolved and outdated:
Latest automated review: the Validation at this HEAD (fresh local run): The current |
Contributor
TPC-DS: Total hot run time: 150489 ms |
Contributor
ClickBench: Total hot run time: 24.26 s |
Member
Author
|
Re-checked the full review history at HEAD New top-level comments since the previous report (both are benchmark payloads and contain no code finding):
The other entries are Previous inline findings — fixed; both threads are resolved and outdated:
Latest automated review: the Validation at this HEAD (fresh local run): The |
Member
Author
|
run beut |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Problem Summary:
After key-range or index pruning leaves only a few rows, page-index pruning still used whole-segment ranges. Fixing
RowsStatsFilteredaccounting alone did not avoid parsing and evaluating unrelated page ZoneMaps or reading unrelated Bloom filters.This PR retains the exact surviving-row accounting and fixes the actual work:
Corruption.ZoneMapIndexPagesEvaluatedandBloomFilterIndexPagesEvaluatedso the work can be distinguished from filtered-row accounting. ZoneMap evaluations count column-pages, including expression evaluation, rather than unique physical pages.RowsZoneMapRuntimePredicateFilteredcounter from the original accounting fix.Evidence: On master
3750a819a2af157ad539d947bc301ff0f806b928plus the original counter-only changes, an instrumented predicate was evaluated on all 8 pages for both a one-row point lookup and two disjoint keys. The same assertions pass after this fix with exactly 1 and 2 evaluations. SQL regression on 200,000 rows observes 1 ZoneMap evaluation for the point lookup versus 13 for the no-key-range control, 2 for disjoint keys, and 1 Bloom-filter evaluation for the point lookup. A rejecting expression evaluates and rejects only the candidate page.Whole-index ZoneMap metadata loading is unchanged. The RELEASE microbenchmark below measures actual page-index pruning, not end-to-end query latency or disk-cold I/O.
RELEASE benchmark
Added
be/benchmark/page_index_pruning/README.md, a real ColumnWriter/ColumnReader benchmark and reproducible run/comparison scripts. The identical source selects the real old/new reader API at compile time. Compare counter-only baseline7f9400797d5against fix27896b5cbe6, with identical benchmark changes, Clang 20-O3, pinned CPUs, and the same third-party dependencies.The 144-case matrix covers ZoneMap/Bloom Filter, 8/128/4096 pages, six candidate shapes, half/all page matches, and warm/fresh readers. Before timing, it checks the complete surviving bitmap, ordinal boundaries, and actual predicate calls. Counting predicates are not used inside the timed loop. A/B/B/A, five repetitions per run, 0.1 seconds minimum, yields 10 samples per implementation/case (2,880 total).
Example: 4096 pages, one surviving candidate row, every page matches
v = 0; median CPU microseconds:Actual page evaluations drop from 4096 to 1. FreshReader includes reader creation/index decoding but uses warm file/page caches: it is not a disk-cold benchmark. The deliberately controlled constant-per-page INT data is not a general SQL workload. Alternating matching pages additionally expose the old repeated RowRanges union cost, so large ratios must not be attributed only to predicate evaluation or presented as query speedups.
Full-candidate controls are retained. The shared host and CPU scaling caused substantial noise in a few short cases; eight suspicious 8-page warm cases were rerun A/B/B/A on another pinned CPU, seven repetitions and 0.5 seconds minimum (224 additional samples). The worst repeated median increase was ~4.2% (~53 ns) when all eight pages contain candidates and half match; full-row controls increased by up to ~2.5%. This is not a claim that every workload improves. Raw JSON, per-case ranges, environment/build records and exact binaries were retained locally.
Release note
Avoid redundant ZoneMap and Bloom-filter page evaluations after key-range/index pruning, while keeping filtered-row profile counters accurate for sparse candidate rows. Add page-index evaluation counters to query profiles.
Check List (For Author)
./run-be-ut.sh --run --filter='SegmentIterator*:*ZoneMap*:*ColumnReader*:*OrdinalPageIndex*' -j32— 269 ASAN tests passed. Includes actual predicate-call instrumentation, sparse/same-page/empty candidates, cross-column pruning, randomized candidate-page oracle checks, expression enabled/disabled paths, inconsistent index metadata, and synthetic constant readers.point_lookup_zone_map_profile— generated the golden file using-forceGenOut, then reran normally with an isolated cluster configuration; query results and profile assertions passed.test_expr_zonemap_pruning,test_zone_map_delete,test_bloom_filter_is_not_nullpassed on the isolated cluster../build.sh --be --fe -j24(ASAN)../build.sh --benchmark -j32on both revisions; 144-case preflight on both plus 2,880 matrix / 224 focused timing samples, no skip/error. Fresh benchmark-only packaging needs the documented local bin/conf staging. The shared benchmark entry point also needed three stale Variant API uses adapted; no product changes were needed for the benchmark.git diff --check._in Example1, outside changed lines.NOLINTENDinbe/src/core/types.h:576on the base revision. A local-only VFS overlay removes that lint directive for analysis, with a toolchain resource-directory wrapper; no product/header workaround is included in this PR. With this analysis-only overlay, the checker passed with zero changed-line diagnostics.be/benchmark/page_index_pruning/README.md.