github-actions[bot] commented on code in PR #65548:
URL: https://github.com/apache/doris/pull/65548#discussion_r3575217642
##########
be/src/exec/scan/file_scanner_v2.cpp:
##########
@@ -428,6 +448,16 @@ Status FileScannerV2::_prepare_next_split(bool* eos) {
_state->update_num_finished_scan_range(1);
continue;
}
+ if (_should_skip_empty(status)) {
Review Comment:
This branch needs to distinguish empty-file EOF from stop EOF before
treating the split as successfully consumed. A stopped scanner can hit this
during deletion vector parsing: `stop_scanners()` calls
`FileScannerV2::try_stop()`, which sets `_io_ctx->should_stop`, and
`DeletionVectorReader::_create_file_reader()`/`read_at()` return
`Status::EndOfFile("stop read.")` when that parent context is stopped.
`TableReader::prepare_split()` propagates that raw status through
`_parse_delete_predicates()`, so this new generic `_should_skip_empty(status)`
branch aborts the split, increments `EmptyFileNum`, and marks the scan range
finished as if it were a valid empty file. Please gate this branch on the
scanner/IO stop state, or use a dedicated empty-file status, so
cancellation/limit shutdown is not reported and advanced as an empty split.
##########
be/src/format_v2/parquet/parquet_file_context.cpp:
##########
@@ -226,6 +196,12 @@ class DorisRandomAccessFile final : public
arrow::io::RandomAccessFile {
arrow::Status Close() override {
if (!_closed) {
collect_active_merge_range_profile();
+ std::lock_guard lock(_page_cache_mutex);
+ // The cache payload may outlive this reader in StoragePageCache,
but the auxiliary
+ // range directory must not. For example, after file A closes,
file B must not scan A's
+ // ranges or retain their metadata until process shutdown.
+ _cached_page_range_index.clear();
Review Comment:
This clears the only metadata that makes non-exact page-cache hits possible
while the actual cached pages remain in `StoragePageCache`. After reader A
caches `[100,200)` and closes, reader B can still exact-hit `[100,200)`, but a
normal warm request for a subset such as `[120,150)` has an empty reader-local
range directory and cannot discover the cached wider key, so it rereads from
the file. That regresses the documented subset/superset fallback for repeated
scans. Please keep bounded range metadata discoverable by file key across
reader instances, or deliberately remove the non-exact warm-cache behavior and
update the tests/comments around it.
##########
be/src/format_v2/parquet/parquet_column_schema.cpp:
##########
@@ -358,11 +358,16 @@ Status build_node_schema_with_mode(const
::parquet::SchemaDescriptor& schema,
}
column_schema->type_descriptor =
resolve_parquet_type(column_schema->descriptor);
column_schema->type = column_schema->type_descriptor.doris_type;
+ if (column_schema->type == nullptr &&
+ !column_schema->type_descriptor.unsupported_reason.empty()) {
+ // Keep unsupported logical leaves in the file schema using their
physical storage
+ // type. For example, a file `{id: INT32, clock: TIME_MILLIS}`
remains readable for
+ // `SELECT id`: schema mapping sees `clock` as its physical INT32
but never creates its
+ // reader. `SELECT clock` still fails explicitly in
ParquetColumnReaderFactory before
+ // any physical value is decoded, preserving the unsupported-type
contract.
+ column_schema->type =
column_schema->type_descriptor.physical_doris_type;
Review Comment:
This fallback keeps unsupported TIME leaves in the schema as their physical
type, but the rejection now happens only if a `ParquetColumnReader` is actually
created. Some requested-column paths use metadata before that point: `open()`
can finish with an empty scan plan after statistics/page-index pruning, and
MIN/MAX pushdown reaches
`validate_minmax_aggregate_statistics()`/`TransformColumnStatistics()` with
this physical INT leaf and no `unsupported_reason` check. A query that
references the unsupported column can therefore return an all-pruned result or
physical min/max values instead of the promised NotSupported error. Please
reject requested predicate/aggregate leaves whose
`type_descriptor.unsupported_reason` is set before any metadata pruning or
aggregate pushdown uses them.
##########
be/src/format_v2/parquet/parquet_file_context.cpp:
##########
@@ -45,6 +44,32 @@ namespace doris::format::parquet {
namespace detail {
+namespace {
+
+bool page_cache_range_less(const ParquetPageCacheRange& lhs, const
ParquetPageCacheRange& rhs) {
+ return lhs.offset < rhs.offset || (lhs.offset == rhs.offset && lhs.size <
rhs.size);
+}
+
+} // namespace
+
+void ParquetPageCacheRangeIndex::insert(ParquetPageCacheRange range) {
Review Comment:
The old global directory had a per-file range cap, but the replacement
`ParquetPageCacheRangeIndex` is an uncapped vector. During a large Parquet scan
with page cache enabled, every full in-scope `ReadAt` inserts another range,
and later misses call `plan_page_cache_range_read()` over the reader-local
vector, which copies/scans/sorts candidates while the page-cache mutex is held.
Since `StoragePageCache` eviction does not prune this metadata, one large
reader can grow this without bound until close and pay increasing CPU and
memory overhead. Please restore a per-reader/file cap or prune the index as
entries become stale.
--
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]
