Repository navigation
[fix](be) Fix ORC condition-cache seeks past filtered stripe suffixes - #70079
Merged
Merged
Conversation
### What problem does this PR solve? Related PR: apache/doris-thirdparty#431 Problem Summary: A condition-cache granule can start inside a stripe's SARG-filtered suffix while containing matching rows in the next stripe. ORC seekToRow treated the stripe-end sentinel as a valid row-group index, causing an out-of-bounds index access or premature EOF. Pin the ORC fix that initializes the next selected stripe before positioning column readers. Add a Doris reader regression, a synthetic two-stripe ORC fixture with its generator, and a SQL regression covering cache hits with lazy reads and min/max pruning enabled or disabled. ### Release note Fix potential crashes and missing rows when ORC condition-cache seeks reach a SARG-filtered stripe suffix. ### Check List (For Author) - Test: 247 ORC-related BE unit tests passed under AddressSanitizer; 56 focused standalone ORC tests passed. SQL regression added and Groovy compilation checked; external SQL suite has not been executed locally. - Behavior changed: Yes, safely advance to matching rows or EOF. - Does this need documentation: No.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 28652 ms |
Contributor
TPC-DS: Total hot run time: 151278 ms |
Contributor
ClickBench: Total hot run time: 24.21 s |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
|
Title: fix Fix condition-cache seeks past SARG-filtered stripe suffixes BackgroundWith the condition cache ( What problem does this PR solve?Issue Number: None Problem Summary: A condition-cache hit on an ORC file can crash the BE or silently drop rows. The granule it seeks to can start in row groups at the end of a stripe that the SARG skipped, while its matching row sits in the next stripe, and ORC's Details
Testing
Notes for Reviewers
Release noteFix BE crashes and missing rows in repeated ORC queries with the condition cache on (the default), when a cached range started inside the skipped end of a stripe. Check List (For Author)
AI Disclosure
|
morningman
previously approved these changes
Nov 4, 2026
Contributor
Author
Local pipeline review — ✅ PASSschema: doris-repo-review/v1
status: PASS
pr: apache/doris#70079
commit: 79b0eb0d7ba93ca9a2801b47a2ed68d59a6a2a11
base: c8a75b273e999370e354ed39ae65db2c483c0ad0
reviewed_at: 2026-11-04T10:17+08:00
reviewer: Gabriel39
model: gpt-6-astra
effort: xhigh
findings: {blocker: 0, major: 0, minor: 1, nit: 0}
rounds: 1
converged: trueNotes for maintainers
Reviewed locally with the |
### What problem does this PR solve? Related PR: apache#70079 Problem Summary: A full condition-cache shard first records a new key in LRU-K ghost history and admits it after the second scan. The old backend count plus one retry bound can fail before any backend receives a third scan. Allow twice the backend count plus one attempts, retaining early exit on a cache hit, and document the admission requirement. ### Release note None ### Check List (For Author) - Test: Groovy 4.0.19 syntax conversion and git diff --check passed; reviewed the LRU-K insertion and reader-finalization paths. The full SQL regression was not run locally because no built test cluster is available. - Behavior changed: No production behavior changes; regression warmup only. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
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.
Static review of PR #70079 at 176aa32d4717a008166804a0fd5ed52d652a13dc: no actionable new defect found. All five changed paths and the complete pinned ORC dependency delta were reviewed. There are no existing inline findings to retain and no additional user focus points were supplied.
The ORC change recognizes the filtered-suffix stripe-end sentinel before row-index positioning, initializes the next selected stripe, and returns at the configured range end. Static tracing supports correct file-global row positions and aligned predicate/payload columns for eager and lazy reads.
| Critical checkpoint | Conclusion |
|---|---|
| Goal and supporting tests | The fix addresses the stated invalid-index/premature-EOF path. The BE regression exercises granules 0/9 and seek row 18432 with lazy reads and min/max pruning independently toggled. |
| Scope and reuse | Focused dependency update plus tests, fixture and generator; existing stripe initialization and reader helpers are reused. |
| Concurrency and locking | No new threads, shared production state, locks or lock-order changes; reader state remains per RowReader. |
| Lifecycle and static initialization | Transition releases old column readers and clears index, bloom and follower state; scoped test resources and callbacks have appropriate lifetimes. No new static initialization dependency. |
| Configuration | No configuration added. Tests set existing session/query options and restore SQL settings in finally. |
| Compatibility | No storage format, public API or wire change. The dependency prerequisite is merged. No new glibc-facing call is introduced; release ELF compatibility was not audited in this static review. |
| Thrift/protobuf IDs | No gensrc definition changes. |
| Parallel and special paths | Inspected initialized/fresh and backward seeks, exact/partial final groups, skipped intermediate stripes, split limits, EOF, and lazy follower positioning including a nonzero first selected group. |
| Conditional checks | The new sentinel branch has a concrete failure example and precedes index access; it reuses the existing bounded stripe-selection path. |
| Functional and negative coverage | Inspected the four-way BE/SQL option matrix and 32 dependency cases covering initialization, exact/partial groups, stripe count, range restriction and lazy/eager reads, plus EOF and backward-seek assertions. |
| Expected results | Independently parsed fixture metadata: 1458 bytes, 40000 rows, two 20000-row stripes, stride 10000. IDs/payloads agree with the generator and ordered SQL expectations. The two-SQL quickTest overload needs no golden .out file. |
| Observability | Tests require actual seek/skipped-row evidence; SQL profiles must show positive ConditionCacheHit and ConditionCacheFilteredRows. Existing reader counters remain in use. |
| Transactions and persistence | No production commit, EditLog, replay, failover or persistent-write path changes; version and delete-bitmap invariants are unaffected. |
| Data writes and crash behavior | Production change is read-only; exception/cancellation conversion remains in the existing Doris seek/next boundary. No new production write atomicity requirement. |
| FE-BE variables | No variables added or newly transmitted. Existing lazy/min/max options reach the reader. |
| Performance and memory | No new per-row work or allocation; pruning and lazy reads are retained. Stripe initialization frees the old reader before rebuilding; test allocations are bounded. No performance measurements were executed. |
| Additional checks | UUID profile markers do not enter predicate cache digests; EOF gates cache publication. Fixture discovery, S3/Azure upload/cleanup, result comparison and settings restoration follow the existing harness. |
Review converged after two rounds, including final independent review of the frozen payload. All risk questions and candidates are resolved with evidence; no source scope remains unreviewed.
Validation limits: this invocation performed static inspection only. No builds, tests, generator execution, benchmarks, external SQL/S3/Azure runs, submodule initialization or source edits were performed. The author's reported test passes were not independently reproduced. The next stripe starting at a nonzero selected group was checked by control-flow analysis rather than a newly executed case.
Review converged after 2 round(s).
zhangstar333
approved these changes
Nov 4, 2026
Contributor
TPC-H: Total hot run time: 28555 ms |
Contributor
TPC-DS: Total hot run time: 151569 ms |
Contributor
ClickBench: Total hot run time: 24.34 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
4 of 8 tasks
yiguolei
pushed a commit
that referenced
this pull request
Nov 5, 2026
…/JSON/WAL fixes (#70185) ### What problem does this PR solve? Backport the following master PRs to `branch-4.2`, in master merge order: 1. #69672 — Preserve source values and alias semantics in ES scans. 2. #69971 — Add session controls for Parquet page indexes and data-page size. 3. #70155 — Repair ES source predicate regression cases. 4. #70079 — Fix ORC condition-cache seeks past filtered stripe suffixes. 5. #70154 — Preserve JSON and WAL rows with empty file projections. The ES changes are adapted to the existing 4.2 FE planner and BE source locations. Mapping safety, scan-wide read mode, local residuals, native `esquery` handling and alias routing retain the upstream behavior without introducing the master connector framework. Added adapter tests cover both scan protocols, aliases, missing-value negations and native residual rejection. Preserve existing branch-specific ES regression coverage. Bring in the previously absent predicate suite, adapting its plan assertions to 4.2's `LOCAL_PREDICATES` output and retaining LIMIT result checks. Parquet variables use the 4.2 annotation interface and the original optional Thrift field IDs. The ORC dependency is pinned to the same fixed commit as #70079. The ES implementation also preserves source numeric and JSONB predicate semantics after scan-wide fallback. Numeric-token rounding must not make a remote predicate discard a row matching the value Doris materializes; empty JSONB objects remain nonnull even when ES indexes no leaf. Docvalue eligibility and pushdown are retained. Mapped date formats and their order are carried per concrete backing index through the existing property map to both scan protocols. Calendar/epoch alternatives now distinguish basic_date from epoch_millis, for strings and source numeric tokens. Numeric docvalues and older FE numeric requests retain their epoch decoder. Typed alias selection consistently uses the requested mapping type in schema inference and planning. The pinned ORC decoder's libdeflate dependency is included on ARM, matching master. ### Release note Fix ES source-value filtering, mapped dates and typed alias query correctness, ORC condition-cache seek crashes/missing rows and its ARM link dependency, and JSON/WAL row counts for empty file projections. Add Parquet output page-index and data-page-size session controls. ### Validation - FE compilation and Checkstyle passed. ES validation: 81 tests (48 datasource/planner tests and 33 legacy ES tests), 0 failures/errors, 1 external-environment skip. The 9 Parquet session-variable tests passed. - Rebuilt the pinned ORC dependency under AddressSanitizer: 56 focused tests passed, including 32 filtered-stripe-suffix seek cases. - Compiled all 10 affected Doris C++ implementation/test translation units and the ORC fixture generator. The regenerated fixture is byte-for-byte identical to the committed fixture. - clang-format 16 checks passed for all affected Doris C++ sources and headers. Modified Groovy suites compiled successfully. - The ES reader/parser implementations and test translation units compiled. The shared DATETIMEV2 date-format algorithm passed 21 standalone ASAN checks. CMake dependency checks passed for aarch64, arm64 and x86_64; an actual ARM link was not run locally. - Added `test_es_source_review_cases.groovy` with SQL-to-SQL checks against an internal reference table for numeric/JSONB source predicates, LIMIT, epoch-only formats, and explicit legacy DATETIMEV2 basic_date columns. It covers both scanners and parallel-scroll settings, waits for legacy metadata readiness, and restores session settings. The suite compiled; no golden files were handwritten. - The applicable ES fixes are synchronized to master in #70205. Master already links libdeflate on every architecture. - Full Doris BE unit-test execution and live external SQL regression were not performed locally; CI validation is requested. ### Check List (For Author) - Test - [x] Unit Test - [x] Regression tests included - Behavior changed: - [x] Yes; the upstream behavior is backported to 4.2. - Does this need documentation? - [x] Parquet controls are documented by the upstream documentation PR: apache/doris-website#4581 ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
4 of 7 tasks
Gabriel39
added a commit
that referenced
this pull request
Nov 9, 2026
) ### What problem does this PR solve? Backport three master fixes to `branch-4.1`: avoid unnecessary database-object loading for SCHEMATA scans, prevent ORC condition-cache seek crashes or missing rows, and preserve JSON/WAL row counts when no file columns are projected. The commits retain their original authors and are applied in master merge order: 1. #70010 — Avoid database-object enumeration when SCHEMATA only needs names. 2. #70079 — Advance safely past SARG-filtered ORC stripe suffixes during condition-cache seeks. 3. #70154 — Track logical JSON records independently of projected columns and capture WAL row counts before column ownership is transferred. Branch adaptations: - Keep Thrift field 7 reserved and add the optional `include_db_ids` flag at upstream field 8 with its default `true`. Preserve existing privilege checks, pattern matching and schema-name formatting for both enumeration paths. - Use the upstream fixed ORC dependency revision `5962bb071d6345cc29c5353d64d85a203525e4d9`, which descends from the branch's existing revision. - Resolve the JSON reader conflicts by retaining 4.1's error handling and adding logical row accounting without importing unrelated missing-field/load-rejection changes. Remove the rooted-array pre-count that consumes the ondemand cursor before JSON-path materialization; retain the upstream rooted-array coverage. ### Validation - The branch's FE production and test sources compiled with the existing Java 8 target settings. All **7 targeted FE tests passed**, including the existing `FrontendServiceImplTest#testGetDBNames`, the 100,000-namespace case, filtering and legacy-request compatibility. FE Checkstyle passed. - A fresh ASAN build of the exact pinned ORC dependency passed **56 focused tests**, including **32 filtered-stripe-suffix boundary cases**. - All **7 affected C++ implementation/test/generator translation units compiled**. Clang-format 16 passed for all **8 affected C++ source/header files**. - The ORC fixture generator reproduced the committed fixture byte for byte. All **3 added Groovy suites compiled**. - Full Doris BE unit-test execution and external SQL regression execution remain for CI; they were not run locally for this backport. ### Release note Improve SCHEMATA enumeration for large catalogs. Fix potential crashes or missing rows during ORC condition-cache seeks and incorrect JSON/WAL row counts with empty file projections. ### Check List (For Author) - Test - [x] Unit tests and focused dependency tests - [x] Regression tests included and compiled; full SQL execution pending CI - Behavior changed: - [x] Yes. Apply the upstream fixes while preserving branch-specific validation and protocol compatibility. - Does this need documentation? - [x] No. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Add branch pick label ### ARM64 link dependency follow-up The ORC revision enables libdeflate ZLIB decoding on every architecture. Remove the old x86-only guard around `add_thirdparty(deflate)` so ARM64 BE links also receive the required library, matching master. An adjacent comment records this dependency invariant. Validation: probes include each branch's actual `be/cmake/thirdparty.cmake` and select `aarch64`, `arm64`, `x86` and `x86_64` dependency configurations. Before the fix, both ARM configurations fail to link with missing libdeflate symbols while both x86 controls pass. After the fix, all eight repository/configuration combinations link and pass a DEFLATE compression/decompression round trip. These probes run natively on x86_64 and validate CMake dependency selection; a native ARM64/full BE build remains for CI. Clang-format 16 passed for the PR-affected C++ files.
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?
Related PR: apache/doris-thirdparty#431 (merge the ORC dependency fix first)
A condition-cache granule can start in a stripe's SARG-filtered suffix while containing a matching row in the next stripe. On an initialized stripe, ORC's
seekToRow()uses the stripe-end sentinel as a row-group index. This can access past the index when the stripe ends on an index boundary, or prematurely return EOF when the last row group is partial.The same-stripe fast path originates in Apache ORC ORC-1170; the same missing stripe-end handling remains in the current upstream C++ reader.
Pin the ORC fix that initializes the next selected stripe before positioning its column readers. Preserve SARG pruning, lazy reads and split boundaries.
Add a Doris reader regression and a 1.5 KB synthetic ORC fixture with its generator. The fixture has two 20,000-row stripes, a 10,000-row index stride, and one matching row at the start of each stripe. A cache hit seeks to row 18,432 in the first stripe's filtered suffix. The SQL regression compares cache-disabled and cached results with lazy reads and min/max pruning enabled or disabled, and checks the profile for actual cache hits and skipped rows.
Release note
Fix potential crashes and missing rows when ORC condition-cache seeks reach a SARG-filtered stripe suffix.
Check List (For Author)