wenzhenghu opened a new issue, #65762: URL: https://github.com/apache/doris/issues/65762
## Search before asking - [x] I searched the open issues for `FileScannerV2`, COUNT pushdown, external ConditionCache identity, adaptive ORC batches, and NativeReader block length, and found no similar open issue. ## Version This static review is based on the local `master` / `origin/master` head: ```text 728d362448a25051293291b42985a79f91c0122c [fix](test) Stabilize BDB unmatched transaction test (#65587) 2026-07-16T21:06:34+08:00 ``` The initial FileScannerV2 implementation was introduced by `3645dc94306d3429ca3248f17567639678bba35f` (`#65046`). I reviewed the current implementation after the follow-up fixes through the head above. This report is based on static code review only. I did not build Doris or run unit/regression tests for these findings. ## What's Wrong? I found five issues/gaps in the current FileScannerV2 path. The first two can affect availability or query correctness. The remaining items affect memory control, error handling, or observability. ### 1. File-level COUNT pushdown materializes the whole split count in one Block (high severity, V2-specific) `TableReader::_materialize_aggregate_pushdown_rows()` converts the complete `file_result.count` into synthetic rows in one call: https://github.com/apache/doris/blob/728d362448a25051293291b42985a79f91c0122c/be/src/format_v2/table_reader.h#L1499-L1508 Parquet and ORC return the total row count of all selected row groups/stripes in the split. Therefore, a file containing tens or hundreds of millions of rows can cause an equally large Doris column allocation in one scheduler call, bypassing `RuntimeState::batch_size()` and potentially causing a BE memory spike or OOM. The table-level COUNT path already emits bounded batches: https://github.com/apache/doris/blob/728d362448a25051293291b42985a79f91c0122c/be/src/format_v2/table_reader.h#L586-L605 V1 also uses `CountReader`, which emits `min(remaining_rows, batch_size)` rows per batch: https://github.com/apache/doris/blob/728d362448a25051293291b42985a79f91c0122c/be/src/format/count_reader.h#L57-L69 This appears to be a FileScannerV2 regression. The file-level aggregate result should be retained as remaining synthetic rows and emitted in bounded batches. ### 2. External ConditionCache keys do not establish a stable and globally unique file identity (high severity, inherited/shared) The V2 key is built from path, mtime, file size, predicate digest, and split range: https://github.com/apache/doris/blob/728d362448a25051293291b42985a79f91c0122c/be/src/format_v2/table_reader.cpp#L642-L657 The shared `ExternalCacheKey` does not include `fs_name`, HDFS cluster identity, S3-compatible endpoint, catalog/resource identity, or another filesystem namespace: https://github.com/apache/doris/blob/728d362448a25051293291b42985a79f91c0122c/be/src/storage/segment/condition_cache.h#L89-L123 There are two independent stale-hit scenarios: 1. If FE does not provide mtime/file size, V2 uses `mtime=0` and `file_size=-1` but still enables ConditionCache. A mutable Hive/TVF file overwritten at the same path may reuse an old survivor bitmap. 2. Two catalogs/filesystems may use the same logical path, size, mtime, split range, and predicate digest while pointing to different objects. Because the filesystem identity is absent, one scan may hit the other scan's bitmap. Unlike a normal byte-cache false hit, a ConditionCache hit can skip granules marked as having no surviving row, so either collision can silently omit valid rows. `enable_condition_cache` is enabled by default. This issue also exists in the shared V1 external cache-key design; V2 adopted it. The key should include a canonical filesystem/object identity, and caching should be disabled when no stable version is available unless the table format explicitly guarantees immutable paths. The V2 Parquet page-cache key already follows the latter rule for unknown mtime: https://github.com/apache/doris/blob/728d362448a25051293291b42985a79f91c0122c/be/src/format_v2/parquet/parquet_file_context.cpp#L191-L207 ### 3. Adaptive batch size is enabled for readers that ignore the predicted size (medium severity, V2-specific) FileScannerV2 enables adaptive batching for ORC, CSV/TEXT, JSON, and JNI in addition to Parquet: https://github.com/apache/doris/blob/728d362448a25051293291b42985a79f91c0122c/be/src/exec/scan/file_scanner_v2.cpp#L786-L815 However: - ORC always creates a 4096-row batch: https://github.com/apache/doris/blob/728d362448a25051293291b42985a79f91c0122c/be/src/format_v2/orc/orc_reader.cpp#L1598-L1609 - Delimited text uses `RuntimeState::batch_size()`: https://github.com/apache/doris/blob/728d362448a25051293291b42985a79f91c0122c/be/src/format_v2/delimited_text/delimited_text_reader.cpp#L309-L330 - JSON also uses `RuntimeState::batch_size()`: https://github.com/apache/doris/blob/728d362448a25051293291b42985a79f91c0122c/be/src/format_v2/json/json_reader.cpp#L271-L285 These readers inherit the no-op `FileReader::set_batch_size()`. Consequently, the profile reports adaptive predictions that the physical reader does not use. In particular, very wide/nested ORC rows still start with 4096 physical rows instead of the intended 32-row probe, defeating the memory-control goal. Either the readers should implement the dynamic batch-size contract, or unsupported formats should be removed from the adaptive enablement matrix until they do. ### 4. NativeReader allocates an untrusted block length before validating it (medium severity, inherited) The V2 NativeReader reads a `uint64_t block_len` from the file and immediately executes `buffer->assign(block_len, '\0')`: https://github.com/apache/doris/blob/728d362448a25051293291b42985a79f91c0122c/be/src/format_v2/native/native_reader.cpp#L222-L266 It does not first verify that `block_len <= file_size - current_offset` or that it safely fits the allocation type. A corrupt or truncated Native file can therefore request a huge allocation and trigger `std::bad_alloc`/OOM before the reader returns a bounded corruption error. V1 has the same behavior. Both readers should validate the remaining file length before allocating the block buffer. ### 5. Paimon/Hudi hybrid readers hide native child ConditionCache hit counts (low severity, V2-specific) FileScannerV2 reads the hit count from the top-level `TableReader`: https://github.com/apache/doris/blob/728d362448a25051293291b42985a79f91c0122c/be/src/exec/scan/file_scanner_v2.cpp#L1030-L1044 `TableReader::condition_cache_hit_count()` is non-virtual and returns only that object's counter: https://github.com/apache/doris/blob/728d362448a25051293291b42985a79f91c0122c/be/src/format_v2/table_reader.h#L309-L323 `PaimonHybridReader` and `HudiHybridReader` delegate native splits to child `TableReader` objects, so the actual child increments its counter while the top-level hybrid counter remains zero. Cache filtering still works because the IO context is shared, but `ConditionCacheHit` remains underreported. The accessor should be virtual, or hybrid readers should expose the sum of their child counters. ## What You Expected? - File-level COUNT pushdown should emit synthetic rows in batches bounded by `batch_size`. - ConditionCache must never reuse a bitmap across different physical objects or unknown mutable versions. - Every format advertised as adaptive should apply the predicted physical batch size, and profile counters should reflect the actual batch. - Malformed Native files should return a bounded error before attempting an allocation larger than the remaining file data. - Hybrid readers should report the cache-hit counters of the readers that performed the scan. ## How to Reproduce? These are static-review findings and have not yet been runtime-tested. Suggested focused reproductions/tests are: 1. Set a fake/file aggregate COUNT result to `batch_size + 5`; verify that TableReader returns multiple bounded batches rather than one oversized Block. 2. Populate ConditionCache from one HDFS/S3-compatible catalog, then scan another catalog with the same path/mtime/size/range/predicate but different contents; verify that the second scan cannot hit the first bitmap. Also test same-path overwrite with missing mtime. 3. Enable adaptive batching and scan a wide ORC file; verify the first physical ORC batch uses the 32-row probe and subsequent batches follow predictions. 4. Create a Native file whose block-length header is larger than the remaining file; verify it returns a corruption error without a large allocation. 5. Warm ConditionCache through a native Paimon/Hudi split and verify `ConditionCacheHit` increases on the hybrid scanner profile. ## Anything Else? The review also checked the current split-level runtime-filter digest refresh, EOF-only ConditionCache publication, delete/deletion-vector cache gating, ignored-NOT_FOUND cleanup, residual predicate preservation, and current FileScannerV2 IO accounting. I did not find a remaining issue in those paths at the reviewed head. No code changes are included with this report. ## Are you willing to submit PR? - [ ] Yes I am willing to submit a PR. ## Code of Conduct - [x] I agree to follow this project's [Code of Conduct](https://www.apache.org/foundation/policies/conduct). -- 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]
