morningman commented on PR #70079: URL: https://github.com/apache/doris/pull/70079#issuecomment-6606940277
Title: [fix](orc) Fix condition-cache seeks past SARG-filtered stripe suffixes ### Background With the condition cache (`enable_condition_cache`, on by default), Doris records which 2048-row granules of a file may hold rows that match a filter, and a later scan with the same filter skips the other granules ([condition cache](https://doris.apache.org/docs/4.x/query-acceleration/condition-cache)). The format-v2 ORC reader skips them by calling the ORC library's `seekToRow`. An ORC file consists of stripes, each split into row groups of `rowIndexStride` rows (10,000 by default) with min/max statistics, which the search argument (SARG) uses to skip row groups that cannot match. ### What problem does this PR solve? Issue Number: None Related PR: apache/doris-thirdparty#431 (the ORC fix this PR pins; merged), #60897 (condition cache for external tables, which first seeks ORC readers to cached granules), #65183 (format-v2 ORC reader, whose condition-cache seek triggers the bug), #67141 (earlier fix to the same seek), #69591 (disabled the LOCAL TVF, which turned off the existing ORC condition-cache suite), #68593 (Doris side of the ORC libdeflate commits that a 4.x pick also brings in) PR Type: Bugfix 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 `seekToRow` then uses the stripe end as a row-group index. The condition cache is on by default, so repeating a filter on such a file is enough. The fix is in the ORC library (apache/doris-thirdparty#431): `seekToRow` now starts the next selected stripe, as a fresh seek does. This PR moves `contrib/apache-orc` to that fix; the rest of the diff is tests. ### Details - **Frequency:** Always, given the trigger: on a condition-cache hit, the next granule to read starts in the current stripe, the SARG skipped every row group from there to the stripe's end, and the granule's match is in a later stripe. The fixture's hit seeks to row 18,432, in the skipped second row group of a 20,000-row stripe. TODO(author): whether users reported it or it was found by reading the code. - **Root cause:** For a seek inside the stripe being read, ORC's `RowReaderImpl::seekToRow` (fast path from ORC-1170, apache/orc#1114) moves the target to the next row group the SARG selected, via `advanceToNextRowGroup`. When none is left, that returns the stripe's row count, and the readers are positioned at a row group that doesn't exist: they read past the row index if the stripe ends on an index boundary, and per #431 hit an early EOF if the last group is partial. - **Why existing tests missed it:** BE: in `be/test/format_v2/orc/orc_reader_test.cpp`, only a cache-miss test uses the fixture with SARG-skipped row groups, so no cache hit seeks into a skipped tail; ORC's own tests had no such case. Regression: the closest suite, `external_table_p0/cache/condition_cache_orc.groovy`, has returned early since #69591 disabled the LOCAL TVF it reads through (1 ms in this PR's External run). - **Fix:** Moves `contrib/apache-orc` from `5860f94768b` to `5962bb071d6`, where `seekToRow` advances through `startNextStripe`, the call a fresh seek makes, and returns at the end of the selected range. `5962bb071d6` is #431's head, which only `refs/pull/431/head` reaches; #431 was squash-merged as `7955b007759` with the same tree, so the pin can move there without a code change. - **Other paths and branches:** Every `seekToRow` caller gets the fix, including the row-id fetch; Parquet's granule skip is Doris code and doesn't use ORC. branch-4.1, branch-4.2, 4.1.5 and 4.2.0-rc02 have the bug: their ORC `be0f1b73a7a` has the same fast path, and the format-v2 ORC reader with this seek (backported in #65559) is their default. Moving their pin to the fix also brings apache/doris-thirdparty#417 and apache/doris-thirdparty#418, and #417 makes ORC decompress ZLIB with libdeflate on every architecture, while 4.x links `deflate` only on x86; a pick needs #68593 first, as master had, or a 4.x ORC commit with only the fix. ### Testing - New BE unit test `NewOrcReaderTest.ConditionCacheHitSeeksPastSargFilteredStripeSuffix`: a cache hit on granules 0 and 9 of the fixture, with lazy reads and min/max filtering each on and off, returns both matching rows with their payloads and skips rows by seeking. Passed in this PR's BE UT run at `79b0eb0d7ba`; the author reports 247 ORC-related BE tests passing under AddressSanitizer. TODO(author): whether it fails with the old ORC commit, and how. - New regression suite `external_table_p0/tvf/test_orc_condition_cache_seek.groovy`: the same four combinations through the `S3` TVF on the fixture uploaded to the test object store, with and without the cache, requiring the two expected rows and non-zero `ConditionCacheHit` and `ConditionCacheFilteredRows`. Passed in this PR's External Regression run at `79b0eb0d7ba`; the author did not run it locally. - #431 adds 32 ORC seek tests for both stripe endings; the author reports that the 16 on an initialized stripe fail on the original reader, that all 32 pass with the fix, and that 56 focused ORC tests pass under AddressSanitizer. ### Notes for Reviewers - The code to review is in apache/doris-thirdparty#431: 11 lines in `seekToRow`; its other `Reader.cc` changes (+25 −25) only rewrap unrelated lines. - `GenerateOrcConditionCacheSeek.cpp` regenerates the 1.5 KB fixture `condition_cache_seek.orc`. ### Release note Fix 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) - Behavior changed: No - Does this need documentation: No - Pre-submission status: Partially verified: the new BE test and regression suite have not been shown to fail with the old ORC commit ### AI Disclosure - Tool: Claude Code (Claude Opus 5.5) for this description; not stated for the code - Generated: this description; whether the code was generated is not stated - Human review: not stated -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
