Search before asking
Version
This static review is based on the local master / origin/master head:
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:
|
Status _materialize_aggregate_pushdown_rows(TPushAggOp::type agg_type, |
|
const FileAggregateResult& file_result, |
|
Block* block) { |
|
if (agg_type == TPushAggOp::type::COUNT) { |
|
// COUNT pushdown is not a final count value. It emits `count` default rows so the |
|
// upper COUNT(*) aggregate can count them and produce the final result, including |
|
// zero rows when count is 0. |
|
DORIS_CHECK(file_result.count >= 0); |
|
return _materialize_count_rows(cast_set<size_t>(file_result.count), block); |
|
} |
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:
|
Status _read_table_level_count(Block* block, bool* eos) { |
|
DORIS_CHECK(block != nullptr); |
|
DORIS_CHECK(eos != nullptr); |
|
DORIS_CHECK(_push_down_agg_type == TPushAggOp::type::COUNT); |
|
DORIS_CHECK(_remaining_table_level_count >= 0); |
|
if (_remaining_table_level_count == 0) { |
|
_remaining_table_level_count = -1; |
|
_current_task.reset(); |
|
*eos = true; |
|
return Status::OK(); |
|
} |
|
|
|
const int64_t batch_size = _runtime_state == nullptr |
|
? _remaining_table_level_count |
|
: static_cast<int64_t>(_runtime_state->batch_size()); |
|
const auto rows = std::min(_remaining_table_level_count, batch_size); |
|
RETURN_IF_ERROR(_materialize_count_rows(cast_set<size_t>(rows), block)); |
|
_remaining_table_level_count -= rows; |
|
*eos = false; |
|
return Status::OK(); |
V1 also uses CountReader, which emits min(remaining_rows, batch_size) rows per batch:
|
Status _do_get_next_block(Block* block, size_t* read_rows, bool* eof) override { |
|
auto rows = std::min(_remaining_rows, static_cast<int64_t>(_batch_size)); |
|
_remaining_rows -= rows; |
|
|
|
auto mutable_columns_guard = block->mutate_columns_scoped(); |
|
auto& mutate_columns = mutable_columns_guard.mutable_columns(); |
|
for (auto& col : mutate_columns) { |
|
col->resize(rows); |
|
} |
|
|
|
*read_rows = rows; |
|
*eof = (_remaining_rows == 0); |
|
return Status::OK(); |
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:
|
Status TableReader::_init_reader_condition_cache(const FileScanRequest& file_request) { |
|
_condition_cache = nullptr; |
|
_condition_cache_ctx = nullptr; |
|
if (!_should_enable_condition_cache(file_request)) { |
|
return Status::OK(); |
|
} |
|
|
|
auto* cache = segment_v2::ConditionCache::instance(); |
|
if (cache == nullptr) { |
|
return Status::OK(); |
|
} |
|
const auto& file = *_current_file_description; |
|
_condition_cache_key = segment_v2::ConditionCache::ExternalCacheKey( |
|
file.path, file.mtime, file.file_size, _condition_cache_digest, file.range_start_offset, |
|
file.range_size, |
|
segment_v2::ConditionCache::ExternalCacheKey::BASE_GRANULE_AWARE_VERSION); |
The shared ExternalCacheKey does not include fs_name, HDFS cluster identity, S3-compatible endpoint, catalog/resource identity, or another filesystem namespace:
|
// Cache key for external tables (Hive ORC/Parquet) |
|
struct ExternalCacheKey { |
|
static constexpr uint8_t BASE_GRANULE_AWARE_VERSION = 1; |
|
|
|
ExternalCacheKey() = default; |
|
ExternalCacheKey(const std::string& path_, int64_t modification_time_, int64_t file_size_, |
|
uint64_t digest_, int64_t start_offset_, int64_t size_, |
|
uint8_t format_version_ = 0) |
|
: path(path_), |
|
modification_time(modification_time_), |
|
file_size(file_size_), |
|
digest(digest_), |
|
start_offset(start_offset_), |
|
size(size_), |
|
format_version(format_version_) {} |
|
std::string path; |
|
int64_t modification_time = 0; |
|
int64_t file_size = 0; |
|
uint64_t digest = 0; |
|
int64_t start_offset = 0; |
|
int64_t size = 0; |
|
uint8_t format_version = 0; |
|
|
|
[[nodiscard]] std::string encode() const { |
|
std::string key = path; |
|
char buf[41]; |
|
memcpy(buf, &modification_time, 8); |
|
memcpy(buf + 8, &file_size, 8); |
|
memcpy(buf + 16, &digest, 8); |
|
memcpy(buf + 24, &start_offset, 8); |
|
memcpy(buf + 32, &size, 8); |
|
buf[40] = static_cast<char>(format_version); |
|
key.append(buf, 41); |
|
return key; |
|
} |
There are two independent stale-hit scenarios:
- 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.
- 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:
|
std::string build_page_cache_file_key(const io::FileReader& file_reader, |
|
const io::FileDescription& file_description) { |
|
const int64_t mtime = |
|
file_description.mtime != 0 ? file_description.mtime : file_reader.mtime(); |
|
if (mtime == 0 && !file_description.is_immutable) { |
|
// mtime == 0 means "unknown version", not the Unix epoch. V1 historically caches such a |
|
// file under path::0, but copying that behavior for every V2 file is unsafe: a mutable file |
|
// can be overwritten with different bytes while retaining both its path and size, causing |
|
// process-global page cache entries to return stale data. Only callers that explicitly |
|
// guarantee path immutability may use the mtime=0 cache key below. |
|
return {}; |
|
} |
|
const int64_t file_size = file_description.file_size >= 0 |
|
? file_description.file_size |
|
: static_cast<int64_t>(file_reader.size()); |
|
return fmt::format("{}::{}::mtime={}::size={}", file_description.fs_name, |
|
file_reader.path().native(), mtime, file_size); |
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:
|
bool FileScannerV2::_should_enable_adaptive_batch_size(TFileFormatType::type format_type) const { |
|
if (!config::enable_adaptive_batch_size) { |
|
return false; |
|
} |
|
switch (format_type) { |
|
case TFileFormatType::FORMAT_PARQUET: |
|
case TFileFormatType::FORMAT_ORC: |
|
case TFileFormatType::FORMAT_CSV_PLAIN: |
|
case TFileFormatType::FORMAT_CSV_GZ: |
|
case TFileFormatType::FORMAT_CSV_BZ2: |
|
case TFileFormatType::FORMAT_CSV_LZ4FRAME: |
|
case TFileFormatType::FORMAT_CSV_LZ4BLOCK: |
|
case TFileFormatType::FORMAT_CSV_LZOP: |
|
case TFileFormatType::FORMAT_CSV_DEFLATE: |
|
case TFileFormatType::FORMAT_CSV_SNAPPYBLOCK: |
|
case TFileFormatType::FORMAT_PROTO: |
|
case TFileFormatType::FORMAT_TEXT: |
|
case TFileFormatType::FORMAT_JSON: |
|
case TFileFormatType::FORMAT_JNI: |
|
return true; |
|
default: |
|
return false; |
|
} |
|
} |
|
|
|
bool FileScannerV2::_should_run_adaptive_batch_size() const { |
|
// COUNT pushdown emits synthetic rows from file metadata and does not materialize file columns, |
|
// so there is no useful row-width sample to learn from. |
|
return _block_size_predictor != nullptr && |
|
_local_state->get_push_down_agg_type() != TPushAggOp::type::COUNT; |
However:
- ORC always creates a 4096-row batch:
|
Status OrcReader::_create_row_reader() { |
|
try { |
|
if (_state->orc_lazy_read_enabled && _orc_filter == nullptr) { |
|
_orc_filter = std::make_unique<OrcFilterImpl>(this); |
|
} |
|
_state->row_reader = _state->reader->createRowReader( |
|
_state->row_reader_options, |
|
_state->orc_lazy_read_enabled ? _orc_filter.get() : nullptr); |
|
_state->selected_type = &_state->row_reader->getSelectedType(); |
|
DORIS_CHECK(_state->selected_type->getKind() == ::orc::TypeKind::STRUCT); |
|
_state->batch = _state->row_reader->createRowBatch(DEFAULT_ORC_READ_BATCH_SIZE); |
|
_state->orc_lazy_selection_valid = false; |
- Delimited text uses
RuntimeState::batch_size():
|
Status DelimitedTextReader::get_block(Block* file_block, size_t* rows, bool* eof) { |
|
DORIS_CHECK(file_block != nullptr); |
|
DORIS_CHECK(rows != nullptr); |
|
DORIS_CHECK(eof != nullptr); |
|
if (_line_reader == nullptr) { |
|
return Status::InternalError("{} v2 reader is not open", _reader_name); |
|
} |
|
|
|
const auto batch_size = _runtime_state != nullptr ? _runtime_state->batch_size() : 4096; |
|
const auto max_block_bytes = _runtime_state != nullptr |
|
? _runtime_state->preferred_block_size_bytes() |
|
: std::numeric_limits<size_t>::max(); |
|
*rows = 0; |
|
*eof = false; |
|
|
|
{ |
|
auto columns_guard = file_block->mutate_columns_scoped(); |
|
auto& columns = columns_guard.mutable_columns(); |
|
// Delimited text readers are column-pruned but not lazy materialized: all file-local |
|
// columns requested by TableReader are decoded before file-local conjuncts are evaluated. |
|
while (*rows < batch_size && !_line_reader_eof && |
|
Block::columns_byte_size(columns) < max_block_bytes) { |
- JSON also uses
RuntimeState::batch_size():
|
Status JsonReader::get_block(Block* file_block, size_t* rows, bool* eof) { |
|
DORIS_CHECK(file_block != nullptr); |
|
DORIS_CHECK(rows != nullptr); |
|
DORIS_CHECK(eof != nullptr); |
|
if (_json_parser == nullptr || _physical_file_reader == nullptr) { |
|
return Status::InternalError("JSON v2 reader is not open"); |
|
} |
|
|
|
const auto batch_size = _runtime_state->batch_size(); |
|
const auto max_block_bytes = _runtime_state->preferred_block_size_bytes(); |
|
*rows = 0; |
|
*eof = false; |
|
|
|
while (file_block->rows() < batch_size && !_reader_eof && |
|
file_block->bytes() < max_block_bytes) { |
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'):
|
Status NativeReader::_read_next_pblock(std::string* buffer, bool* eof) const { |
|
DORIS_CHECK(buffer != nullptr); |
|
DORIS_CHECK(eof != nullptr); |
|
DORIS_CHECK(_tracing_file_reader != nullptr); |
|
buffer->clear(); |
|
*eof = false; |
|
|
|
if (_current_offset >= _file_size) { |
|
*eof = true; |
|
return Status::OK(); |
|
} |
|
|
|
uint64_t block_len = 0; |
|
Slice len_slice(reinterpret_cast<char*>(&block_len), sizeof(block_len)); |
|
size_t bytes_read = 0; |
|
RETURN_IF_ERROR( |
|
_tracing_file_reader->read_at(_current_offset, len_slice, &bytes_read, _io_ctx.get())); |
|
if (bytes_read == 0) { |
|
*eof = true; |
|
return Status::OK(); |
|
} |
|
if (bytes_read != sizeof(block_len)) { |
|
return Status::InternalError( |
|
"Failed to read native block length from file {}, expect {}, actual {}", |
|
_file_description->path, sizeof(block_len), bytes_read); |
|
} |
|
_current_offset += sizeof(block_len); |
|
if (block_len == 0) { |
|
*eof = (_current_offset >= _file_size); |
|
return Status::OK(); |
|
} |
|
|
|
buffer->assign(block_len, '\0'); |
|
Slice data_slice(buffer->data(), block_len); |
|
bytes_read = 0; |
|
RETURN_IF_ERROR( |
|
_tracing_file_reader->read_at(_current_offset, data_slice, &bytes_read, _io_ctx.get())); |
|
if (bytes_read != block_len) { |
|
return Status::InternalError( |
|
"Failed to read native block body from file {}, expect {}, actual {}", |
|
_file_description->path, block_len, bytes_read); |
|
} |
|
_current_offset += block_len; |
|
*eof = (_current_offset >= _file_size); |
|
return Status::OK(); |
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:
|
void FileScannerV2::_report_condition_cache_profile() { |
|
auto* local_state = static_cast<FileScanLocalState*>(_local_state); |
|
const int64_t hit_count = |
|
_table_reader != nullptr ? _table_reader->condition_cache_hit_count() : 0; |
|
const int64_t hit_delta = hit_count - _reported_condition_cache_hit_count; |
|
if (hit_delta > 0) { |
|
COUNTER_UPDATE(local_state->_condition_cache_hit_counter, hit_delta); |
|
_reported_condition_cache_hit_count = hit_count; |
|
} |
|
const int64_t filtered_rows = _io_ctx != nullptr ? _io_ctx->condition_cache_filtered_rows : 0; |
|
const int64_t filtered_delta = filtered_rows - _reported_condition_cache_filtered_rows; |
|
if (filtered_delta > 0) { |
|
COUNTER_UPDATE(local_state->_condition_cache_filtered_rows_counter, filtered_delta); |
|
_reported_condition_cache_filtered_rows = filtered_rows; |
|
} |
TableReader::condition_cache_hit_count() is non-virtual and returns only that object's counter:
|
// Close the table reader and the currently active file reader. Subclasses that hold additional |
|
// table-format resources should override this and call TableReader::close() first. |
|
virtual Status close() { |
|
if (_data_reader.reader) { |
|
RETURN_IF_ERROR(close_current_reader()); |
|
} |
|
_current_task.reset(); |
|
_current_file_description.reset(); |
|
_remaining_table_level_count = -1; |
|
return Status::OK(); |
|
} |
|
|
|
int64_t condition_cache_hit_count() const { return _condition_cache_hit_count; } |
|
|
|
virtual std::string debug_string() const; |
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:
- Set a fake/file aggregate COUNT result to
batch_size + 5; verify that TableReader returns multiple bounded batches rather than one oversized Block.
- 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.
- 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.
- Create a Native file whose block-length header is larger than the remaining file; verify it returns a corruption error without a large allocation.
- 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?
Code of Conduct
Search before asking
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/masterhead: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 completefile_result.countinto synthetic rows in one call:doris/be/src/format_v2/table_reader.h
Lines 1499 to 1508 in 728d362
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:
doris/be/src/format_v2/table_reader.h
Lines 586 to 605 in 728d362
V1 also uses
CountReader, which emitsmin(remaining_rows, batch_size)rows per batch:doris/be/src/format/count_reader.h
Lines 57 to 69 in 728d362
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:
doris/be/src/format_v2/table_reader.cpp
Lines 642 to 657 in 728d362
The shared
ExternalCacheKeydoes not includefs_name, HDFS cluster identity, S3-compatible endpoint, catalog/resource identity, or another filesystem namespace:doris/be/src/storage/segment/condition_cache.h
Lines 89 to 123 in 728d362
There are two independent stale-hit scenarios:
mtime=0andfile_size=-1but still enables ConditionCache. A mutable Hive/TVF file overwritten at the same path may reuse an old survivor 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_cacheis 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:
doris/be/src/format_v2/parquet/parquet_file_context.cpp
Lines 191 to 207 in 728d362
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:
doris/be/src/exec/scan/file_scanner_v2.cpp
Lines 786 to 815 in 728d362
However:
doris/be/src/format_v2/orc/orc_reader.cpp
Lines 1598 to 1609 in 728d362
RuntimeState::batch_size():doris/be/src/format_v2/delimited_text/delimited_text_reader.cpp
Lines 309 to 330 in 728d362
RuntimeState::batch_size():doris/be/src/format_v2/json/json_reader.cpp
Lines 271 to 285 in 728d362
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_lenfrom the file and immediately executesbuffer->assign(block_len, '\0'):doris/be/src/format_v2/native/native_reader.cpp
Lines 222 to 266 in 728d362
It does not first verify that
block_len <= file_size - current_offsetor that it safely fits the allocation type. A corrupt or truncated Native file can therefore request a huge allocation and triggerstd::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:doris/be/src/exec/scan/file_scanner_v2.cpp
Lines 1030 to 1044 in 728d362
TableReader::condition_cache_hit_count()is non-virtual and returns only that object's counter:doris/be/src/format_v2/table_reader.h
Lines 309 to 323 in 728d362
PaimonHybridReaderandHudiHybridReaderdelegate native splits to childTableReaderobjects, 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, butConditionCacheHitremains underreported.The accessor should be virtual, or hybrid readers should expose the sum of their child counters.
What You Expected?
batch_size.How to Reproduce?
These are static-review findings and have not yet been runtime-tested. Suggested focused reproductions/tests are:
batch_size + 5; verify that TableReader returns multiple bounded batches rather than one oversized Block.ConditionCacheHitincreases 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?
Code of Conduct