zclllyybb commented on issue #65762: URL: https://github.com/apache/doris/issues/65762#issuecomment-5000780913
Breakwater-GitHub-Analysis-Slot: slot_89cf5530d3cf This content is generated by AI for reference only. I re-read the live issue and checked the reported Doris commit `728d362448a25051293291b42985a79f91c0122c`. I also refreshed Apache `upstream/master` to `d8e75cf63cd8372a1d7e4b68d629859d9dfa75dc`; the files involved in these findings have not changed on upstream master after the reported commit, so the report still looks actionable against current master. I did not run BE/FE tests; this is a read-only code-path assessment. Initial assessment: 1. File-level COUNT pushdown batching looks valid and high priority. `TableReader::_try_materialize_aggregate_pushdown_rows()` calls `_materialize_aggregate_pushdown_rows()` once, then closes the reader. For COUNT, `_materialize_aggregate_pushdown_rows()` directly resizes every projected column to `file_result.count`. Parquet and ORC compute that count from all selected row groups/stripes for the split, so this path can build a block far larger than `RuntimeState::batch_size()`. The existing table-level count path already has the right remaining-count state machine, so the file-level metadata count should use the same bounded emission model. 2. The external ConditionCache key identity issue also looks valid and correctness-sensitive. `FileDescription` carries `fs_name`, `mtime`, `file_size`, range, and an `is_immutable` flag, but `ExternalCacheKey::encode()` only uses path, mtime, file size, predicate digest, range, and format version. For generic mutable files, missing scan-range metadata becomes `mtime=0` and `file_size=-1`, while `enable_condition_cache` is true by default when FE sends a digest. A false cache hit is not just stale metadata: Parquet/ORC use false granules to prune row ranges before normal predicate evaluation, so a collision can silently omit rows. The fix should add a stable filesystem/object namespace to the key and disable condition-cache use when the object version is unknown unless the table-format reader has explicitly marked the file immutable. 3. The adaptive-batch finding is partially confirmed. The enablement matrix includes ORC, delimited text/CSV, and JSON, but those physical readers do not consume the predicted batch size: ORC creates row batches with `DEFAULT_ORC_READ_BATCH_SIZE`, and text/JSON read against `RuntimeState::batch_size()`. Parquet does implement `set_batch_size()`, and JNI has a table-reader override, so I would scope this item to the readers that ignore the hook rather than all enabled formats. Either implement the hook for ORC/text/JSON, or remove those formats from adaptive enablement until the profile counter reflects the real physical batch. 4. The NativeReader resource-safety report is valid. V2 reads `block_len`, advances the offset, and calls `buffer->assign(block_len, '\0')` before proving that the requested body fits in the remaining file bytes. A malformed length prefix can therefore request an allocation much larger than the physical file before returning the later short-read error. This should be fixed with a pre-allocation `block_len <= file_size - current_offset` check and a malformed-file unit test. Public PR #65548 appears to include an overlapping pre-allocation validation, but it is still open at the time of this comment. 5. The hybrid-reader profile undercount is valid but lower severity. `FileScannerV2` reports `ConditionCacheHit` through the top-level table reader. `TableReader::condition_cache_hit_count()` is non-virtual and returns only the top-level counter, while Paimon/Hudi hybrid readers delegate native splits to child readers that own the actual cache lookup counter. Filtering should still work through the shared IO context, but the hit counter can remain zero. A virtual accessor or hybrid sum/current-child accessor would fix the profile. Suggested next steps: - Treat items 1 and 2 as correctness/availability fixes and cover them with focused tests. - Cross-check open PR #65548 for overlap before implementing, especially NativeReader validation and COUNT-related fallback behavior, but do not assume it resolves the bounded file-level COUNT batching, ConditionCache key identity, adaptive ORC/text/JSON batching, or hybrid hit-count profile issue without reviewing the final merged diff. - Add regression or BE unit coverage for: COUNT result larger than `batch_size`, ConditionCache collisions across filesystem identities and missing mtime/file size, ORC first-batch adaptive probe behavior on wide rows, truncated Native block length/body, and Paimon/Hudi native split `ConditionCacheHit` reporting. -- 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]
