From 8933a465d234e4beb0c48ead831f29591d096acd Mon Sep 17 00:00:00 2001 From: Gabriel Date: Mon, 13 Jul 2026 17:10:13 +0800 Subject: [PATCH 01/17] [fix](be) Harden FileScannerV2 schema and reader edge cases ### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: FileScannerV2 treated valid empty splits as scan failures, localized promoted nested struct predicates with mismatched operand types, retained a process-global Parquet page-cache range index, and rejected whole Parquet schemas when unprojected leaves used unsupported logical types. This change skips and counts empty splits, casts localized nested leaves back to table types, scopes range metadata to each file reader, and defers unsupported-type errors until projection. Regression fixtures and commented examples cover empty CSV input, mixed Parquet struct leaf types, repeated multi-file scans, and an unprojected TIME_MILLIS column. ### Release note Fix FileScannerV2 compatibility for empty files, evolved nested predicates, and Parquet files containing unprojected unsupported columns; reduce Parquet page-cache range-index contention and lifetime. ### Check List (For Author) - Test: Regression test / Unit Test - 55 targeted BE ASAN unit tests - test_file_scanner_v2_review_fixes regression suite - Behavior changed: Yes (valid empty splits are skipped; unsupported Parquet logical leaves fail only when projected) - Does this need documentation: No --- be/src/exec/scan/file_scanner_v2.cpp | 34 +++++ be/src/exec/scan/file_scanner_v2.h | 3 + be/src/format_v2/column_mapper.cpp | 15 +++ .../parquet/parquet_column_schema.cpp | 13 +- .../parquet/parquet_file_context.cpp | 97 ++++++-------- .../format_v2/parquet/parquet_file_context.h | 16 +++ be/src/format_v2/parquet/parquet_type.cpp | 3 +- be/src/format_v2/parquet/parquet_type.h | 3 + be/test/exec/scan/file_scanner_v2_test.cpp | 6 + be/test/format_v2/column_mapper_test.cpp | 125 ++++++++++++++++++ .../format_v2/native/native_reader_test.cpp | 5 +- .../parquet/parquet_page_cache_range_test.cpp | 23 ++++ .../parquet/parquet_reader_control_test.cpp | 14 ++ .../format_v2/parquet/parquet_schema_test.cpp | 26 ++-- .../tvf/file_scanner_v2_empty.csv | 0 .../file_scanner_v2_struct_0_bigint.parquet | Bin 0 -> 965 bytes .../tvf/file_scanner_v2_struct_1_int.parquet | Bin 0 -> 923 bytes .../file_scanner_v2_unsupported_time.parquet | Bin 0 -> 689 bytes .../tvf/test_file_scanner_v2_review_fixes.out | 14 ++ .../test_file_scanner_v2_review_fixes.groovy | 84 ++++++++++++ 20 files changed, 405 insertions(+), 76 deletions(-) create mode 100644 regression-test/data/external_table_p0/tvf/file_scanner_v2_empty.csv create mode 100644 regression-test/data/external_table_p0/tvf/file_scanner_v2_struct_0_bigint.parquet create mode 100644 regression-test/data/external_table_p0/tvf/file_scanner_v2_struct_1_int.parquet create mode 100644 regression-test/data/external_table_p0/tvf/file_scanner_v2_unsupported_time.parquet create mode 100644 regression-test/data/external_table_p0/tvf/test_file_scanner_v2_review_fixes.out create mode 100644 regression-test/suites/external_table_p0/tvf/test_file_scanner_v2_review_fixes.groovy diff --git a/be/src/exec/scan/file_scanner_v2.cpp b/be/src/exec/scan/file_scanner_v2.cpp index 99586b5cfa96e2..01575e598fe739 100644 --- a/be/src/exec/scan/file_scanner_v2.cpp +++ b/be/src/exec/scan/file_scanner_v2.cpp @@ -272,6 +272,10 @@ void FileScannerV2::TEST_report_file_cache_profile( bool FileScannerV2::TEST_should_skip_not_found(const Status& status, bool ignore_not_found) { return _should_skip_not_found(status, ignore_not_found); } + +bool FileScannerV2::TEST_should_skip_empty(const Status& status) { + return _should_skip_empty(status); +} #endif bool FileScannerV2::is_supported(const TFileScanRangeParams& params, const TFileRangeDesc& range) { @@ -322,6 +326,8 @@ Status FileScannerV2::init(RuntimeState* state, const VExprContextSPtrs& conjunc RETURN_IF_ERROR(Scanner::init(state, conjuncts)); _get_block_timer = ADD_TIMER_WITH_LEVEL(_local_state->scanner_profile(), "FileScannerV2GetBlockTime", 1); + _empty_file_counter = + ADD_COUNTER_WITH_LEVEL(_local_state->scanner_profile(), "EmptyFileNum", TUnit::UNIT, 1); _not_found_file_counter = ADD_COUNTER_WITH_LEVEL(_local_state->scanner_profile(), "NotFoundFileNum", TUnit::UNIT, 1); _file_counter = @@ -394,6 +400,20 @@ Status FileScannerV2::_get_block_impl(RuntimeState* state, Block* block, bool* e *eof = false; continue; } + if (_should_skip_empty(status)) { + // END_OF_FILE here means the reader discovered a valid split with no data while + // opening or probing it, not that the Scanner has exhausted all splits. Examples + // are a zero-byte CSV with an explicit schema and a Doris Native file containing + // only its 12-byte header. Treat it like V1's empty-file path: finish this range, + // discard partial reader state, and let the loop fetch the next split. + RETURN_IF_ERROR(_table_reader->abort_split()); + COUNTER_UPDATE(_empty_file_counter, 1); + _state->update_num_finished_scan_range(1); + _has_prepared_split = false; + block->clear_column_data(cast_set(_projected_columns.size())); + *eof = false; + continue; + } RETURN_IF_ERROR(status); } if (*eof) { @@ -438,6 +458,16 @@ Status FileScannerV2::_prepare_next_split(bool* eos) { _state->update_num_finished_scan_range(1); continue; } + if (_should_skip_empty(status)) { + // Schema discovery can reach EOF before a split becomes prepared. A header-only Native + // file follows this path, while a reader that discovers emptiness on its first + // get_block() follows the symmetric branch in _get_block_impl(). Both paths must + // advance exactly one scan range and preserve later files in the same scan. + RETURN_IF_ERROR(_table_reader->abort_split()); + COUNTER_UPDATE(_empty_file_counter, 1); + _state->update_num_finished_scan_range(1); + continue; + } RETURN_IF_ERROR(status); if (_table_reader->current_split_pruned()) { _state->update_num_finished_scan_range(1); @@ -540,6 +570,10 @@ bool FileScannerV2::_should_skip_not_found(const Status& status, bool ignore_not return ignore_not_found && status.is(); } +bool FileScannerV2::_should_skip_empty(const Status& status) { + return status.is(); +} + bool FileScannerV2::_should_enable_file_meta_cache() const { return ExecEnv::GetInstance()->file_meta_cache()->enabled() && _split_source->num_scan_ranges() < config::max_external_file_meta_cache_num / 3; diff --git a/be/src/exec/scan/file_scanner_v2.h b/be/src/exec/scan/file_scanner_v2.h index 2bddc5d5e69e6c..b7e5743bfa572d 100644 --- a/be/src/exec/scan/file_scanner_v2.h +++ b/be/src/exec/scan/file_scanner_v2.h @@ -87,6 +87,7 @@ class FileScannerV2 final : public Scanner { static void TEST_report_file_cache_profile( RuntimeProfile* profile, const io::FileCacheStatistics& file_cache_statistics); static bool TEST_should_skip_not_found(const Status& status, bool ignore_not_found); + static bool TEST_should_skip_empty(const Status& status); #endif FileScannerV2(RuntimeState* state, FileScanLocalState* parent, int64_t limit, @@ -121,6 +122,7 @@ class FileScannerV2 final : public Scanner { Status _prepare_table_reader_split(const TFileRangeDesc& range, std::map partition_values); static bool _should_skip_not_found(const Status& status, bool ignore_not_found); + static bool _should_skip_empty(const Status& status); bool _should_enable_file_meta_cache() const; std::optional _create_global_rowid_context( const TFileRangeDesc& range) const; @@ -181,6 +183,7 @@ class FileScannerV2 final : public Scanner { ShardedKVCache* _kv_cache = nullptr; RuntimeProfile::Counter* _get_block_timer = nullptr; + RuntimeProfile::Counter* _empty_file_counter = nullptr; RuntimeProfile::Counter* _not_found_file_counter = nullptr; RuntimeProfile::Counter* _file_counter = nullptr; RuntimeProfile::Counter* _file_read_bytes_counter = nullptr; diff --git a/be/src/format_v2/column_mapper.cpp b/be/src/format_v2/column_mapper.cpp index 313bbf376f860e..1c2d96f7edbded 100644 --- a/be/src/format_v2/column_mapper.cpp +++ b/be/src/format_v2/column_mapper.cpp @@ -876,6 +876,7 @@ static VExprSPtr rewrite_table_expr_to_file_expr( return expr; } if (is_struct_element_expr(expr)) { + const auto table_leaf_type = expr->data_type(); if (!rewrite_struct_element_path_to_file_expr(expr, filter_mappings, global_to_file_slot, rewrite_context)) { // The scanner still evaluates the original table-level conjunct after TableReader @@ -885,6 +886,20 @@ static VExprSPtr rewrite_table_expr_to_file_expr( // parents such as `element_at(element_at(map_values(m), 1), 'field')`; only direct // slot-rooted struct chains are supported here. *can_localize = false; + return expr; + } + DORIS_CHECK(table_leaf_type != nullptr); + DORIS_CHECK(expr->data_type() != nullptr); + if (!expr->data_type()->equals(*table_leaf_type)) { + // Path localization changes the leaf to the physical file type. For example, after an + // Iceberg evolution from STRUCT to STRUCT, the localized old-file + // predicate is initially `element_at(file_col, 'a')::INT = 10::BIGINT`. Cast only the + // leaf back to BIGINT so the comparison has matching operands without forcing a cast + // of the entire evolved struct (whose children may also have been added or reordered). + auto cast_expr = Cast::create_shared(table_leaf_type); + cast_expr->add_child(expr); + rewrite_context->add_created_expr(cast_expr); + return cast_expr; } return expr; } diff --git a/be/src/format_v2/parquet/parquet_column_schema.cpp b/be/src/format_v2/parquet/parquet_column_schema.cpp index b42d47987a54cb..1cdfed80bd273b 100644 --- a/be/src/format_v2/parquet/parquet_column_schema.cpp +++ b/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; + } if (column_schema->type == nullptr) { - if (!column_schema->type_descriptor.unsupported_reason.empty()) { - return Status::NotSupported("Unsupported parquet column '{}': {}", node.name(), - column_schema->type_descriptor.unsupported_reason); - } return Status::NotSupported("Unsupported parquet column type for column {}", node.name()); } diff --git a/be/src/format_v2/parquet/parquet_file_context.cpp b/be/src/format_v2/parquet/parquet_file_context.cpp index 52151c7942af2b..4eb2e553d34c92 100644 --- a/be/src/format_v2/parquet/parquet_file_context.cpp +++ b/be/src/format_v2/parquet/parquet_file_context.cpp @@ -27,7 +27,6 @@ #include #include #include -#include #include #include "common/check.h" @@ -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) { + const auto it = std::lower_bound(_ranges.begin(), _ranges.end(), range, page_cache_range_less); + if (it == _ranges.end() || it->offset != range.offset || it->size != range.size) { + _ranges.insert(it, range); + } +} + +void ParquetPageCacheRangeIndex::erase(ParquetPageCacheRange range) { + const auto it = std::lower_bound(_ranges.begin(), _ranges.end(), range, page_cache_range_less); + if (it != _ranges.end() && it->offset == range.offset && it->size == range.size) { + _ranges.erase(it); + } +} + +void ParquetPageCacheRangeIndex::clear() { + std::vector().swap(_ranges); +} + std::vector plan_page_cache_range_read( int64_t position, int64_t nbytes, const std::vector& cached_ranges) { if (position < 0 || nbytes <= 0) { @@ -133,61 +158,6 @@ bool should_use_merge_range_reader(const std::vector& ran namespace { -// StoragePageCache only supports exact-key lookup. Keep lightweight range metadata here so later -// Arrow ReadAt requests can reuse cached bytes when their requested ranges are subsets of, or are -// fully covered by, previously cached ranges. Stale metadata is pruned on lookup. -std::mutex cached_page_range_index_mutex; -std::unordered_map> cached_page_range_index; -constexpr size_t MAX_CACHED_PAGE_RANGE_FILES = 4096; -constexpr size_t MAX_CACHED_PAGE_RANGES_PER_FILE = 65536; - -void register_cached_page_range(const std::string& file_key, int64_t position, int64_t nbytes) { - DORIS_CHECK(nbytes > 0); - std::lock_guard lock(cached_page_range_index_mutex); - if (cached_page_range_index.find(file_key) == cached_page_range_index.end() && - cached_page_range_index.size() >= MAX_CACHED_PAGE_RANGE_FILES) { - cached_page_range_index.erase(cached_page_range_index.begin()); - } - auto& ranges = cached_page_range_index[file_key]; - auto it = std::find_if(ranges.begin(), ranges.end(), [&](const ParquetPageCacheRange& range) { - return range.offset == position && range.size == nbytes; - }); - if (it == ranges.end()) { - if (ranges.size() >= MAX_CACHED_PAGE_RANGES_PER_FILE) { - ranges.erase(ranges.begin()); - } - ranges.push_back(ParquetPageCacheRange {position, nbytes}); - } -} - -void unregister_cached_page_range(const std::string& file_key, - const ParquetPageCacheRange& stale_range) { - std::lock_guard lock(cached_page_range_index_mutex); - auto it = cached_page_range_index.find(file_key); - if (it == cached_page_range_index.end()) { - return; - } - auto& ranges = it->second; - ranges.erase(std::remove_if(ranges.begin(), ranges.end(), - [&](const ParquetPageCacheRange& range) { - return range.offset == stale_range.offset && - range.size == stale_range.size; - }), - ranges.end()); - if (ranges.empty()) { - cached_page_range_index.erase(it); - } -} - -std::vector cached_page_ranges_for_file(const std::string& file_key) { - std::lock_guard lock(cached_page_range_index_mutex); - auto it = cached_page_range_index.find(file_key); - if (it == cached_page_range_index.end()) { - return {}; - } - return it->second; -} - std::string build_page_cache_file_key(const io::FileReader& file_reader, const io::FileDescription& file_description) { const int64_t mtime = @@ -228,6 +198,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(); + std::vector().swap(_page_cache_ranges); _closed = true; } return arrow::Status::OK(); @@ -393,7 +369,7 @@ class DorisRandomAccessFile final : public arrow::io::RandomAccessFile { if (!StoragePageCache::instance()->lookup( page_cache_key(cached_range.offset, cached_range.size), &handle, segment_v2::DATA_PAGE)) { - unregister_cached_page_range(_page_cache_file_key, cached_range); + _cached_page_range_index.erase(cached_range); return false; } Slice cached = handle.data(); @@ -406,8 +382,8 @@ class DorisRandomAccessFile final : public arrow::io::RandomAccessFile { } bool try_read_from_cached_ranges(int64_t position, int64_t nbytes, void* out) { - auto plan = detail::plan_page_cache_range_read( - position, nbytes, cached_page_ranges_for_file(_page_cache_file_key)); + auto plan = detail::plan_page_cache_range_read(position, nbytes, + _cached_page_range_index.ranges()); if (plan.empty()) { return false; } @@ -458,7 +434,7 @@ class DorisRandomAccessFile final : public arrow::io::RandomAccessFile { PageCacheHandle handle; StoragePageCache::instance()->insert(page_cache_key(position, nbytes), page, &handle, segment_v2::DATA_PAGE); - register_cached_page_range(_page_cache_file_key, position, nbytes); + _cached_page_range_index.insert(ParquetPageCacheRange {.offset = position, .size = nbytes}); ++_page_cache_stats.write_count; ++_page_cache_stats.compressed_write_count; } @@ -516,6 +492,7 @@ class DorisRandomAccessFile final : public arrow::io::RandomAccessFile { std::string _page_cache_file_key; mutable std::mutex _page_cache_mutex; std::vector _page_cache_ranges; + detail::ParquetPageCacheRangeIndex _cached_page_range_index; ParquetPageCacheStats _page_cache_stats; }; diff --git a/be/src/format_v2/parquet/parquet_file_context.h b/be/src/format_v2/parquet/parquet_file_context.h index 0dca52244957d7..80a45aa0d273d0 100644 --- a/be/src/format_v2/parquet/parquet_file_context.h +++ b/be/src/format_v2/parquet/parquet_file_context.h @@ -66,6 +66,22 @@ struct ParquetPageCacheStats { namespace detail { +class ParquetPageCacheRangeIndex { +public: + void insert(ParquetPageCacheRange range); + void erase(ParquetPageCacheRange range); + void clear(); + + const std::vector& ranges() const { return _ranges; } + +private: + // This index belongs to one DorisRandomAccessFile. Thus two files reading [0, 4KB) never + // contend on or observe the same range metadata, and closing the reader releases all entries. + // StoragePageCache remains process-wide; only its lightweight exact-range directory is scoped + // to the reader that inserted and can validate those entries. + std::vector _ranges; +}; + // Build the copy plan for a ReadAt(position, nbytes) request from the range metadata of // previously cached entries. // StoragePageCache cannot do range lookup by itself; it can only lookup an exact key. The diff --git a/be/src/format_v2/parquet/parquet_type.cpp b/be/src/format_v2/parquet/parquet_type.cpp index d35181d0397178..8411d53f0fba91 100644 --- a/be/src/format_v2/parquet/parquet_type.cpp +++ b/be/src/format_v2/parquet/parquet_type.cpp @@ -293,6 +293,7 @@ ParquetTypeDescriptor resolve_parquet_type(const ::parquet::ColumnDescriptor* co result.physical_type = column->physical_type(); result.converted_type = column->converted_type(); result.fixed_length = column->type_length(); + result.physical_doris_type = physical_type_to_doris_type(column); if (auto logical_type = logical_type_to_doris_type(column, &result); logical_type != nullptr) { result.doris_type = logical_type; @@ -306,7 +307,7 @@ ParquetTypeDescriptor resolve_parquet_type(const ::parquet::ColumnDescriptor* co result.doris_type = nullptr; result.supports_record_reader = false; } else { - result.doris_type = physical_type_to_doris_type(column); + result.doris_type = result.physical_doris_type; if (result.physical_type == ::parquet::Type::INT96) { result.extra_type_info = ParquetExtraTypeInfo::IMPALA_TIMESTAMP; } diff --git a/be/src/format_v2/parquet/parquet_type.h b/be/src/format_v2/parquet/parquet_type.h index 5d21aae6bae092..a4d99abc0e982a 100644 --- a/be/src/format_v2/parquet/parquet_type.h +++ b/be/src/format_v2/parquet/parquet_type.h @@ -54,6 +54,9 @@ enum class ParquetTimeUnit { // ============================================================================ struct ParquetTypeDescriptor { DataTypePtr doris_type; + // Physical fallback used only to keep file schema construction alive when the logical type is + // unsupported. Column reader creation still rejects unsupported_reason before decoding. + DataTypePtr physical_doris_type; ParquetExtraTypeInfo extra_type_info = ParquetExtraTypeInfo::NONE; ParquetTimeUnit time_unit = ParquetTimeUnit::UNKNOWN; ::parquet::Type::type physical_type = ::parquet::Type::UNDEFINED; diff --git a/be/test/exec/scan/file_scanner_v2_test.cpp b/be/test/exec/scan/file_scanner_v2_test.cpp index 34b51a5b40da34..a18ae9c6d5bd85 100644 --- a/be/test/exec/scan/file_scanner_v2_test.cpp +++ b/be/test/exec/scan/file_scanner_v2_test.cpp @@ -707,6 +707,12 @@ TEST(FileScannerV2Test, NotFoundIsSkippedOnlyWhenConfigured) { EXPECT_FALSE(FileScannerV2::TEST_should_skip_not_found(Status::OK(), true)); } +TEST(FileScannerV2Test, EndOfFileIsSkippedAsEmptySplit) { + EXPECT_TRUE(FileScannerV2::TEST_should_skip_empty(Status::EndOfFile("empty file"))); + EXPECT_FALSE(FileScannerV2::TEST_should_skip_empty(Status::InternalError("read failed"))); + EXPECT_FALSE(FileScannerV2::TEST_should_skip_empty(Status::OK())); +} + // Scenario: partition slots are identified from the explicit FE category when present, otherwise // from the legacy is_file_slot flag. Scanner-generated rowid columns must never be treated as // partition columns even if FE marks them as non-file slots. diff --git a/be/test/format_v2/column_mapper_test.cpp b/be/test/format_v2/column_mapper_test.cpp index 3bca3c4152c888..738e19261c216c 100644 --- a/be/test/format_v2/column_mapper_test.cpp +++ b/be/test/format_v2/column_mapper_test.cpp @@ -28,6 +28,7 @@ #include "common/consts.h" #include "core/assert_cast.h" #include "core/block/block.h" +#include "core/column/column_struct.h" #include "core/column/column_vector.h" #include "core/data_type/data_type_array.h" #include "core/data_type/data_type_decimal.h" @@ -38,6 +39,7 @@ #include "core/data_type/data_type_struct.h" #include "core/data_type/data_type_timestamptz.h" #include "core/data_type/data_type_varbinary.h" +#include "exprs/vectorized_fn_call.h" #include "exprs/vexpr.h" #include "exprs/vexpr_context.h" #include "exprs/vin_predicate.h" @@ -316,6 +318,38 @@ class TestFunctionExpr final : public VExpr { std::string _expr_name; }; +class ExecutableStructElementExpr final : public VExpr { +public: + explicit ExecutableStructElementExpr(DataTypePtr child_type) + : VExpr(std::move(child_type), false) { + set_node_type(TExprNodeType::FUNCTION_CALL); + TFunctionName fn_name; + fn_name.__set_function_name(_expr_name); + _fn.__set_name(fn_name); + } + + const std::string& expr_name() const override { return _expr_name; } + + Status clone_node(VExprSPtr* cloned_expr) const override { + DORIS_CHECK(cloned_expr != nullptr); + *cloned_expr = std::make_shared(data_type()); + return Status::OK(); + } + + Status execute_column_impl(VExprContext* context, const Block* block, const Selector* selector, + size_t count, ColumnPtr& result_column) const override { + ColumnPtr struct_column; + RETURN_IF_ERROR( + get_child(0)->execute_column(context, block, selector, count, struct_column)); + const auto& input = assert_cast(*struct_column); + result_column = input.get_column_ptr(0); + return Status::OK(); + } + +private: + const std::string _expr_name = "element_at"; +}; + VExprSPtr table_slot(int slot_id, int column_id, DataTypePtr type, const std::string& name) { return VSlotRef::create_shared(slot_id, column_id, -1, std::move(type), name); } @@ -340,6 +374,40 @@ VExprSPtr element_at(const VExprSPtr& parent, DataTypePtr child_type, return expr; } +VExprSPtr executable_struct_element(const VExprSPtr& parent, DataTypePtr child_type, + const std::string& child_name) { + auto expr = std::make_shared(std::move(child_type)); + expr->add_child(parent); + expr->add_child(literal(str(), Field::create_field(child_name))); + return expr; +} + +VExprSPtr executable_binary_predicate(TExprOpcode::type opcode, const VExprSPtr& left, + const VExprSPtr& right) { + const auto result_type = u8(); + TFunctionName fn_name; + fn_name.__set_function_name(opcode == TExprOpcode::GT ? "gt" : "eq"); + TFunction fn; + fn.__set_name(fn_name); + fn.__set_binary_type(TFunctionBinaryType::BUILTIN); + fn.__set_arg_types({left->data_type()->to_thrift(), right->data_type()->to_thrift()}); + fn.__set_ret_type(result_type->to_thrift()); + fn.__set_has_var_args(false); + + TExprNode node; + node.__set_node_type(TExprNodeType::BINARY_PRED); + node.__set_opcode(opcode); + node.__set_type(result_type->to_thrift()); + node.__set_fn(fn); + node.__set_num_children(2); + node.__set_is_nullable(false); + + auto expr = VectorizedFnCall::create_shared(node); + expr->add_child(left); + expr->add_child(right); + return expr; +} + VExprSPtr array_element_at(const VExprSPtr& parent, DataTypePtr child_type, int64_t ordinal) { auto expr = std::make_shared("element_at", std::move(child_type)); expr->add_child(parent); @@ -2993,6 +3061,63 @@ TEST(ColumnMapperScanRequestTest, NestedElementAtConjunctUsesFileChildTypeForRen EXPECT_EQ(localized_parent_type->get_element_name(1), "bb"); } +// Scenario: Iceberg promotes a nested struct leaf from INT to BIGINT while an old file still +// stores INT. The localized element_at must be cast back to the table leaf type so the comparison +// is prepared and executed with matching operand types. +TEST_F(ColumnMapperCastTest, NestedElementAtConjunctCastsPromotedFileLeafToTableType) { + const auto file_int_type = i32(); + const auto table_bigint_type = i64(); + + auto table_b = field_id_col("b", 11, table_bigint_type); + auto table_struct = struct_col("s", 10, {table_b}); + auto file_b = field_id_col("b", 11, file_int_type, 0); + auto file_struct = struct_col("s", 10, {file_b}, 5); + + auto table_leaf = executable_struct_element( + table_slot(0, 0, table_struct.type, table_struct.name), table_bigint_type, "b"); + auto filter_expr = executable_binary_predicate( + TExprOpcode::GT, table_leaf, + literal(table_bigint_type, Field::create_field(15))); + TableFilter filter {.conjunct = VExprContext::create_shared(filter_expr), + .global_indices = {GlobalIndex(0)}}; + + TableColumnMapper mapper({.mode = TableColumnMappingMode::BY_FIELD_ID}); + ASSERT_TRUE(mapper.create_mapping({table_struct}, {}, {file_struct}).ok()); + + FileScanRequest request; + ASSERT_TRUE(mapper.create_scan_request({filter}, {table_struct}, &request, &state).ok()); + ASSERT_EQ(request.conjuncts.size(), 1); + const auto& localized_root = request.conjuncts[0]->root(); + ASSERT_EQ(localized_root->get_num_children(), 2); + const auto& localized_cast = localized_root->children()[0]; + ASSERT_NE(dynamic_cast(localized_cast.get()), nullptr); + EXPECT_TRUE(localized_cast->data_type()->equals(*table_bigint_type)); + ASSERT_EQ(localized_cast->get_num_children(), 1); + EXPECT_EQ(localized_cast->children()[0]->expr_name(), "element_at"); + EXPECT_TRUE(localized_cast->children()[0]->data_type()->equals(*file_int_type)); + + auto values = ColumnInt32::create(); + values->insert_value(10); + values->insert_value(20); + MutableColumns children; + children.push_back(std::move(values)); + Block block; + block.insert({ColumnStruct::create(std::move(children)), mapper.mappings()[0].file_type, "s"}); + + auto* conjunct = request.conjuncts[0].get(); + auto status = conjunct->prepare(&state, RowDescriptor()); + ASSERT_TRUE(status.ok()) << status; + status = conjunct->open(&state); + ASSERT_TRUE(status.ok()) << status; + IColumn::Filter result(block.rows(), 1); + bool can_filter_all = false; + status = conjunct->execute_filter(&block, result.data(), block.rows(), false, &can_filter_all); + ASSERT_TRUE(status.ok()) << status; + EXPECT_FALSE(can_filter_all); + EXPECT_EQ(result, IColumn::Filter({0, 1})); + conjunct->close(); +} + // Scenario: output projection reads one struct child while the row filter reads a different nested // struct child. File-local conjunct rewrite must use the merged scan projection type. In the SQL // shape below, `SELECT element_at(s, 'c') WHERE element_at(element_at(s, 'b'), 'cc') LIKE ...` diff --git a/be/test/format_v2/native/native_reader_test.cpp b/be/test/format_v2/native/native_reader_test.cpp index aaa7aa90e0681e..6b0c9ab27b90a8 100644 --- a/be/test/format_v2/native/native_reader_test.cpp +++ b/be/test/format_v2/native/native_reader_test.cpp @@ -295,7 +295,7 @@ TEST(NativeV2ReaderTest, RejectsInvalidHeaderAndEmptyFile) { static_cast(io::global_local_filesystem()->delete_file(empty_path)); } -TEST(NativeV2ReaderTest, RejectsUnsupportedVersionAndHeaderOnlyFile) { +TEST(NativeV2ReaderTest, RejectsUnsupportedVersionAndReportsHeaderOnlyFileAsEmpty) { std::filesystem::create_directories("./log"); RuntimeState state; RuntimeProfile profile("native_v2_reader_header_boundary_test"); @@ -322,7 +322,8 @@ TEST(NativeV2ReaderTest, RejectsUnsupportedVersionAndHeaderOnlyFile) { auto header_only_reader = create_reader(header_only_path, &state, &profile); ASSERT_TRUE(header_only_reader->init(&state).ok()); std::vector schema; - EXPECT_FALSE(header_only_reader->get_schema(&schema).ok()); + const auto header_only_status = header_only_reader->get_schema(&schema); + EXPECT_TRUE(header_only_status.is()) << header_only_status; static_cast(io::global_local_filesystem()->delete_file(header_only_path)); } diff --git a/be/test/format_v2/parquet/parquet_page_cache_range_test.cpp b/be/test/format_v2/parquet/parquet_page_cache_range_test.cpp index fac89b31c422d8..f06c3fd4660c09 100644 --- a/be/test/format_v2/parquet/parquet_page_cache_range_test.cpp +++ b/be/test/format_v2/parquet/parquet_page_cache_range_test.cpp @@ -115,6 +115,29 @@ TEST(ParquetPageCacheRangeTest, InvalidRequestMisses) { EXPECT_TRUE(detail::plan_page_cache_range_read(100, -1, cached_ranges).empty()); } +TEST(ParquetPageCacheRangeTest, PerFileRangeIndexSortsDeduplicatesAndClears) { + detail::ParquetPageCacheRangeIndex index; + index.insert({200, 20}); + index.insert({100, 30}); + index.insert({100, 10}); + index.insert({100, 30}); + + ASSERT_EQ(index.ranges().size(), 3); + EXPECT_EQ(index.ranges()[0].offset, 100); + EXPECT_EQ(index.ranges()[0].size, 10); + EXPECT_EQ(index.ranges()[1].offset, 100); + EXPECT_EQ(index.ranges()[1].size, 30); + EXPECT_EQ(index.ranges()[2].offset, 200); + + index.erase({100, 30}); + ASSERT_EQ(index.ranges().size(), 2); + EXPECT_EQ(index.ranges()[0].size, 10); + EXPECT_EQ(index.ranges()[1].offset, 200); + + index.clear(); + EXPECT_TRUE(index.ranges().empty()); +} + TEST(ParquetPageCacheRangeTest, ValidPrefetchRangesSkipInvalidAndOverflowRanges) { const std::vector ranges = { {100, 50}, diff --git a/be/test/format_v2/parquet/parquet_reader_control_test.cpp b/be/test/format_v2/parquet/parquet_reader_control_test.cpp index 2fd85cefd49ac9..36b7cebdaa9cb9 100644 --- a/be/test/format_v2/parquet/parquet_reader_control_test.cpp +++ b/be/test/format_v2/parquet/parquet_reader_control_test.cpp @@ -1206,6 +1206,20 @@ TEST(ParquetColumnReaderFactoryTest, RejectsInvalidLeafIdBeforeCreatingRecordRea EXPECT_NE(status.to_string().find("Invalid parquet leaf column id"), std::string::npos); } +TEST(ParquetColumnReaderFactoryTest, RejectsProjectedUnsupportedLogicalType) { + ParquetColumnSchema schema = int64_schema("unsupported_time"); + schema.kind = ParquetColumnSchemaKind::PRIMITIVE; + schema.type_descriptor.unsupported_reason = + "Parquet TIME with isAdjustedToUTC=true is not supported"; + + ParquetColumnReaderFactory factory(nullptr, 1); + std::unique_ptr reader; + const auto status = factory.create(schema, &reader); + EXPECT_FALSE(status.ok()); + EXPECT_NE(status.to_string().find(schema.type_descriptor.unsupported_reason), + std::string::npos); +} + TEST(ParquetColumnReaderFactoryTest, RejectsStructInvalidAndEmptyProjection) { auto schema = struct_schema_for_projection(); ParquetColumnReaderFactory factory(nullptr, 0); diff --git a/be/test/format_v2/parquet/parquet_schema_test.cpp b/be/test/format_v2/parquet/parquet_schema_test.cpp index e620ed718efbf2..9f78735bfb560a 100644 --- a/be/test/format_v2/parquet/parquet_schema_test.cpp +++ b/be/test/format_v2/parquet/parquet_schema_test.cpp @@ -414,7 +414,7 @@ TEST(ParquetSchemaTest, BuildEntryValidatesNullPointerAndEmptyRoot) { EXPECT_TRUE(fields.empty()); } -TEST(ParquetSchemaTest, RejectInvalidListMapAndUnsupportedTime) { +TEST(ParquetSchemaTest, RejectInvalidListMapAndPreserveUnsupportedTime) { const auto bad_list = ::parquet::schema::GroupNode::Make( "bad_list", ::parquet::Repetition::OPTIONAL, {::parquet::schema::PrimitiveNode::Make("item", ::parquet::Repetition::OPTIONAL, @@ -432,9 +432,11 @@ TEST(ParquetSchemaTest, RejectInvalidListMapAndUnsupportedTime) { const auto converted_time = ::parquet::schema::PrimitiveNode::Make( "time_ms", ::parquet::Repetition::REQUIRED, ::parquet::Type::INT32, ::parquet::ConvertedType::TIME_MILLIS); - const auto status = build_status({converted_time}); - EXPECT_FALSE(status.ok()); - EXPECT_NE(status.to_string().find("Parquet TIME with isAdjustedToUTC=true is not supported"), + const auto fields = build_fields({converted_time}); + ASSERT_EQ(fields.size(), 1); + EXPECT_EQ(remove_nullable(fields[0]->type)->get_primitive_type(), TYPE_INT); + EXPECT_NE(fields[0]->type_descriptor.unsupported_reason.find( + "Parquet TIME with isAdjustedToUTC=true is not supported"), std::string::npos); } @@ -513,15 +515,21 @@ TEST(ParquetSchemaTest, RejectAdditionalInvalidListAndMapLayouts) { EXPECT_FALSE(build_status({repeated_map}).ok()); } -TEST(ParquetSchemaTest, LogicalUtcTimeIsRejected) { +TEST(ParquetSchemaTest, LogicalUtcTimeIsPreservedForProjection) { const auto adjusted_time = ::parquet::schema::PrimitiveNode::Make( "time_ms", ::parquet::Repetition::REQUIRED, ::parquet::LogicalType::Time(true, ::parquet::LogicalType::TimeUnit::MILLIS), ::parquet::Type::INT32); - const auto status = build_status({adjusted_time}); - EXPECT_FALSE(status.ok()); - EXPECT_NE(status.to_string().find("Parquet TIME with isAdjustedToUTC=true is not supported"), - std::string::npos); + const auto supported_value = ::parquet::schema::PrimitiveNode::Make( + "value", ::parquet::Repetition::REQUIRED, ::parquet::Type::INT64); + const auto row = ::parquet::schema::GroupNode::Make("row", ::parquet::Repetition::OPTIONAL, + {adjusted_time, supported_value}); + const auto fields = build_fields({row}); + ASSERT_EQ(fields.size(), 1); + ASSERT_EQ(fields[0]->children.size(), 2); + EXPECT_EQ(remove_nullable(fields[0]->children[0]->type)->get_primitive_type(), TYPE_INT); + EXPECT_FALSE(fields[0]->children[0]->type_descriptor.unsupported_reason.empty()); + EXPECT_EQ(remove_nullable(fields[0]->children[1]->type)->get_primitive_type(), TYPE_BIGINT); } } // namespace doris::format::parquet diff --git a/regression-test/data/external_table_p0/tvf/file_scanner_v2_empty.csv b/regression-test/data/external_table_p0/tvf/file_scanner_v2_empty.csv new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/regression-test/data/external_table_p0/tvf/file_scanner_v2_struct_0_bigint.parquet b/regression-test/data/external_table_p0/tvf/file_scanner_v2_struct_0_bigint.parquet new file mode 100644 index 0000000000000000000000000000000000000000..22eff58b7a23895a7e1d7f2e4323d6f5b1f9529d GIT binary patch literal 965 zcmaJ=-AWrl7@Zx5B}i|C&N2ggksB8a(Q2usl=b4td~q|Z`(h<;}#o1~a_NM_FWbIzRkVB6H)(?Z8OZfM0I)M0Ft6hask@E(b+rD$V= zQz)tcMIh!hbYqEJd&*fXQ(N}{A}g{BF`SxOA73G*I29DW6!b)-Y;4m+#9G?TP*}8U&PI39T~bT`I}sbR zT~-q5v(P3&u>}YzC$xnNgTiUu2hn|yd=FByVjBOP(O>efc=R)UZ`AAe=3akFHoF+l zqhYO5i+-#{Dyi`yY;2xTxBK?}8$UhsAfIEE!;FYJ{Kq;aDk&S%5z&yELzm0A;S8ur zbb^!P>f{K}cS4AcKe6a*2KjjlF_U=ZZiim(u6pOS;6B0{<|_(@`bwgfH*FDqHT;A@Xcye(6l_82M#A z+}gUjvn%K*)FEjaaq3|Om9Zx&-E8CcXoGn RZ5j*Vf8sZPfNlPO{{Z@npC14K literal 0 HcmV?d00001 diff --git a/regression-test/data/external_table_p0/tvf/file_scanner_v2_struct_1_int.parquet b/regression-test/data/external_table_p0/tvf/file_scanner_v2_struct_1_int.parquet new file mode 100644 index 0000000000000000000000000000000000000000..5a9ca0baa47e892df900ff12a74417c3db63f91f GIT binary patch literal 923 zcmaJ=%TC)s6ulls7DC;y7;8rIA{(M00fnkmRTUQ3!3_vOlT=kV-8gBKra%+I!xBrr zpr6-|sQLvgxM!Ti&>%+ko%=fHKBQ@JXUC9y(BWC+9PXqRa{Db z9UzFbMQ}~LRJLJ)yP@b3WN4Uqts)*~>22pH|Y?UD^Idq4W;nbBm_nzCy6 zopzO7>W$83t{^KyL6;)!Vpp$HY?NCI$syf{H@b~(tIPElCwH;5D@;^h4_(y{UlP?& zFv#XycY%T2_ioDDWva3Nl%)v1tcf+ z5`3vI^~Kw{$mYpS*T>u7l@8|kh6g1+zrcGC)MNgdBKD@vR`5_$5JO-XmKX tXOoNT{?%r0Jl@PElTlu|=}#_)qhFObZ@0F$wp~3YDn9xn53D86@&QX@oumK& literal 0 HcmV?d00001 diff --git a/regression-test/data/external_table_p0/tvf/file_scanner_v2_unsupported_time.parquet b/regression-test/data/external_table_p0/tvf/file_scanner_v2_unsupported_time.parquet new file mode 100644 index 0000000000000000000000000000000000000000..fe1daf97c37e4e6cfa72a93adb57f9e3ac952a20 GIT binary patch literal 689 zcmaJ<%TB^T6rI*dW#K|hrfHKd*wAPM1f%g0Hd6vYjfxmU!p0yph=EeeDPi6a8JMj&PD z%__SBp6M?a2SpK&f~Z@@cVB& zj)O}f&27idxXcl{oi>lzNJfb9BBy^b|??vP^X>qEae}j8J$AZVK(HH*E*?ezS6xZ#4>iLdXhQ zR6oK4`GB7)`C5RKprxdmJaxX*kNt#AbiglsgFlP?D%!n@DljV*wxZXGKhCzpFx!fv f;Vd=vqfu`dqz;dA`CQ)4;D6IE-^XLC;j#Vz9r1=H literal 0 HcmV?d00001 diff --git a/regression-test/data/external_table_p0/tvf/test_file_scanner_v2_review_fixes.out b/regression-test/data/external_table_p0/tvf/test_file_scanner_v2_review_fixes.out new file mode 100644 index 00000000000000..746fd355f15392 --- /dev/null +++ b/regression-test/data/external_table_p0/tvf/test_file_scanner_v2_review_fixes.out @@ -0,0 +1,14 @@ +-- This file is automatically generated. You should know what you did if you want to edit this +-- !empty_csv -- + +-- !struct_leaf_promotion_cold -- +1 10 100 +3 10 300 + +-- !struct_leaf_promotion_warm -- +1 10 100 +3 10 300 + +-- !unprojected_unsupported_time -- +1 +2 diff --git a/regression-test/suites/external_table_p0/tvf/test_file_scanner_v2_review_fixes.groovy b/regression-test/suites/external_table_p0/tvf/test_file_scanner_v2_review_fixes.groovy new file mode 100644 index 00000000000000..ac8359d980a023 --- /dev/null +++ b/regression-test/suites/external_table_p0/tvf/test_file_scanner_v2_review_fixes.groovy @@ -0,0 +1,84 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +suite("test_file_scanner_v2_review_fixes", "p0,external") { + def backends = sql "show backends" + assertTrue(backends.size() > 0) + def backendId = backends[0][0] + def dataPath = context.config.dataPath + "/external_table_p0/tvf" + // A developer may point this at fixtures already visible to every BE (for example, a shared + // checkout mounted at /tmp). CI leaves it unset and exercises the normal per-BE distribution. + def predeployedFixturePath = context.config.otherConfigs.get("fileScannerV2FixturePath") + def remotePath = predeployedFixturePath ?: "/tmp/file_scanner_v2_review_fixes" + def fixtureNames = [ + "file_scanner_v2_empty.csv", + "file_scanner_v2_struct_0_bigint.parquet", + "file_scanner_v2_struct_1_int.parquet", + "file_scanner_v2_unsupported_time.parquet" + ] + + if (predeployedFixturePath == null) { + mkdirRemotePathOnAllBE("root", remotePath) + for (def backend : backends) { + for (def fixtureName : fixtureNames) { + scpFiles("root", backend[1], "${dataPath}/${fixtureName}", remotePath, false) + } + } + } + + sql "set enable_file_scanner_v2 = true" + + // A zero-byte CSV is a valid zero-row split when the schema is supplied explicitly. The EOF + // raised during reader preparation must skip this split instead of failing the query. + order_qt_empty_csv """ + SELECT id, name + FROM local( + "file_path" = "${remotePath}/file_scanner_v2_empty.csv", + "backend_id" = "${backendId}", + "format" = "csv", + "csv_schema" = "id:int;name:string") + ORDER BY id + """ + + // The first file defines STRUCT, while the second stores a as INT. The predicate is + // localized independently for each split, so the INT leaf must be cast back to BIGINT before + // comparison. Repeating the multi-file read also exercises per-reader page-cache range state: + // closing one file must not leak its range directory into the next file or the warm scan. + def evolvedStructQuery = """ + SELECT id, col.a, col.b + FROM local( + "file_path" = "${remotePath}/file_scanner_v2_struct_*.parquet", + "backend_id" = "${backendId}", + "format" = "parquet") + WHERE col.a = CAST(10 AS BIGINT) + ORDER BY id + """ + order_qt_struct_leaf_promotion_cold evolvedStructQuery + order_qt_struct_leaf_promotion_warm evolvedStructQuery + + // TIME_MILLIS is intentionally unsupported by the record reader. It must not prevent schema + // construction when only the supported id column is projected. + order_qt_unprojected_unsupported_time """ + SELECT id + FROM local( + "file_path" = "${remotePath}/file_scanner_v2_unsupported_time.parquet", + "backend_id" = "${backendId}", + "format" = "parquet") + ORDER BY id + """ + +} From 493bfddcb8493944a4a81dcdd6fa9e41853c9441 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Tue, 14 Jul 2026 10:06:32 +0800 Subject: [PATCH 02/17] [fix](be) Address FileScannerV2 review edge cases ### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Stopped scans could be accounted as empty files, unsupported Parquet logical leaves could reach metadata pruning before rejection, and the reader-local page-cache range index lost warm non-exact reuse while growing without a bound. Gate empty EOF handling on scanner stop state, validate requested leaves before pruning and aggregate metadata, and share bounded per-file range indexes through a bounded directory outside the ReadAt global hot path. ### Release note Preserve correct FileScannerV2 stop handling and Parquet unsupported-type errors while retaining bounded cross-reader page-cache range reuse. ### Check List (For Author) - Test: Regression test / Unit Test - 56 targeted BE ASAN unit tests - test_file_scanner_v2_review_fixes regression suite - Behavior changed: Yes (stop EOF is not counted as an empty file; unsupported requested leaves fail before metadata pruning) - Does this need documentation: No --- be/src/exec/scan/file_scanner_v2.cpp | 16 ++-- be/src/exec/scan/file_scanner_v2.h | 4 +- .../parquet/parquet_file_context.cpp | 87 +++++++++++++++--- .../format_v2/parquet/parquet_file_context.h | 32 +++++-- be/src/format_v2/parquet/parquet_reader.cpp | 73 +++++++++++++++ be/test/exec/scan/file_scanner_v2_test.cpp | 10 +- .../parquet/parquet_page_cache_range_test.cpp | 42 ++++++--- .../file_scanner_v2_unsupported_time.parquet | Bin 689 -> 673 bytes .../test_file_scanner_v2_review_fixes.groovy | 16 ++++ 9 files changed, 235 insertions(+), 45 deletions(-) diff --git a/be/src/exec/scan/file_scanner_v2.cpp b/be/src/exec/scan/file_scanner_v2.cpp index 01575e598fe739..09cc18a2b6aa5c 100644 --- a/be/src/exec/scan/file_scanner_v2.cpp +++ b/be/src/exec/scan/file_scanner_v2.cpp @@ -273,8 +273,8 @@ bool FileScannerV2::TEST_should_skip_not_found(const Status& status, bool ignore return _should_skip_not_found(status, ignore_not_found); } -bool FileScannerV2::TEST_should_skip_empty(const Status& status) { - return _should_skip_empty(status); +bool FileScannerV2::TEST_should_skip_empty(const Status& status, bool stopped) { + return _should_skip_empty(status, stopped); } #endif @@ -400,7 +400,7 @@ Status FileScannerV2::_get_block_impl(RuntimeState* state, Block* block, bool* e *eof = false; continue; } - if (_should_skip_empty(status)) { + if (_should_skip_empty(status, _should_stop || _io_ctx->should_stop)) { // END_OF_FILE here means the reader discovered a valid split with no data while // opening or probing it, not that the Scanner has exhausted all splits. Examples // are a zero-byte CSV with an explicit schema and a Doris Native file containing @@ -458,7 +458,7 @@ Status FileScannerV2::_prepare_next_split(bool* eos) { _state->update_num_finished_scan_range(1); continue; } - if (_should_skip_empty(status)) { + if (_should_skip_empty(status, _should_stop || _io_ctx->should_stop)) { // Schema discovery can reach EOF before a split becomes prepared. A header-only Native // file follows this path, while a reader that discovers emptiness on its first // get_block() follows the symmetric branch in _get_block_impl(). Both paths must @@ -570,8 +570,12 @@ bool FileScannerV2::_should_skip_not_found(const Status& status, bool ignore_not return ignore_not_found && status.is(); } -bool FileScannerV2::_should_skip_empty(const Status& status) { - return status.is(); +bool FileScannerV2::_should_skip_empty(const Status& status, bool stopped) { + // Several readers use END_OF_FILE both for a valid zero-row split and for an interrupted IO. + // For example, DeletionVectorReader returns END_OF_FILE("stop read.") after try_stop() marks + // the shared IOContext. That status must unwind the stopped scanner; counting it as an empty + // file would incorrectly finish the scan range and increment EmptyFileNum. + return !stopped && status.is(); } bool FileScannerV2::_should_enable_file_meta_cache() const { diff --git a/be/src/exec/scan/file_scanner_v2.h b/be/src/exec/scan/file_scanner_v2.h index b7e5743bfa572d..452b1652bee461 100644 --- a/be/src/exec/scan/file_scanner_v2.h +++ b/be/src/exec/scan/file_scanner_v2.h @@ -87,7 +87,7 @@ class FileScannerV2 final : public Scanner { static void TEST_report_file_cache_profile( RuntimeProfile* profile, const io::FileCacheStatistics& file_cache_statistics); static bool TEST_should_skip_not_found(const Status& status, bool ignore_not_found); - static bool TEST_should_skip_empty(const Status& status); + static bool TEST_should_skip_empty(const Status& status, bool stopped); #endif FileScannerV2(RuntimeState* state, FileScanLocalState* parent, int64_t limit, @@ -122,7 +122,7 @@ class FileScannerV2 final : public Scanner { Status _prepare_table_reader_split(const TFileRangeDesc& range, std::map partition_values); static bool _should_skip_not_found(const Status& status, bool ignore_not_found); - static bool _should_skip_empty(const Status& status); + static bool _should_skip_empty(const Status& status, bool stopped); bool _should_enable_file_meta_cache() const; std::optional _create_global_rowid_context( const TFileRangeDesc& range) const; diff --git a/be/src/format_v2/parquet/parquet_file_context.cpp b/be/src/format_v2/parquet/parquet_file_context.cpp index 4eb2e553d34c92..aa8f2622ade43b 100644 --- a/be/src/format_v2/parquet/parquet_file_context.cpp +++ b/be/src/format_v2/parquet/parquet_file_context.cpp @@ -52,22 +52,65 @@ bool page_cache_range_less(const ParquetPageCacheRange& lhs, const ParquetPageCa } // namespace +ParquetPageCacheRangeIndex::ParquetPageCacheRangeIndex(size_t max_ranges) + : _max_ranges(max_ranges) { + DORIS_CHECK(_max_ranges > 0); +} + void ParquetPageCacheRangeIndex::insert(ParquetPageCacheRange range) { - const auto it = std::lower_bound(_ranges.begin(), _ranges.end(), range, page_cache_range_less); - if (it == _ranges.end() || it->offset != range.offset || it->size != range.size) { - _ranges.insert(it, range); + std::lock_guard lock(_mutex); + auto it = std::lower_bound(_ranges.begin(), _ranges.end(), range, page_cache_range_less); + if (it != _ranges.end() && it->offset == range.offset && it->size == range.size) { + return; + } + if (_ranges.size() >= _max_ranges) { + _ranges.erase(_ranges.begin()); + it = std::lower_bound(_ranges.begin(), _ranges.end(), range, page_cache_range_less); } + _ranges.insert(it, range); } void ParquetPageCacheRangeIndex::erase(ParquetPageCacheRange range) { + std::lock_guard lock(_mutex); const auto it = std::lower_bound(_ranges.begin(), _ranges.end(), range, page_cache_range_less); if (it != _ranges.end() && it->offset == range.offset && it->size == range.size) { _ranges.erase(it); } } -void ParquetPageCacheRangeIndex::clear() { - std::vector().swap(_ranges); +std::vector ParquetPageCacheRangeIndex::ranges() const { + std::lock_guard lock(_mutex); + return _ranges; +} + +size_t ParquetPageCacheRangeIndex::size() const { + std::lock_guard lock(_mutex); + return _ranges.size(); +} + +ParquetPageCacheRangeDirectory::ParquetPageCacheRangeDirectory(size_t max_files) + : _max_files(max_files) { + DORIS_CHECK(_max_files > 0); +} + +std::shared_ptr ParquetPageCacheRangeDirectory::get_or_create( + const std::string& file_key) { + DORIS_CHECK(!file_key.empty()); + std::lock_guard lock(_mutex); + if (const auto it = _indexes.find(file_key); it != _indexes.end()) { + return it->second; + } + if (_indexes.size() >= _max_files) { + _indexes.erase(_indexes.begin()); + } + auto index = std::make_shared(); + _indexes.emplace(file_key, index); + return index; +} + +size_t ParquetPageCacheRangeDirectory::size() const { + std::lock_guard lock(_mutex); + return _indexes.size(); } std::vector plan_page_cache_range_read( @@ -158,6 +201,16 @@ bool should_use_merge_range_reader(const std::vector& ran namespace { +detail::ParquetPageCacheRangeDirectory& cached_page_range_directory() { + // Directory lookup is paid once when a reader opens. ReadAt() then synchronizes only on the + // shared index for this file, so unrelated Parquet files no longer serialize on a process-wide + // hot-path mutex. Strong references deliberately keep range discovery alive after reader A + // closes: reader B can reuse cached [100, 200) for a request [120, 150). The directory and each + // per-file index are independently capped, bounding stale metadata left by page-cache eviction. + static detail::ParquetPageCacheRangeDirectory directory; + return directory; +} + std::string build_page_cache_file_key(const io::FileReader& file_reader, const io::FileDescription& file_description) { const int64_t mtime = @@ -185,7 +238,11 @@ class DorisRandomAccessFile final : public arrow::io::RandomAccessFile { _base_file_reader(_file_reader), _io_ctx(io_ctx), _enable_page_cache(enable_page_cache), - _page_cache_file_key(std::move(page_cache_file_key)) { + _page_cache_file_key(std::move(page_cache_file_key)), + _cached_page_range_index( + _enable_page_cache && !_page_cache_file_key.empty() + ? cached_page_range_directory().get_or_create(_page_cache_file_key) + : nullptr) { DORIS_CHECK(_file_reader != nullptr); if (auto tracing_reader = std::dynamic_pointer_cast(_file_reader)) { _file_reader_stats = tracing_reader->stats(); @@ -199,10 +256,8 @@ class DorisRandomAccessFile final : public arrow::io::RandomAccessFile { 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(); + // Page payloads and their bounded per-file range index intentionally outlive this + // reader for warm scans. Only reader-specific projected ranges are released here. std::vector().swap(_page_cache_ranges); _closed = true; } @@ -341,7 +396,8 @@ class DorisRandomAccessFile final : public arrow::io::RandomAccessFile { private: bool page_cache_enabled() const { return _enable_page_cache && !config::disable_storage_page_cache && - StoragePageCache::instance() != nullptr && !_page_cache_file_key.empty(); + StoragePageCache::instance() != nullptr && !_page_cache_file_key.empty() && + _cached_page_range_index != nullptr; } bool range_in_page_cache_scope(int64_t position, int64_t nbytes) const { @@ -369,7 +425,7 @@ class DorisRandomAccessFile final : public arrow::io::RandomAccessFile { if (!StoragePageCache::instance()->lookup( page_cache_key(cached_range.offset, cached_range.size), &handle, segment_v2::DATA_PAGE)) { - _cached_page_range_index.erase(cached_range); + _cached_page_range_index->erase(cached_range); return false; } Slice cached = handle.data(); @@ -383,7 +439,7 @@ class DorisRandomAccessFile final : public arrow::io::RandomAccessFile { bool try_read_from_cached_ranges(int64_t position, int64_t nbytes, void* out) { auto plan = detail::plan_page_cache_range_read(position, nbytes, - _cached_page_range_index.ranges()); + _cached_page_range_index->ranges()); if (plan.empty()) { return false; } @@ -434,7 +490,8 @@ class DorisRandomAccessFile final : public arrow::io::RandomAccessFile { PageCacheHandle handle; StoragePageCache::instance()->insert(page_cache_key(position, nbytes), page, &handle, segment_v2::DATA_PAGE); - _cached_page_range_index.insert(ParquetPageCacheRange {.offset = position, .size = nbytes}); + _cached_page_range_index->insert( + ParquetPageCacheRange {.offset = position, .size = nbytes}); ++_page_cache_stats.write_count; ++_page_cache_stats.compressed_write_count; } @@ -492,7 +549,7 @@ class DorisRandomAccessFile final : public arrow::io::RandomAccessFile { std::string _page_cache_file_key; mutable std::mutex _page_cache_mutex; std::vector _page_cache_ranges; - detail::ParquetPageCacheRangeIndex _cached_page_range_index; + std::shared_ptr _cached_page_range_index; ParquetPageCacheStats _page_cache_stats; }; diff --git a/be/src/format_v2/parquet/parquet_file_context.h b/be/src/format_v2/parquet/parquet_file_context.h index 80a45aa0d273d0..67e9102139dbf2 100644 --- a/be/src/format_v2/parquet/parquet_file_context.h +++ b/be/src/format_v2/parquet/parquet_file_context.h @@ -21,6 +21,9 @@ #include #include #include +#include +#include +#include #include #include "common/status.h" @@ -68,20 +71,37 @@ namespace detail { class ParquetPageCacheRangeIndex { public: + static constexpr size_t DEFAULT_MAX_RANGES = 65536; + + explicit ParquetPageCacheRangeIndex(size_t max_ranges = DEFAULT_MAX_RANGES); + void insert(ParquetPageCacheRange range); void erase(ParquetPageCacheRange range); - void clear(); - const std::vector& ranges() const { return _ranges; } + std::vector ranges() const; + size_t size() const; private: - // This index belongs to one DorisRandomAccessFile. Thus two files reading [0, 4KB) never - // contend on or observe the same range metadata, and closing the reader releases all entries. - // StoragePageCache remains process-wide; only its lightweight exact-range directory is scoped - // to the reader that inserted and can validate those entries. + const size_t _max_ranges; + mutable std::mutex _mutex; std::vector _ranges; }; +class ParquetPageCacheRangeDirectory { +public: + static constexpr size_t DEFAULT_MAX_FILES = 4096; + + explicit ParquetPageCacheRangeDirectory(size_t max_files = DEFAULT_MAX_FILES); + + std::shared_ptr get_or_create(const std::string& file_key); + size_t size() const; + +private: + const size_t _max_files; + mutable std::mutex _mutex; + std::unordered_map> _indexes; +}; + // Build the copy plan for a ReadAt(position, nbytes) request from the range metadata of // previously cached entries. // StoragePageCache cannot do range lookup by itself; it can only lookup an exact key. The diff --git a/be/src/format_v2/parquet/parquet_reader.cpp b/be/src/format_v2/parquet/parquet_reader.cpp index 753b3628bfa19b..37607b3df2b864 100644 --- a/be/src/format_v2/parquet/parquet_reader.cpp +++ b/be/src/format_v2/parquet/parquet_reader.cpp @@ -118,6 +118,60 @@ void collect_request_leaf_column_ids( } } +Status validate_all_projected_leaves_supported(const ParquetColumnSchema& column_schema) { + if (column_schema.kind == ParquetColumnSchemaKind::PRIMITIVE) { + if (!column_schema.type_descriptor.unsupported_reason.empty()) { + return Status::NotSupported("Unsupported parquet column '{}': {}", column_schema.name, + column_schema.type_descriptor.unsupported_reason); + } + return Status::OK(); + } + for (const auto& child : column_schema.children) { + DORIS_CHECK(child != nullptr); + RETURN_IF_ERROR(validate_all_projected_leaves_supported(*child)); + } + return Status::OK(); +} + +Status validate_projected_leaves_supported(const ParquetColumnSchema& column_schema, + const format::LocalColumnIndex& projection) { + if (column_schema.kind == ParquetColumnSchemaKind::PRIMITIVE || + projection.project_all_children || projection.children.empty()) { + return validate_all_projected_leaves_supported(column_schema); + } + for (const auto& child_projection : projection.children) { + const auto child_it = + std::ranges::find_if(column_schema.children, [&](const auto& child_schema) { + return child_schema->local_id == child_projection.local_id(); + }); + DORIS_CHECK(child_it != column_schema.children.end()); + RETURN_IF_ERROR(validate_projected_leaves_supported(**child_it, child_projection)); + } + return Status::OK(); +} + +Status validate_requested_columns_supported( + const std::vector>& file_schema, + const format::FileScanRequest& request) { + auto validate_scan_column = [&](const format::LocalColumnIndex& projection) -> Status { + const auto local_id = projection.local_id(); + if (local_id == format::ROW_POSITION_COLUMN_ID || + local_id == format::GLOBAL_ROWID_COLUMN_ID) { + return Status::OK(); + } + DORIS_CHECK(local_id >= 0 && local_id < static_cast(file_schema.size())); + DORIS_CHECK(file_schema[local_id] != nullptr); + return validate_projected_leaves_supported(*file_schema[local_id], projection); + }; + for (const auto& column : request.predicate_columns) { + RETURN_IF_ERROR(validate_scan_column(column)); + } + for (const auto& column : request.non_predicate_columns) { + RETURN_IF_ERROR(validate_scan_column(column)); + } + return Status::OK(); +} + std::vector build_page_cache_ranges( const ::parquet::FileMetaData& metadata, const std::vector>& file_schema, @@ -454,6 +508,12 @@ Status ParquetReader::open(std::shared_ptr request) { DORIS_CHECK(local_id >= 0 && local_id < num_fields); } + // Reject requested unsupported logical leaves before row-group statistics, dictionaries, + // bloom filters or page indexes inspect their physical fallback type. For example, a predicate + // on TIME_MILLIS must fail here even when its INT32 statistics would prune every row group; + // otherwise the same unsupported SELECT could fail or silently succeed depending on data. + RETURN_IF_ERROR(validate_requested_columns_supported(_state->file_schema, *request_snapshot)); + RowGroupScanPlan row_group_plan; ParquetScanRange scan_range; scan_range.start_offset = _file_description->range_start_offset; @@ -598,6 +658,19 @@ Status ParquetReader::get_aggregate_result(const format::FileAggregateRequest& r request.agg_type); } + for (const auto& aggregate_column : request.columns) { + const auto local_id = aggregate_column.projection.local_id(); + if (local_id < 0 || local_id >= static_cast(_state->file_schema.size())) { + return Status::InvalidArgument("Invalid parquet aggregate column id {}", local_id); + } + DORIS_CHECK(_state->file_schema[local_id] != nullptr); + // Aggregate pushdown can return directly from footer statistics without constructing a + // column reader. Validate first so MIN/MAX(TIME_MILLIS), or an all-pruned COUNT request, + // cannot expose the physical INT32 fallback as a supported logical value. + RETURN_IF_ERROR(validate_projected_leaves_supported(*_state->file_schema[local_id], + aggregate_column.projection)); + } + // Aggregate row count in all selected row groups. For MIN/MAX aggregate, this is used to determine whether there is no row group selected. for (const auto& row_group_plan : _state->scan_plan.row_groups) { auto row_group_metadata = diff --git a/be/test/exec/scan/file_scanner_v2_test.cpp b/be/test/exec/scan/file_scanner_v2_test.cpp index a18ae9c6d5bd85..2723e9300de32f 100644 --- a/be/test/exec/scan/file_scanner_v2_test.cpp +++ b/be/test/exec/scan/file_scanner_v2_test.cpp @@ -708,9 +708,13 @@ TEST(FileScannerV2Test, NotFoundIsSkippedOnlyWhenConfigured) { } TEST(FileScannerV2Test, EndOfFileIsSkippedAsEmptySplit) { - EXPECT_TRUE(FileScannerV2::TEST_should_skip_empty(Status::EndOfFile("empty file"))); - EXPECT_FALSE(FileScannerV2::TEST_should_skip_empty(Status::InternalError("read failed"))); - EXPECT_FALSE(FileScannerV2::TEST_should_skip_empty(Status::OK())); + EXPECT_TRUE(FileScannerV2::TEST_should_skip_empty(Status::EndOfFile("empty file"), false)); + // Deletion-vector and Parquet readers also use EOF to unwind an interrupted read. Once either + // scanner stop flag is visible, the same status is no longer evidence of an empty file. + EXPECT_FALSE(FileScannerV2::TEST_should_skip_empty(Status::EndOfFile("stop read."), true)); + EXPECT_FALSE( + FileScannerV2::TEST_should_skip_empty(Status::InternalError("read failed"), false)); + EXPECT_FALSE(FileScannerV2::TEST_should_skip_empty(Status::OK(), false)); } // Scenario: partition slots are identified from the explicit FE category when present, otherwise diff --git a/be/test/format_v2/parquet/parquet_page_cache_range_test.cpp b/be/test/format_v2/parquet/parquet_page_cache_range_test.cpp index f06c3fd4660c09..940f94a373a013 100644 --- a/be/test/format_v2/parquet/parquet_page_cache_range_test.cpp +++ b/be/test/format_v2/parquet/parquet_page_cache_range_test.cpp @@ -115,27 +115,43 @@ TEST(ParquetPageCacheRangeTest, InvalidRequestMisses) { EXPECT_TRUE(detail::plan_page_cache_range_read(100, -1, cached_ranges).empty()); } -TEST(ParquetPageCacheRangeTest, PerFileRangeIndexSortsDeduplicatesAndClears) { - detail::ParquetPageCacheRangeIndex index; +TEST(ParquetPageCacheRangeTest, PerFileRangeIndexDeduplicatesAndEvictsAtCapacity) { + detail::ParquetPageCacheRangeIndex index(3); index.insert({200, 20}); index.insert({100, 30}); index.insert({100, 10}); index.insert({100, 30}); - ASSERT_EQ(index.ranges().size(), 3); - EXPECT_EQ(index.ranges()[0].offset, 100); - EXPECT_EQ(index.ranges()[0].size, 10); - EXPECT_EQ(index.ranges()[1].offset, 100); - EXPECT_EQ(index.ranges()[1].size, 30); - EXPECT_EQ(index.ranges()[2].offset, 200); + EXPECT_EQ(index.size(), 3); + index.insert({300, 40}); + const auto ranges = index.ranges(); + ASSERT_EQ(ranges.size(), 3); + EXPECT_EQ(ranges[0].offset, 100); + EXPECT_EQ(ranges[0].size, 30); + EXPECT_EQ(ranges[1].offset, 200); + EXPECT_EQ(ranges[1].size, 20); + EXPECT_EQ(ranges[2].offset, 300); index.erase({100, 30}); - ASSERT_EQ(index.ranges().size(), 2); - EXPECT_EQ(index.ranges()[0].size, 10); - EXPECT_EQ(index.ranges()[1].offset, 200); + EXPECT_EQ(index.size(), 2); +} + +TEST(ParquetPageCacheRangeTest, DirectorySharesBoundedIndexAcrossReaderLifetimes) { + detail::ParquetPageCacheRangeDirectory directory(2); + auto first_reader_index = directory.get_or_create("file-a"); + first_reader_index->insert({100, 100}); + first_reader_index.reset(); + + // The directory owns the per-file index, so reader B still discovers reader A's wider cache + // entry and can plan a subset hit after A closes. + auto second_reader_index = directory.get_or_create("file-a"); + const auto plan = detail::plan_page_cache_range_read(120, 30, second_reader_index->ranges()); + ASSERT_EQ(plan.size(), 1); + expect_plan_entry(plan[0], {100, 100}, 20, 0, 30); - index.clear(); - EXPECT_TRUE(index.ranges().empty()); + directory.get_or_create("file-b"); + directory.get_or_create("file-c"); + EXPECT_EQ(directory.size(), 2); } TEST(ParquetPageCacheRangeTest, ValidPrefetchRangesSkipInvalidAndOverflowRanges) { diff --git a/regression-test/data/external_table_p0/tvf/file_scanner_v2_unsupported_time.parquet b/regression-test/data/external_table_p0/tvf/file_scanner_v2_unsupported_time.parquet index fe1daf97c37e4e6cfa72a93adb57f9e3ac952a20..5b973181a56148d379ff1ee20612491d1121b700 100644 GIT binary patch delta 29 lcmdnUx{!6k21db&8+|6TG6rmpW%Oa>kY-|F2yhHC1OSTr2fY9Q delta 44 ucmZ3;x{-Cl21cQY8-3VqG}$E87+5C$34}2>`!M=4a%eL#Fa$UT83F(c910x( diff --git a/regression-test/suites/external_table_p0/tvf/test_file_scanner_v2_review_fixes.groovy b/regression-test/suites/external_table_p0/tvf/test_file_scanner_v2_review_fixes.groovy index ac8359d980a023..fe39450d71d0e7 100644 --- a/regression-test/suites/external_table_p0/tvf/test_file_scanner_v2_review_fixes.groovy +++ b/regression-test/suites/external_table_p0/tvf/test_file_scanner_v2_review_fixes.groovy @@ -81,4 +81,20 @@ suite("test_file_scanner_v2_review_fixes", "p0,external") { ORDER BY id """ + // A requested unsupported leaf must fail before metadata pruning. The fixture stores positive + // TIME_MILLIS values, so `unsupported_time < 0` can be eliminated from INT32 row-group + // statistics. Without the early validation, this query incorrectly returns an empty result + // instead of preserving the unsupported logical-type contract. + test { + sql """ + SELECT unsupported_time + FROM local( + "file_path" = "${remotePath}/file_scanner_v2_unsupported_time.parquet", + "backend_id" = "${backendId}", + "format" = "parquet") + WHERE unsupported_time < 0 + """ + exception "Parquet TIME with isAdjustedToUTC=true is not supported" + } + } From b2693d173b1c335f9e0f1e3b82f5fb19197510aa Mon Sep 17 00:00:00 2001 From: Gabriel Date: Tue, 14 Jul 2026 12:03:23 +0800 Subject: [PATCH 03/17] [fix](be) Address remaining FileScannerV2 review cases ### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Primitive COUNT(column) pushdown omitted the counted projection, allowing unsupported Parquet logical types to bypass aggregate validation and return footer row counts. Explicit zero-length Native blocks also surfaced as EOF and could be accounted as valid empty files. Preserve scalar COUNT projections for file-reader validation, reuse footer counts only after validating required primitive columns, and reject explicit zero-length Native blocks as malformed input. ### Release note Reject unsupported Parquet scalar COUNT projections and malformed zero-length Native blocks consistently. ### Check List (For Author) - Test: Unit Test - 16 targeted Backend unit tests - Behavior changed: Yes (primitive COUNT projections are validated and explicit zero-length Native blocks return an error) - Does this need documentation: No --- be/src/format_v2/native/native_reader.cpp | 8 ++- be/src/format_v2/parquet/parquet_reader.cpp | 10 ++++ be/src/format_v2/table_reader.h | 13 +++-- .../format_v2/native/native_reader_test.cpp | 4 +- .../format_v2/parquet/parquet_scan_test.cpp | 53 +++++++++++++++++++ be/test/format_v2/table_reader_test.cpp | 7 +++ 6 files changed, 85 insertions(+), 10 deletions(-) diff --git a/be/src/format_v2/native/native_reader.cpp b/be/src/format_v2/native/native_reader.cpp index 0e348f3de2ccf5..867ea5c6a54c98 100644 --- a/be/src/format_v2/native/native_reader.cpp +++ b/be/src/format_v2/native/native_reader.cpp @@ -247,8 +247,12 @@ Status NativeReader::_read_next_pblock(std::string* buffer, bool* eof) const { } _current_offset += sizeof(block_len); if (block_len == 0) { - *eof = (_current_offset >= _file_size); - return Status::OK(); + // A header-only file reaches the `_current_offset >= _file_size` branch above and is a + // valid empty Native file. Once an explicit length prefix is present, however, zero is not + // an EOF marker in the Native format. For example, `header + uint64(0)` is truncated input, + // not a zero-row split, and must not escape as EOF for FileScannerV2 to count as empty. + return Status::InternalError("zero-length native block in file {} at offset {}", + _file_description->path, _current_offset - sizeof(block_len)); } buffer->assign(block_len, '\0'); diff --git a/be/src/format_v2/parquet/parquet_reader.cpp b/be/src/format_v2/parquet/parquet_reader.cpp index 37607b3df2b864..1c5d00e7d76e1c 100644 --- a/be/src/format_v2/parquet/parquet_reader.cpp +++ b/be/src/format_v2/parquet/parquet_reader.cpp @@ -687,6 +687,16 @@ Status ParquetReader::get_aggregate_result(const format::FileAggregateRequest& r } const auto& count_projection = request.columns[0].projection; const auto& root_schema = projected_root_schema(_state->file_schema, count_projection); + // A required primitive COUNT(col) still carries its projection so the unsupported-type + // validation above cannot be bypassed. Once validated, its definition level proves that + // every selected row is non-NULL, so preserve the already-computed footer row count and + // avoid reading definition levels merely to rediscover COUNT(col) == COUNT(*). Complex + // roots continue through the shape reader because their count semantics and read-row + // accounting are derived from nested levels. + if (root_schema.kind == ParquetColumnSchemaKind::PRIMITIVE && + root_schema.max_definition_level == 0) { + return Status::OK(); + } result->count = 0; for (const auto& row_group_plan : _state->scan_plan.row_groups) { std::shared_ptr<::parquet::RowGroupReader> row_group; diff --git a/be/src/format_v2/table_reader.h b/be/src/format_v2/table_reader.h index 73bade81bc4c65..eac72a1e4c8287 100644 --- a/be/src/format_v2/table_reader.h +++ b/be/src/format_v2/table_reader.h @@ -1463,16 +1463,15 @@ class TableReader { request->columns.clear(); if (agg_type == TPushAggOp::type::COUNT) { // COUNT pushdown historically meant COUNT(*) and therefore carried no columns. For - // complex COUNT(col), materializing the full MAP/LIST/STRUCT value only to test the - // top-level NULL bit can be extremely expensive. When the scan projects exactly one - // directly-mapped complex column, pass that file column to the reader so formats such - // as Parquet can count the column shape from metadata/levels without decoding payload - // values like MAP value strings. Other COUNT cases stay on the existing row-count path - // to avoid changing count(*) semantics. + // COUNT(col), Nereids retains exactly the counted slot in the file scan. Pass that + // single direct mapping for both scalar and complex columns: besides allowing nullable + // complex values to be counted from levels, it lets the file reader validate logical + // types before returning footer row counts. For example, a required TIME_MILLIS leaf + // has the same count as COUNT(*), but it must still fail as an unsupported requested + // column rather than silently returning the row-group count. if (_data_reader.column_mapper->mappings().size() == 1) { const auto& mapping = _data_reader.column_mapper->mappings()[0]; if (mapping.file_local_id.has_value() && mapping.file_type != nullptr && - is_complex_type(remove_nullable(mapping.file_type)->get_primitive_type()) && mapping.virtual_column_type == TableVirtualColumnType::INVALID && mapping.default_expr == nullptr) { FileAggregateRequest::Column column; diff --git a/be/test/format_v2/native/native_reader_test.cpp b/be/test/format_v2/native/native_reader_test.cpp index 6b0c9ab27b90a8..422b2d5c37b2b2 100644 --- a/be/test/format_v2/native/native_reader_test.cpp +++ b/be/test/format_v2/native/native_reader_test.cpp @@ -375,7 +375,9 @@ TEST(NativeV2ReaderTest, RejectsZeroLengthBlockAndInvalidPBlock) { auto zero_len_reader = create_reader(zero_len_path, &state, &profile); ASSERT_TRUE(zero_len_reader->init(&state).ok()); std::vector schema; - EXPECT_FALSE(zero_len_reader->get_schema(&schema).ok()); + const auto zero_len_status = zero_len_reader->get_schema(&schema); + EXPECT_TRUE(zero_len_status.is()) << zero_len_status; + EXPECT_NE(zero_len_status.to_string().find("zero-length native block"), std::string::npos); static_cast(io::global_local_filesystem()->delete_file(zero_len_path)); const auto invalid_pblock_path = diff --git a/be/test/format_v2/parquet/parquet_scan_test.cpp b/be/test/format_v2/parquet/parquet_scan_test.cpp index 55387d2c1ed380..6b1ae8918cad4d 100644 --- a/be/test/format_v2/parquet/parquet_scan_test.cpp +++ b/be/test/format_v2/parquet/parquet_scan_test.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -304,6 +305,32 @@ void write_table(const std::string& file_path, const std::shared_ptr out = *file_result; + + // Arrow intentionally writes time32 as local time (isAdjustedToUTC=false), so use Parquet's + // low-level writer to produce the unsupported required logical type needed by this aggregate + // regression. Required is important: Nereids may push COUNT(col) because NULL filtering cannot + // change its value, which is precisely the path where an empty aggregate request lost `col`. + const auto adjusted_time = ::parquet::schema::PrimitiveNode::Make( + "unsupported_time", ::parquet::Repetition::REQUIRED, + ::parquet::LogicalType::Time(true, ::parquet::LogicalType::TimeUnit::MILLIS), + ::parquet::Type::INT32); + const auto schema_node = ::parquet::schema::GroupNode::Make( + "schema", ::parquet::Repetition::REQUIRED, {adjusted_time}); + const auto schema = std::static_pointer_cast<::parquet::schema::GroupNode>(schema_node); + auto writer = ::parquet::ParquetFileWriter::Open(out, schema); + auto* row_group = writer->AppendRowGroup(); + auto* time_writer = static_cast<::parquet::Int32Writer*>(row_group->NextColumn()); + const int32_t values[] = {1000, 2000}; + EXPECT_EQ(time_writer->WriteBatch(2, nullptr, nullptr, values), 2); + time_writer->Close(); + row_group->Close(); + writer->Close(); +} + void write_int_pair_parquet_file(const std::string& file_path, int64_t row_group_size = 2, bool enable_statistics = true) { auto schema = arrow::schema({ @@ -737,6 +764,15 @@ TEST_F(ParquetScanTest, AggregateCountAndMinMaxUseAllSelectedRowGroups) { EXPECT_EQ(count_result.count, 6); EXPECT_TRUE(count_result.columns.empty()); + format::FileAggregateResult required_count_result; + format::FileAggregateRequest required_count_request; + required_count_request.agg_type = TPushAggOp::COUNT; + required_count_request.columns.push_back({.projection = field_projection(0)}); + ASSERT_TRUE(reader->get_aggregate_result(required_count_request, &required_count_result).ok()); + // The required scalar projection is retained for logical-type validation, but after that + // validation COUNT(id) can reuse the same selected-row-group footer count as COUNT(*). + EXPECT_EQ(required_count_result.count, 6); + format::FileAggregateResult minmax_result; format::FileAggregateRequest minmax_request; minmax_request.agg_type = TPushAggOp::MINMAX; @@ -753,6 +789,23 @@ TEST_F(ParquetScanTest, AggregateCountAndMinMaxUseAllSelectedRowGroups) { EXPECT_EQ(minmax_result.columns[1].max_value.get(), 60); } +TEST_F(ParquetScanTest, AggregateCountRejectsRequiredUnsupportedScalarProjection) { + write_required_adjusted_time_parquet_file(_file_path); + auto reader = create_reader(); + RuntimeState state {TQueryOptions(), TQueryGlobals()}; + ASSERT_TRUE(reader->init(&state).ok()); + open_all_row_groups(reader.get()); + + format::FileAggregateRequest request; + request.agg_type = TPushAggOp::COUNT; + request.columns.push_back({.projection = field_projection(0)}); + format::FileAggregateResult result; + const auto status = reader->get_aggregate_result(request, &result); + EXPECT_TRUE(status.is()) << status; + EXPECT_NE(status.to_string().find("Parquet TIME with isAdjustedToUTC=true is not supported"), + std::string::npos); +} + TEST_F(ParquetScanTest, AggregateMinMaxRejectsInexactBinaryStatistics) { write_binary_minmax_parquet_file(_file_path); auto reader = create_reader(); diff --git a/be/test/format_v2/table_reader_test.cpp b/be/test/format_v2/table_reader_test.cpp index 05ef805ae71267..86a6c751d48ee2 100644 --- a/be/test/format_v2/table_reader_test.cpp +++ b/be/test/format_v2/table_reader_test.cpp @@ -1012,6 +1012,7 @@ struct FakeFileReaderState { bool stop_during_read = false; bool not_found_during_init = false; std::shared_ptr last_request; + std::optional last_aggregate_request; std::shared_ptr condition_cache_ctx; std::shared_ptr io_ctx; }; @@ -1127,6 +1128,7 @@ class FakeFileReader final : public FileReader { if (request.agg_type != TPushAggOp::type::COUNT) { return Status::NotSupported("fake reader only supports COUNT aggregate pushdown"); } + _state->last_aggregate_request = request; if (_state->stop_during_aggregate) { DORIS_CHECK(_state->io_ctx != nullptr); _state->io_ctx->should_stop = true; @@ -1753,6 +1755,11 @@ TEST(TableReaderTest, PushDownCountRecordsReaderRowsBeforeClosingReader) { EXPECT_EQ(block.rows(), 3); EXPECT_EQ(file_reader_stats.read_rows, 3); EXPECT_EQ(fake_state->close_count, 1); + ASSERT_TRUE(fake_state->last_aggregate_request.has_value()); + ASSERT_EQ(fake_state->last_aggregate_request->columns.size(), 1); + // A primitive COUNT(col) projection must reach the file reader just like a complex one. This + // is observable even though the required `id` values make its numeric result equal COUNT(*). + EXPECT_EQ(fake_state->last_aggregate_request->columns[0].projection.local_id(), 0); } TEST(TableReaderTest, PushDownCountStopConvertsAggregateEndOfFileToEos) { From 2b85e8371658ada4a122a1b006a84d962cf13a1d Mon Sep 17 00:00:00 2001 From: Gabriel Date: Tue, 14 Jul 2026 16:58:10 +0800 Subject: [PATCH 04/17] [fix](be) Preserve COUNT star semantics in file scans Issue Number: None Related PR: #65548 Problem Summary: File aggregate pushdown inferred COUNT(column) from a single post-pruning scan mapping. COUNT(*) also retains an arbitrary placeholder slot, so a nullable placeholder made Parquet and ORC readers count non-null values instead of rows. Carry the semantic COUNT arguments from Nereids through the scan thrift contract, keep true COUNT(*) column-free, and fall back when an explicit COUNT argument cannot be mapped directly. Also default split-level conjunct overrides to nullopt so existing standalone readers compile and preserve their initial predicate snapshot. Fix COUNT(*) results for Parquet and ORC file scans when the retained placeholder column contains NULL values. - Test: Unit Test - 7 targeted Backend ASAN unit tests - Added Frontend rule coverage for COUNT(*) and COUNT(column); execution was blocked by unrelated untracked cloud storage sources missing vendor SDKs in the validation workspace - Behavior changed: Yes (file aggregate pushdown now distinguishes COUNT(*) from COUNT(column) explicitly) - Does this need documentation: No --- be/src/exec/operator/scan_operator.cpp | 8 +++ be/src/exec/operator/scan_operator.h | 6 ++ be/src/exec/scan/file_scanner_v2.cpp | 12 ++++ be/src/format_v2/table_reader.cpp | 1 + be/src/format_v2/table_reader.h | 58 +++++++++++++------ be/test/format_v2/table_reader_test.cpp | 45 ++++++++++++++ .../apache/doris/datasource/FileScanNode.java | 2 + .../translator/PhysicalPlanTranslator.java | 9 +++ .../translator/PlanTranslatorContext.java | 10 ++++ .../implementation/AggregateStrategies.java | 17 ++++-- .../PhysicalStorageLayerAggregate.java | 23 ++++++-- .../org/apache/doris/planner/PlanNode.java | 7 +++ .../PhysicalStorageLayerAggregateTest.java | 25 +++++++- gensrc/thrift/PlanNodes.thrift | 4 ++ 14 files changed, 198 insertions(+), 29 deletions(-) diff --git a/be/src/exec/operator/scan_operator.cpp b/be/src/exec/operator/scan_operator.cpp index a9e92d08e08613..b142c621a06a04 100644 --- a/be/src/exec/operator/scan_operator.cpp +++ b/be/src/exec/operator/scan_operator.cpp @@ -1046,6 +1046,11 @@ TPushAggOp::type ScanLocalState::get_push_down_agg_type() { return _parent->cast()._push_down_agg_type; } +template +const std::vector& ScanLocalState::get_push_down_count_slot_ids() const { + return _parent->cast()._push_down_count_slot_ids; +} + template int64_t ScanLocalState::limit_per_scanner() { return _parent->cast()._limit_per_scanner; @@ -1231,6 +1236,9 @@ Status ScanOperatorX::init(const TPlanNode& tnode, RuntimeState* } else { _push_down_agg_type = TPushAggOp::type::NONE; } + if (tnode.__isset.push_down_count_slot_ids) { + _push_down_count_slot_ids = tnode.push_down_count_slot_ids; + } if (tnode.__isset.topn_filter_source_node_ids) { _topn_filter_source_node_ids = tnode.topn_filter_source_node_ids; diff --git a/be/src/exec/operator/scan_operator.h b/be/src/exec/operator/scan_operator.h index 8b12ccf0bc1195..51bc4048606eea 100644 --- a/be/src/exec/operator/scan_operator.h +++ b/be/src/exec/operator/scan_operator.h @@ -74,6 +74,7 @@ class ScanLocalStateBase : public PipelineXLocalState<> { virtual void set_scan_ranges(RuntimeState* state, const std::vector& scan_ranges) = 0; virtual TPushAggOp::type get_push_down_agg_type() = 0; + virtual const std::vector& get_push_down_count_slot_ids() const = 0; // If scan operator is serial operator(like topn), its real parallelism is 1. // Otherwise, its real parallelism is query_parallel_instance_num. @@ -252,6 +253,7 @@ class ScanLocalState : public ScanLocalStateBase { const std::vector& scan_ranges) override {} TPushAggOp::type get_push_down_agg_type() override; + const std::vector& get_push_down_count_slot_ids() const override; std::vector execution_dependencies() override { if (_filter_dependencies.empty()) { @@ -452,6 +454,10 @@ class ScanOperatorX : public OperatorX { TPushAggOp::type _push_down_agg_type; + // Semantic arguments of a pushed-down COUNT. An empty list means COUNT(*)/COUNT(1), even + // when column pruning keeps an arbitrary physical slot as a scan placeholder. + std::vector _push_down_count_slot_ids; + // Record the value of the aggregate function 'count' from doris's be int64_t _push_down_count = -1; const int _parallel_tasks = 0; diff --git a/be/src/exec/scan/file_scanner_v2.cpp b/be/src/exec/scan/file_scanner_v2.cpp index 09cc18a2b6aa5c..73bc11718eb0c6 100644 --- a/be/src/exec/scan/file_scanner_v2.cpp +++ b/be/src/exec/scan/file_scanner_v2.cpp @@ -488,6 +488,17 @@ Status FileScannerV2::_init_table_reader(const TFileRangeDesc& range) { VExprContextSPtrs table_conjuncts; RETURN_IF_ERROR(_build_table_conjuncts(&table_conjuncts)); + std::vector push_down_count_columns; + push_down_count_columns.reserve(_local_state->get_push_down_count_slot_ids().size()); + for (const auto slot_id : _local_state->get_push_down_count_slot_ids()) { + const auto global_index_it = _slot_id_to_global_index.find(slot_id); + if (global_index_it == _slot_id_to_global_index.end()) { + return Status::InternalError( + "Pushed-down COUNT argument is not a projected file scan slot, slot_id={}", + slot_id); + } + push_down_count_columns.push_back(global_index_it->second); + } RETURN_IF_ERROR(_table_reader->init({ .projected_columns = _projected_columns, .conjuncts = std::move(table_conjuncts), @@ -498,6 +509,7 @@ Status FileScannerV2::_init_table_reader(const TFileRangeDesc& range) { .scanner_profile = _local_state->scanner_profile(), .file_slot_descs = &_file_slot_descs, .push_down_agg_type = _local_state->get_push_down_agg_type(), + .push_down_count_columns = std::move(push_down_count_columns), .condition_cache_digest = _local_state->get_condition_cache_digest(), })); return Status::OK(); diff --git a/be/src/format_v2/table_reader.cpp b/be/src/format_v2/table_reader.cpp index 5f37538c1f2639..3cbb58659316a5 100644 --- a/be/src/format_v2/table_reader.cpp +++ b/be/src/format_v2/table_reader.cpp @@ -527,6 +527,7 @@ Status TableReader::init(TableReadOptions&& options) { _scanner_profile = options.scanner_profile; _file_slot_descs = options.file_slot_descs; _push_down_agg_type = options.push_down_agg_type; + _push_down_count_columns = options.push_down_count_columns; _initial_condition_cache_digest = options.condition_cache_digest; _condition_cache_digest = _initial_condition_cache_digest; _projected_columns = std::move(options.projected_columns); diff --git a/be/src/format_v2/table_reader.h b/be/src/format_v2/table_reader.h index eac72a1e4c8287..2e7f026961526b 100644 --- a/be/src/format_v2/table_reader.h +++ b/be/src/format_v2/table_reader.h @@ -135,6 +135,9 @@ struct TableReadOptions { const std::vector* file_slot_descs = nullptr; // Push-down aggregate type. const TPushAggOp::type push_down_agg_type = TPushAggOp::type::NONE; + // Table/global indices of explicit COUNT arguments. Empty means COUNT(*)/COUNT(1), not "infer + // the argument from projected_columns": COUNT(*) still projects one arbitrary placeholder. + const std::vector push_down_count_columns = {}; // Initial digest of predicates available during scanner open. Scanner-driven splits override it // with SplitReadOptions::condition_cache_digest after collecting late-arrival runtime filters. // A zero digest disables condition cache. @@ -147,7 +150,7 @@ struct SplitReadOptions { // Latest scanner conjuncts rewritten to table/global column indices. Runtime filters may // arrive after TableReader::init(), so scanner-driven splits replace the initial snapshot. // nullopt preserves the initial snapshot for standalone TableReader callers. - std::optional conjuncts; + std::optional conjuncts = std::nullopt; // Independent clones used for partition pruning because evaluation prepares and opens them // against a synthetic partition block before the file reader opens its row-level conjuncts. VExprContextSPtrs partition_prune_conjuncts; @@ -979,7 +982,19 @@ class TableReader { return false; } if (agg_type == TPushAggOp::type::COUNT) { - return true; + // COUNT(*) needs no column metadata. COUNT(col) currently supports one direct file + // column; multiple COUNT arguments fall back to the normal scan so every upper + // aggregate receives the original rows. + if (_push_down_count_columns.empty()) { + return true; + } + if (_push_down_count_columns.size() != 1) { + return false; + } + const auto& mapping = _push_down_count_mapping(); + return mapping.file_local_id.has_value() && mapping.file_type != nullptr && + mapping.virtual_column_type == TableVirtualColumnType::INVALID && + mapping.default_expr == nullptr; } // For MIN/MAX, only support direct file-to-table column mappings. The two emitted rows // must be enough for the upper MIN/MAX aggregate without evaluating default expressions or @@ -1462,23 +1477,16 @@ class TableReader { request->agg_type = agg_type; request->columns.clear(); if (agg_type == TPushAggOp::type::COUNT) { - // COUNT pushdown historically meant COUNT(*) and therefore carried no columns. For - // COUNT(col), Nereids retains exactly the counted slot in the file scan. Pass that - // single direct mapping for both scalar and complex columns: besides allowing nullable - // complex values to be counted from levels, it lets the file reader validate logical - // types before returning footer row counts. For example, a required TIME_MILLIS leaf - // has the same count as COUNT(*), but it must still fail as an unsupported requested - // column rather than silently returning the row-group count. - if (_data_reader.column_mapper->mappings().size() == 1) { - const auto& mapping = _data_reader.column_mapper->mappings()[0]; - if (mapping.file_local_id.has_value() && mapping.file_type != nullptr && - mapping.virtual_column_type == TableVirtualColumnType::INVALID && - mapping.default_expr == nullptr) { - FileAggregateRequest::Column column; - column.projection = - LocalColumnIndex::top_level(LocalColumnId(*mapping.file_local_id)); - request->columns.push_back(std::move(column)); - } + // An empty explicit list is the semantic signal for COUNT(*). Do not inspect the + // mapping count: `SELECT COUNT(*) FROM t` may still project one nullable column because + // the planner keeps a placeholder slot, and counting that slot would return COUNT(col). + if (!_push_down_count_columns.empty()) { + const auto& mapping = _push_down_count_mapping(); + DORIS_CHECK(mapping.file_local_id.has_value()); + FileAggregateRequest::Column column; + column.projection = + LocalColumnIndex::top_level(LocalColumnId(*mapping.file_local_id)); + request->columns.push_back(std::move(column)); } return Status::OK(); } @@ -1495,6 +1503,17 @@ class TableReader { return Status::OK(); } + const ColumnMapping& _push_down_count_mapping() const { + DORIS_CHECK(_push_down_count_columns.size() == 1); + const auto mapping_it = + std::ranges::find(_data_reader.column_mapper->mappings(), + _push_down_count_columns.front(), &ColumnMapping::global_index); + // FileScannerV2 translates FE SlotIds through the same projected-column list used to build + // the mapper, so a missing mapping is an FE/BE contract violation rather than a fallback. + DORIS_CHECK(mapping_it != _data_reader.column_mapper->mappings().end()); + return *mapping_it; + } + Status _materialize_aggregate_pushdown_rows(TPushAggOp::type agg_type, const FileAggregateResult& file_result, Block* block) { @@ -1599,6 +1618,7 @@ class TableReader { const std::vector* _file_slot_descs = nullptr; FileFormat _format; TPushAggOp::type _push_down_agg_type = TPushAggOp::type::NONE; + std::vector _push_down_count_columns; size_t _batch_size = 0; uint64_t _initial_condition_cache_digest = 0; uint64_t _condition_cache_digest = 0; diff --git a/be/test/format_v2/table_reader_test.cpp b/be/test/format_v2/table_reader_test.cpp index 86a6c751d48ee2..dee28baf5286b1 100644 --- a/be/test/format_v2/table_reader_test.cpp +++ b/be/test/format_v2/table_reader_test.cpp @@ -1741,6 +1741,7 @@ TEST(TableReaderTest, PushDownCountRecordsReaderRowsBeforeClosingReader) { .runtime_state = &state, .scanner_profile = nullptr, .push_down_agg_type = TPushAggOp::type::COUNT, + .push_down_count_columns = {GlobalIndex(0)}, }) .ok()); @@ -1762,6 +1763,50 @@ TEST(TableReaderTest, PushDownCountRecordsReaderRowsBeforeClosingReader) { EXPECT_EQ(fake_state->last_aggregate_request->columns[0].projection.local_id(), 0); } +TEST(TableReaderTest, PushDownCountStarIgnoresProjectedPlaceholderColumn) { + const auto nullable_int_type = make_nullable(std::make_shared()); + std::vector file_schema; + file_schema.push_back(make_file_column(0, "nullable_id", nullable_int_type)); + + std::vector projected_columns; + projected_columns.push_back(make_table_column(0, "nullable_id", nullable_int_type)); + set_name_identifiers(&projected_columns); + + RuntimeState state {TQueryOptions(), TQueryGlobals()}; + auto fake_state = std::make_shared(); + fake_state->aggregate_count = 3; + FakeTableReader reader(file_schema, fake_state); + ASSERT_TRUE( + reader.init({ + .projected_columns = projected_columns, + .conjuncts = {}, + .format = FileFormat::PARQUET, + .scan_params = nullptr, + .io_ctx = nullptr, + .runtime_state = &state, + .scanner_profile = nullptr, + .push_down_agg_type = TPushAggOp::type::COUNT, + // COUNT(*) deliberately has no explicit count columns. The + // nullable_id projection is only the planner's scan placeholder. + .push_down_count_columns = {}, + }) + .ok()); + + SplitReadOptions split_options; + split_options.current_range.__set_path("fake-table-reader-input"); + ASSERT_TRUE(reader.prepare_split(split_options).ok()); + + Block block = build_table_block(projected_columns); + bool eos = false; + ASSERT_TRUE(reader.get_block(&block, &eos).ok()); + EXPECT_FALSE(eos); + EXPECT_EQ(block.rows(), 3); + ASSERT_TRUE(fake_state->last_aggregate_request.has_value()); + // Passing nullable_id here would implement COUNT(nullable_id) and reproduce the external ORC + // and Parquet failures where footer row counts were reduced by null values. + EXPECT_TRUE(fake_state->last_aggregate_request->columns.empty()); +} + TEST(TableReaderTest, PushDownCountStopConvertsAggregateEndOfFileToEos) { std::vector file_schema; file_schema.push_back(make_file_column(0, "id", std::make_shared())); diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/FileScanNode.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/FileScanNode.java index 466dad11d5dc49..bfcbb24489176e 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/FileScanNode.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/FileScanNode.java @@ -88,6 +88,8 @@ public FileScanNode(PlanNodeId id, TupleDescriptor desc, String planNodeName, @Override protected void toThrift(TPlanNode planNode) { planNode.setPushDownAggTypeOpt(pushDownAggNoGroupingOp); + planNode.setPushDownCountSlotIds( + pushDownCountSlotIds.stream().map(id -> id.asInt()).collect(Collectors.toList())); planNode.setNodeType(TPlanNodeType.FILE_SCAN_NODE); TFileScanNode fileScanNode = new TFileScanNode(); diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java index 62e35f744732ad..f42112d1e11d28 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java @@ -850,6 +850,11 @@ private PlanFragment getPlanFragmentForPhysicalFileScan(PhysicalFileScan fileSca scanNode.setNereidsId(fileScan.getId()); context.getNereidsIdToPlanNodeIdMap().put(fileScan.getId(), scanNode.getId()); scanNode.setPushDownAggNoGrouping(context.getRelationPushAggOp(fileScan.getRelationId())); + scanNode.setPushDownCountSlotIds(context.getRelationPushCountArgumentExprIds(fileScan.getRelationId()) + .stream() + .map(exprId -> Objects.requireNonNull(context.findSlotRef(exprId), + "missing slot for pushed-down COUNT argument " + exprId).getSlotId()) + .collect(Collectors.toList())); scanNode.setHasPartitionPredicate(fileScan.hasPartitionPredicate()); if (fileScan.getStats() != null) { @@ -1401,6 +1406,10 @@ public PlanFragment visitPhysicalStorageLayerAggregate( context.setRelationPushAggOp( storageLayerAggregate.getRelation().getRelationId(), pushAggOp); + context.setRelationPushCountArgumentExprIds( + storageLayerAggregate.getRelation().getRelationId(), + pushAggOp == TPushAggOp.COUNT + ? storageLayerAggregate.getCountArgumentExprIds() : ImmutableList.of()); PlanFragment planFragment = storageLayerAggregate.getRelation().accept(this, context); diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PlanTranslatorContext.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PlanTranslatorContext.java index f680936b5e5ef4..936f4cbe1a9235 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PlanTranslatorContext.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PlanTranslatorContext.java @@ -52,6 +52,7 @@ import com.google.common.collect.Maps; import com.google.common.collect.Sets; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Optional; @@ -106,6 +107,7 @@ public class PlanTranslatorContext { private final Map cteScanNodeMap = Maps.newHashMap(); private final Map tablePushAggOp = Maps.newHashMap(); + private final Map> tablePushCountArgumentExprIds = Maps.newHashMap(); private final Map> statsUnknownColumnsMap = Maps.newHashMap(); @@ -430,6 +432,14 @@ public TPushAggOp getRelationPushAggOp(RelationId relationId) { return tablePushAggOp.getOrDefault(relationId, TPushAggOp.NONE); } + public void setRelationPushCountArgumentExprIds(RelationId relationId, List exprIds) { + tablePushCountArgumentExprIds.put(relationId, Lists.newArrayList(exprIds)); + } + + public List getRelationPushCountArgumentExprIds(RelationId relationId) { + return tablePushCountArgumentExprIds.getOrDefault(relationId, Collections.emptyList()); + } + public boolean isTopMaterializeNode() { return isTopMaterializeNode; } diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java index 975cfaf973c408..8a3e32d60d63d8 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java @@ -34,6 +34,7 @@ import org.apache.doris.nereids.trees.expressions.Alias; import org.apache.doris.nereids.trees.expressions.Cast; import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.ExprId; import org.apache.doris.nereids.trees.expressions.IsNull; import org.apache.doris.nereids.trees.expressions.Or; import org.apache.doris.nereids.trees.expressions.Slot; @@ -690,6 +691,12 @@ private LogicalAggregate storageLayerAggregate( List usedSlotInTable = (List) Project.findProject(aggUsedSlots, logicalScan.getOutput()); + // COUNT(*) has no aggregate arguments, even though later column pruning retains one + // arbitrary scan slot. Preserve the semantic arguments here so the BE never needs to infer + // COUNT(col) from the post-pruning scan shape. + List countArgumentExprIds = mergeOp == PushDownAggOp.COUNT + ? usedSlotInTable.stream().map(SlotReference::getExprId).collect(Collectors.toList()) + : ImmutableList.of(); for (SlotReference slot : usedSlotInTable) { Optional optionalColumn = slot.getOriginalColumn(); @@ -738,11 +745,12 @@ private LogicalAggregate storageLayerAggregate( if (project != null) { return aggregate.withChildren(ImmutableList.of( project.withChildren( - ImmutableList.of(new PhysicalStorageLayerAggregate(physicalScan, mergeOp))) + ImmutableList.of(new PhysicalStorageLayerAggregate( + physicalScan, mergeOp, countArgumentExprIds))) )); } else { return aggregate.withChildren(ImmutableList.of( - new PhysicalStorageLayerAggregate(physicalScan, mergeOp) + new PhysicalStorageLayerAggregate(physicalScan, mergeOp, countArgumentExprIds) )); } @@ -754,11 +762,12 @@ private LogicalAggregate storageLayerAggregate( if (project != null) { return aggregate.withChildren(ImmutableList.of( project.withChildren( - ImmutableList.of(new PhysicalStorageLayerAggregate(physicalScan, mergeOp))) + ImmutableList.of(new PhysicalStorageLayerAggregate( + physicalScan, mergeOp, countArgumentExprIds))) )); } else { return aggregate.withChildren(ImmutableList.of( - new PhysicalStorageLayerAggregate(physicalScan, mergeOp) + new PhysicalStorageLayerAggregate(physicalScan, mergeOp, countArgumentExprIds) )); } diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalStorageLayerAggregate.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalStorageLayerAggregate.java index 2db9c1afc27492..1d4255b6df98ef 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalStorageLayerAggregate.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalStorageLayerAggregate.java @@ -20,6 +20,7 @@ import org.apache.doris.nereids.memo.GroupExpression; import org.apache.doris.nereids.properties.LogicalProperties; import org.apache.doris.nereids.properties.PhysicalProperties; +import org.apache.doris.nereids.trees.expressions.ExprId; import org.apache.doris.nereids.trees.expressions.functions.agg.AggregateFunction; import org.apache.doris.nereids.trees.expressions.functions.agg.Count; import org.apache.doris.nereids.trees.expressions.functions.agg.Max; @@ -43,21 +44,30 @@ public class PhysicalStorageLayerAggregate extends PhysicalCatalogRelation { private final PhysicalCatalogRelation relation; private final PushDownAggOp aggOp; + private final List countArgumentExprIds; public PhysicalStorageLayerAggregate(PhysicalCatalogRelation relation, PushDownAggOp aggOp) { + this(relation, aggOp, ImmutableList.of()); + } + + public PhysicalStorageLayerAggregate(PhysicalCatalogRelation relation, PushDownAggOp aggOp, + List countArgumentExprIds) { super(relation.getRelationId(), relation.getType(), relation.getTable(), relation.getQualifier(), Optional.empty(), relation.getLogicalProperties(), ImmutableList.of()); this.relation = Objects.requireNonNull(relation, "relation cannot be null"); this.aggOp = Objects.requireNonNull(aggOp, "aggOp cannot be null"); + this.countArgumentExprIds = ImmutableList.copyOf(countArgumentExprIds); } public PhysicalStorageLayerAggregate(PhysicalCatalogRelation relation, PushDownAggOp aggOp, + List countArgumentExprIds, Optional groupExpression, LogicalProperties logicalProperties, PhysicalProperties physicalProperties, Statistics statistics) { super(relation.getRelationId(), relation.getType(), relation.getTable(), relation.getQualifier(), groupExpression, logicalProperties, physicalProperties, statistics, ImmutableList.of()); this.relation = Objects.requireNonNull(relation, "relation cannot be null"); this.aggOp = Objects.requireNonNull(aggOp, "aggOp cannot be null"); + this.countArgumentExprIds = ImmutableList.copyOf(countArgumentExprIds); } public PhysicalRelation getRelation() { @@ -68,6 +78,10 @@ public PushDownAggOp getAggOp() { return aggOp; } + public List getCountArgumentExprIds() { + return countArgumentExprIds; + } + @Override public R accept(PlanVisitor visitor, C context) { return visitor.visitPhysicalStorageLayerAggregate(this, context); @@ -83,20 +97,21 @@ public String toString() { } public PhysicalStorageLayerAggregate withPhysicalOlapScan(PhysicalOlapScan physicalOlapScan) { - return AbstractPlan.copyWithSameId(this, () -> new PhysicalStorageLayerAggregate(physicalOlapScan, aggOp)); + return AbstractPlan.copyWithSameId(this, () -> new PhysicalStorageLayerAggregate( + physicalOlapScan, aggOp, countArgumentExprIds)); } @Override public PhysicalStorageLayerAggregate withGroupExpression(Optional groupExpression) { return AbstractPlan.copyWithSameId(this, () -> new PhysicalStorageLayerAggregate(relation, aggOp, - groupExpression, getLogicalProperties(), physicalProperties, statistics)); + countArgumentExprIds, groupExpression, getLogicalProperties(), physicalProperties, statistics)); } @Override public Plan withGroupExprLogicalPropChildren(Optional groupExpression, Optional logicalProperties, List children) { return AbstractPlan.copyWithSameId(this, () -> new PhysicalStorageLayerAggregate(relation, aggOp, - groupExpression, logicalProperties.get(), physicalProperties, statistics)); + countArgumentExprIds, groupExpression, logicalProperties.get(), physicalProperties, statistics)); } @Override @@ -104,7 +119,7 @@ public PhysicalPlan withPhysicalPropertiesAndStats(PhysicalProperties physicalPr Statistics statistics) { return AbstractPlan.copyWithSameId(this, () -> new PhysicalStorageLayerAggregate( (PhysicalCatalogRelation) relation.withPhysicalPropertiesAndStats(null, statistics), - aggOp, groupExpression, + aggOp, countArgumentExprIds, groupExpression, getLogicalProperties(), physicalProperties, statistics)); } diff --git a/fe/fe-core/src/main/java/org/apache/doris/planner/PlanNode.java b/fe/fe-core/src/main/java/org/apache/doris/planner/PlanNode.java index c1f97fdeb96e20..1fc55fe8f5e1c0 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/planner/PlanNode.java +++ b/fe/fe-core/src/main/java/org/apache/doris/planner/PlanNode.java @@ -771,11 +771,18 @@ public void setCardinalityAfterFilter(long cardinalityAfterFilter) { } protected TPushAggOp pushDownAggNoGroupingOp = TPushAggOp.NONE; + // Explicit COUNT arguments. COUNT(*)/COUNT(1) intentionally keep this empty even though + // column pruning retains one placeholder scan slot. + protected List pushDownCountSlotIds = Collections.emptyList(); public void setPushDownAggNoGrouping(TPushAggOp pushDownAggNoGroupingOp) { this.pushDownAggNoGroupingOp = pushDownAggNoGroupingOp; } + public void setPushDownCountSlotIds(List pushDownCountSlotIds) { + this.pushDownCountSlotIds = Lists.newArrayList(pushDownCountSlotIds); + } + public void setChildrenDistributeExprLists(List> childrenDistributeExprLists) { this.childrenDistributeExprLists = childrenDistributeExprLists; } diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/PhysicalStorageLayerAggregateTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/PhysicalStorageLayerAggregateTest.java index bf5b74f2ea6f25..eb3a42c3868610 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/PhysicalStorageLayerAggregateTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/PhysicalStorageLayerAggregateTest.java @@ -75,7 +75,26 @@ public void testWithoutProject() { .applyImplementation(storageLayerAggregateWithoutProject()) .matches( logicalAggregate( - physicalStorageLayerAggregate().when(agg -> agg.getAggOp() == PushDownAggOp.COUNT) + physicalStorageLayerAggregate().when(agg -> agg.getAggOp() == PushDownAggOp.COUNT + && agg.getCountArgumentExprIds().equals( + ImmutableList.of(olapScan.getOutput().get(0).getExprId()))) + ) + ); + + // COUNT(*) still keeps a placeholder scan slot after column pruning, so its semantic + // argument list must remain empty instead of being inferred from the physical scan shape. + aggregate = new LogicalAggregate<>( + Collections.emptyList(), + ImmutableList.of(new Alias(new Count(), "count_star")), + true, Optional.empty(), olapScan); + context = MemoTestUtils.createCascadesContext(aggregate); + + PlanChecker.from(context) + .applyImplementation(storageLayerAggregateWithoutProject()) + .matches( + logicalAggregate( + physicalStorageLayerAggregate().when(agg -> agg.getAggOp() == PushDownAggOp.COUNT + && agg.getCountArgumentExprIds().isEmpty()) ) ); @@ -138,7 +157,9 @@ public void testWithProject() { .matches( logicalAggregate( logicalProject( - physicalStorageLayerAggregate().when(agg -> agg.getAggOp() == PushDownAggOp.COUNT) + physicalStorageLayerAggregate().when(agg -> agg.getAggOp() == PushDownAggOp.COUNT + && agg.getCountArgumentExprIds().equals( + ImmutableList.of(olapScan.getOutput().get(0).getExprId()))) ) ) ); diff --git a/gensrc/thrift/PlanNodes.thrift b/gensrc/thrift/PlanNodes.thrift index b546649933c239..dddc2d17c3215c 100644 --- a/gensrc/thrift/PlanNodes.thrift +++ b/gensrc/thrift/PlanNodes.thrift @@ -1713,6 +1713,10 @@ struct TPlanNode { 52: optional TRecCTEScanNode rec_cte_scan_node 53: optional TBucketedAggregationNode bucketed_agg_node 54: optional TLocalExchangeNode local_exchange_node + // COUNT(*) and COUNT(col) share push_down_agg_type_opt=COUNT, but file readers need to know + // whether a projected scan slot is the aggregate argument or merely the placeholder retained by + // column pruning. Empty means row-count semantics; non-empty identifies explicit COUNT columns. + 55: optional list push_down_count_slot_ids // projections is final projections, which means projecting into results and materializing them into the output block. 101: optional list projections From bfc8512095b4e0e6acfbe7c3d849f54811d23bfb Mon Sep 17 00:00:00 2001 From: Gabriel Date: Tue, 14 Jul 2026 17:45:18 +0800 Subject: [PATCH 05/17] [fix](fe) Fix aggregate strategy import order ### What problem does this PR solve? Issue Number: None Related PR: #65548 Problem Summary: The COUNT pushdown change added ExprId after Expression, violating the FE CustomImportOrder lexicographical rule. Place ExprId before Expression so Checkstyle accepts AggregateStrategies. ### Release note None ### Check List (For Author) - Test: FE Checkstyle via ./build.sh --fe -j32 on Gabriel; Checkstyle passed and compilation later stopped on unrelated cloud storage sources missing vendor SDKs - Behavior changed: No - Does this need documentation: No --- .../doris/nereids/rules/implementation/AggregateStrategies.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java index 8a3e32d60d63d8..a106429facc605 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java @@ -33,8 +33,8 @@ import org.apache.doris.nereids.rules.expression.rules.FoldConstantRuleOnFE; import org.apache.doris.nereids.trees.expressions.Alias; import org.apache.doris.nereids.trees.expressions.Cast; -import org.apache.doris.nereids.trees.expressions.Expression; import org.apache.doris.nereids.trees.expressions.ExprId; +import org.apache.doris.nereids.trees.expressions.Expression; import org.apache.doris.nereids.trees.expressions.IsNull; import org.apache.doris.nereids.trees.expressions.Or; import org.apache.doris.nereids.trees.expressions.Slot; From d6f7547af732890d7e4681c0c30e80059bcb1e7e Mon Sep 17 00:00:00 2001 From: Gabriel Date: Tue, 14 Jul 2026 19:38:28 +0800 Subject: [PATCH 06/17] [fix](be) Guard file COUNT pushdown semantics Issue Number: None Related PR: #65548 Problem Summary: Metadata COUNT pushdown could bypass schema-evolution casts and nullability validation for non-trivial column mappings. It also treated an omitted semantic-argument field from an old FE as explicit COUNT(*). Preserve thrift field presence through the scan path and only use footer COUNT(col) for a trivial direct mapping; otherwise use the normal materialization path. Fix external-file COUNT correctness and validation for schema-evolved columns during metadata pushdown. - Test: BE unit tests and regression test - ./run-be-ut.sh --run --filter="TableReaderTest.PushDownCount*" - ./run-regression-test.sh --run -d external_table_p0/tvf -s test_file_scanner_v2_review_fixes - Behavior changed: Yes; unsafe or ambiguous metadata COUNT plans now fall back to the normal scan path. - Does this need documentation: No --- be/src/exec/operator/scan_operator.cpp | 3 +- be/src/exec/operator/scan_operator.h | 17 +- be/src/exec/scan/file_scanner_v2.cpp | 22 +-- be/src/format_v2/table_reader.h | 33 ++-- be/test/format_v2/table_reader_test.cpp | 148 +++++++++++++++++- .../tvf/test_file_scanner_v2_review_fixes.out | 3 + .../test_file_scanner_v2_review_fixes.groovy | 11 ++ 7 files changed, 207 insertions(+), 30 deletions(-) diff --git a/be/src/exec/operator/scan_operator.cpp b/be/src/exec/operator/scan_operator.cpp index b142c621a06a04..3bb234d16b19b9 100644 --- a/be/src/exec/operator/scan_operator.cpp +++ b/be/src/exec/operator/scan_operator.cpp @@ -1047,7 +1047,8 @@ TPushAggOp::type ScanLocalState::get_push_down_agg_type() { } template -const std::vector& ScanLocalState::get_push_down_count_slot_ids() const { +const std::optional>& ScanLocalState::get_push_down_count_slot_ids() + const { return _parent->cast()._push_down_count_slot_ids; } diff --git a/be/src/exec/operator/scan_operator.h b/be/src/exec/operator/scan_operator.h index 51bc4048606eea..ae93e155852c00 100644 --- a/be/src/exec/operator/scan_operator.h +++ b/be/src/exec/operator/scan_operator.h @@ -18,6 +18,7 @@ #pragma once #include +#include #include #include @@ -74,7 +75,7 @@ class ScanLocalStateBase : public PipelineXLocalState<> { virtual void set_scan_ranges(RuntimeState* state, const std::vector& scan_ranges) = 0; virtual TPushAggOp::type get_push_down_agg_type() = 0; - virtual const std::vector& get_push_down_count_slot_ids() const = 0; + virtual const std::optional>& get_push_down_count_slot_ids() const = 0; // If scan operator is serial operator(like topn), its real parallelism is 1. // Otherwise, its real parallelism is query_parallel_instance_num. @@ -253,7 +254,7 @@ class ScanLocalState : public ScanLocalStateBase { const std::vector& scan_ranges) override {} TPushAggOp::type get_push_down_agg_type() override; - const std::vector& get_push_down_count_slot_ids() const override; + const std::optional>& get_push_down_count_slot_ids() const override; std::vector execution_dependencies() override { if (_filter_dependencies.empty()) { @@ -454,9 +455,15 @@ class ScanOperatorX : public OperatorX { TPushAggOp::type _push_down_agg_type; - // Semantic arguments of a pushed-down COUNT. An empty list means COUNT(*)/COUNT(1), even - // when column pruning keeps an arbitrary physical slot as a scan placeholder. - std::vector _push_down_count_slot_ids; + // Semantic arguments of a pushed-down COUNT. This is deliberately optional because absence + // and an empty list have different meanings during a BE-first rolling upgrade: + // + // - nullopt: an old FE did not send the field, so the new BE must use the normal scan; + // - empty: the new FE explicitly planned COUNT(*)/COUNT(1); + // - non-empty: the new FE explicitly planned COUNT(col). + // + // Treating nullopt as empty would silently reinterpret an old plan as COUNT(*). + std::optional> _push_down_count_slot_ids; // Record the value of the aggregate function 'count' from doris's be int64_t _push_down_count = -1; diff --git a/be/src/exec/scan/file_scanner_v2.cpp b/be/src/exec/scan/file_scanner_v2.cpp index 73bc11718eb0c6..91da5a48c510bf 100644 --- a/be/src/exec/scan/file_scanner_v2.cpp +++ b/be/src/exec/scan/file_scanner_v2.cpp @@ -488,16 +488,20 @@ Status FileScannerV2::_init_table_reader(const TFileRangeDesc& range) { VExprContextSPtrs table_conjuncts; RETURN_IF_ERROR(_build_table_conjuncts(&table_conjuncts)); - std::vector push_down_count_columns; - push_down_count_columns.reserve(_local_state->get_push_down_count_slot_ids().size()); - for (const auto slot_id : _local_state->get_push_down_count_slot_ids()) { - const auto global_index_it = _slot_id_to_global_index.find(slot_id); - if (global_index_it == _slot_id_to_global_index.end()) { - return Status::InternalError( - "Pushed-down COUNT argument is not a projected file scan slot, slot_id={}", - slot_id); + std::optional> push_down_count_columns; + const auto& push_down_count_slot_ids = _local_state->get_push_down_count_slot_ids(); + if (push_down_count_slot_ids.has_value()) { + push_down_count_columns.emplace(); + push_down_count_columns->reserve(push_down_count_slot_ids->size()); + for (const auto slot_id : *push_down_count_slot_ids) { + const auto global_index_it = _slot_id_to_global_index.find(slot_id); + if (global_index_it == _slot_id_to_global_index.end()) { + return Status::InternalError( + "Pushed-down COUNT argument is not a projected file scan slot, slot_id={}", + slot_id); + } + push_down_count_columns->push_back(global_index_it->second); } - push_down_count_columns.push_back(global_index_it->second); } RETURN_IF_ERROR(_table_reader->init({ .projected_columns = _projected_columns, diff --git a/be/src/format_v2/table_reader.h b/be/src/format_v2/table_reader.h index 2e7f026961526b..558d995a86a90a 100644 --- a/be/src/format_v2/table_reader.h +++ b/be/src/format_v2/table_reader.h @@ -135,9 +135,10 @@ struct TableReadOptions { const std::vector* file_slot_descs = nullptr; // Push-down aggregate type. const TPushAggOp::type push_down_agg_type = TPushAggOp::type::NONE; - // Table/global indices of explicit COUNT arguments. Empty means COUNT(*)/COUNT(1), not "infer - // the argument from projected_columns": COUNT(*) still projects one arbitrary placeholder. - const std::vector push_down_count_columns = {}; + // Table/global indices of explicit COUNT arguments. nullopt means an old FE did not send the + // semantic argument field, while an explicit empty vector means COUNT(*)/COUNT(1). Keeping + // those states separate prevents a rolling-upgrade plan from being reinterpreted by a new BE. + const std::optional> push_down_count_columns = std::nullopt; // Initial digest of predicates available during scanner open. Scanner-driven splits override it // with SplitReadOptions::condition_cache_digest after collecting late-arrival runtime filters. // A zero digest disables condition cache. @@ -982,17 +983,29 @@ class TableReader { return false; } if (agg_type == TPushAggOp::type::COUNT) { + // Old FEs do not serialize push_down_count_slot_ids. During the supported BE-first + // rolling upgrade, nullopt therefore means "COUNT semantics are unknown", not + // COUNT(*). Fall back to reading rows until the FE explicitly sends either an empty + // list for COUNT(*) or one slot for COUNT(col). + if (!_push_down_count_columns.has_value()) { + return false; + } // COUNT(*) needs no column metadata. COUNT(col) currently supports one direct file // column; multiple COUNT arguments fall back to the normal scan so every upper // aggregate receives the original rows. - if (_push_down_count_columns.empty()) { + if (_push_down_count_columns->empty()) { return true; } - if (_push_down_count_columns.size() != 1) { + if (_push_down_count_columns->size() != 1) { return false; } const auto& mapping = _push_down_count_mapping(); + // Metadata COUNT skips TableReader's normal materialization path. Only a trivial + // mapping is safe: for example, a nullable Parquet INT mapped to a NOT NULL table + // BIGINT normally needs both an INT->BIGINT cast and nullability validation. Counting + // footer values directly would bypass both operations and could hide invalid data. return mapping.file_local_id.has_value() && mapping.file_type != nullptr && + mapping.table_type != nullptr && mapping.is_trivial && mapping.virtual_column_type == TableVirtualColumnType::INVALID && mapping.default_expr == nullptr; } @@ -1477,10 +1490,11 @@ class TableReader { request->agg_type = agg_type; request->columns.clear(); if (agg_type == TPushAggOp::type::COUNT) { + DORIS_CHECK(_push_down_count_columns.has_value()); // An empty explicit list is the semantic signal for COUNT(*). Do not inspect the // mapping count: `SELECT COUNT(*) FROM t` may still project one nullable column because // the planner keeps a placeholder slot, and counting that slot would return COUNT(col). - if (!_push_down_count_columns.empty()) { + if (!_push_down_count_columns->empty()) { const auto& mapping = _push_down_count_mapping(); DORIS_CHECK(mapping.file_local_id.has_value()); FileAggregateRequest::Column column; @@ -1504,10 +1518,11 @@ class TableReader { } const ColumnMapping& _push_down_count_mapping() const { - DORIS_CHECK(_push_down_count_columns.size() == 1); + DORIS_CHECK(_push_down_count_columns.has_value()); + DORIS_CHECK(_push_down_count_columns->size() == 1); const auto mapping_it = std::ranges::find(_data_reader.column_mapper->mappings(), - _push_down_count_columns.front(), &ColumnMapping::global_index); + _push_down_count_columns->front(), &ColumnMapping::global_index); // FileScannerV2 translates FE SlotIds through the same projected-column list used to build // the mapper, so a missing mapping is an FE/BE contract violation rather than a fallback. DORIS_CHECK(mapping_it != _data_reader.column_mapper->mappings().end()); @@ -1618,7 +1633,7 @@ class TableReader { const std::vector* _file_slot_descs = nullptr; FileFormat _format; TPushAggOp::type _push_down_agg_type = TPushAggOp::type::NONE; - std::vector _push_down_count_columns; + std::optional> _push_down_count_columns; size_t _batch_size = 0; uint64_t _initial_condition_cache_digest = 0; uint64_t _condition_cache_digest = 0; diff --git a/be/test/format_v2/table_reader_test.cpp b/be/test/format_v2/table_reader_test.cpp index dee28baf5286b1..f902b9119371cd 100644 --- a/be/test/format_v2/table_reader_test.cpp +++ b/be/test/format_v2/table_reader_test.cpp @@ -1512,6 +1512,7 @@ TEST(TableReaderTest, RefreshedConjunctDisablesTableLevelCount) { .runtime_state = &state, .scanner_profile = nullptr, .push_down_agg_type = TPushAggOp::type::COUNT, + .push_down_count_columns = std::vector {}, }) .ok()); @@ -1554,6 +1555,7 @@ TEST(TableReaderTest, PendingRuntimeFilterDisablesTableLevelCount) { .runtime_state = &state, .scanner_profile = nullptr, .push_down_agg_type = TPushAggOp::type::COUNT, + .push_down_count_columns = std::vector {}, }) .ok()); @@ -1654,6 +1656,7 @@ TEST(TableReaderTest, SlotlessConjunctDisablesAggregatePushdown) { .runtime_state = &state, .scanner_profile = nullptr, .push_down_agg_type = TPushAggOp::type::COUNT, + .push_down_count_columns = std::vector {}, }) .ok()); @@ -1716,11 +1719,12 @@ TEST(TableReaderTest, AbortSplitClearsReaderAfterIgnorableNotFound) { } TEST(TableReaderTest, PushDownCountRecordsReaderRowsBeforeClosingReader) { + const auto nullable_int_type = make_nullable(std::make_shared()); std::vector file_schema; - file_schema.push_back(make_file_column(0, "id", std::make_shared())); + file_schema.push_back(make_file_column(0, "id", nullable_int_type)); std::vector projected_columns; - projected_columns.push_back(make_table_column(0, "id", std::make_shared())); + projected_columns.push_back(make_table_column(0, "id", nullable_int_type)); set_name_identifiers(&projected_columns); io::FileReaderStats file_reader_stats; @@ -1741,7 +1745,8 @@ TEST(TableReaderTest, PushDownCountRecordsReaderRowsBeforeClosingReader) { .runtime_state = &state, .scanner_profile = nullptr, .push_down_agg_type = TPushAggOp::type::COUNT, - .push_down_count_columns = {GlobalIndex(0)}, + .push_down_count_columns = + std::vector {GlobalIndex(0)}, }) .ok()); @@ -1758,8 +1763,7 @@ TEST(TableReaderTest, PushDownCountRecordsReaderRowsBeforeClosingReader) { EXPECT_EQ(fake_state->close_count, 1); ASSERT_TRUE(fake_state->last_aggregate_request.has_value()); ASSERT_EQ(fake_state->last_aggregate_request->columns.size(), 1); - // A primitive COUNT(col) projection must reach the file reader just like a complex one. This - // is observable even though the required `id` values make its numeric result equal COUNT(*). + // A primitive COUNT(col) projection must reach the file reader just like a complex one. EXPECT_EQ(fake_state->last_aggregate_request->columns[0].projection.local_id(), 0); } @@ -1788,7 +1792,7 @@ TEST(TableReaderTest, PushDownCountStarIgnoresProjectedPlaceholderColumn) { .push_down_agg_type = TPushAggOp::type::COUNT, // COUNT(*) deliberately has no explicit count columns. The // nullable_id projection is only the planner's scan placeholder. - .push_down_count_columns = {}, + .push_down_count_columns = std::vector {}, }) .ok()); @@ -1807,6 +1811,131 @@ TEST(TableReaderTest, PushDownCountStarIgnoresProjectedPlaceholderColumn) { EXPECT_TRUE(fake_state->last_aggregate_request->columns.empty()); } +TEST(TableReaderTest, PushDownCountFallsBackWhenSemanticArgumentsAreAbsent) { + const auto nullable_int_type = make_nullable(std::make_shared()); + std::vector file_schema; + file_schema.push_back(make_file_column(0, "id", nullable_int_type)); + + std::vector projected_columns; + projected_columns.push_back(make_table_column(0, "id", nullable_int_type)); + set_name_identifiers(&projected_columns); + + RuntimeState state {TQueryOptions(), TQueryGlobals()}; + auto fake_state = std::make_shared(); + fake_state->aggregate_count = 3; + FakeTableReader reader(file_schema, fake_state); + ASSERT_TRUE(reader.init({ + .projected_columns = projected_columns, + .conjuncts = {}, + .format = FileFormat::PARQUET, + .scan_params = nullptr, + .io_ctx = nullptr, + .runtime_state = &state, + .scanner_profile = nullptr, + .push_down_agg_type = TPushAggOp::type::COUNT, + // Simulate an old FE: it can request COUNT pushdown but cannot + // serialize push_down_count_slot_ids. + .push_down_count_columns = std::nullopt, + }) + .ok()); + + SplitReadOptions split_options; + split_options.current_range.__set_path("fake-table-reader-input"); + ASSERT_TRUE(reader.prepare_split(split_options).ok()); + + Block block = build_table_block(projected_columns); + bool eos = false; + ASSERT_TRUE(reader.get_block(&block, &eos).ok()); + EXPECT_FALSE(eos); + EXPECT_EQ(block.rows(), 2); + // The explicit aggregate_count=3 would be returned if absence were confused with COUNT(*). + EXPECT_FALSE(fake_state->last_aggregate_request.has_value()); +} + +TEST(TableReaderTest, PushDownCountFallsBackForNullableToRequiredMapping) { + const auto nullable_int_type = make_nullable(std::make_shared()); + std::vector file_schema; + file_schema.push_back(make_file_column(0, "id", nullable_int_type)); + + std::vector projected_columns; + projected_columns.push_back(make_table_column(0, "id", std::make_shared())); + // make_table_column models the usual nullable external-table descriptor. Override it here to + // reproduce an evolved table contract that declares the mapped physical column required. + projected_columns[0].type = std::make_shared(); + set_name_identifiers(&projected_columns); + + RuntimeState state {TQueryOptions(), TQueryGlobals()}; + auto fake_state = std::make_shared(); + fake_state->aggregate_count = 3; + FakeTableReader reader(file_schema, fake_state); + ASSERT_TRUE(reader.init({ + .projected_columns = projected_columns, + .conjuncts = {}, + .format = FileFormat::PARQUET, + .scan_params = nullptr, + .io_ctx = nullptr, + .runtime_state = &state, + .scanner_profile = nullptr, + .push_down_agg_type = TPushAggOp::type::COUNT, + .push_down_count_columns = + std::vector {GlobalIndex(0)}, + }) + .ok()); + + SplitReadOptions split_options; + split_options.current_range.__set_path("fake-table-reader-input"); + ASSERT_TRUE(reader.prepare_split(split_options).ok()); + + Block block = build_table_block(projected_columns); + bool eos = false; + // The normal scan rejects the nullable physical column because it cannot satisfy the required + // table contract. Footer COUNT would bypass that validation and incorrectly return 3. + EXPECT_FALSE(reader.get_block(&block, &eos).ok()); + EXPECT_FALSE(fake_state->last_aggregate_request.has_value()); +} + +TEST(TableReaderTest, PushDownCountFallsBackForCastMapping) { + const auto nullable_int_type = make_nullable(std::make_shared()); + const auto nullable_bigint_type = make_nullable(std::make_shared()); + std::vector file_schema; + file_schema.push_back(make_file_column(0, "id", nullable_int_type)); + + std::vector projected_columns; + projected_columns.push_back(make_table_column(0, "id", nullable_bigint_type)); + set_name_identifiers(&projected_columns); + + RuntimeState state {TQueryOptions(), TQueryGlobals()}; + auto fake_state = std::make_shared(); + fake_state->aggregate_count = 3; + FakeTableReader reader(file_schema, fake_state); + ASSERT_TRUE(reader.init({ + .projected_columns = projected_columns, + .conjuncts = {}, + .format = FileFormat::PARQUET, + .scan_params = nullptr, + .io_ctx = nullptr, + .runtime_state = &state, + .scanner_profile = nullptr, + .push_down_agg_type = TPushAggOp::type::COUNT, + .push_down_count_columns = + std::vector {GlobalIndex(0)}, + }) + .ok()); + + SplitReadOptions split_options; + split_options.current_range.__set_path("fake-table-reader-input"); + ASSERT_TRUE(reader.prepare_split(split_options).ok()); + + Block block = build_table_block(projected_columns); + bool eos = false; + ASSERT_TRUE(reader.get_block(&block, &eos).ok()); + EXPECT_FALSE(eos); + EXPECT_EQ(block.rows(), 2); + EXPECT_TRUE(block.get_by_position(0).type->equals(*nullable_bigint_type)); + // INT->BIGINT is a non-trivial mapping, so COUNT must not skip the cast/materialization path. + EXPECT_FALSE(fake_state->last_aggregate_request.has_value()); +} + TEST(TableReaderTest, PushDownCountStopConvertsAggregateEndOfFileToEos) { std::vector file_schema; file_schema.push_back(make_file_column(0, "id", std::make_shared())); @@ -1832,6 +1961,7 @@ TEST(TableReaderTest, PushDownCountStopConvertsAggregateEndOfFileToEos) { .runtime_state = &state, .scanner_profile = nullptr, .push_down_agg_type = TPushAggOp::type::COUNT, + .push_down_count_columns = std::vector {}, }) .ok()); @@ -1872,6 +2002,7 @@ TEST(TableReaderTest, DebugStringCoversReaderStateAndEnumNames) { .runtime_state = &state, .scanner_profile = nullptr, .push_down_agg_type = TPushAggOp::type::COUNT, + .push_down_count_columns = std::vector {}, }) .ok()); @@ -2518,6 +2649,7 @@ TEST(TableReaderTest, PushDownCountFromNewParquetReader) { .runtime_state = &state, .scanner_profile = nullptr, .push_down_agg_type = TPushAggOp::type::COUNT, + .push_down_count_columns = std::vector {}, }) .ok()); ASSERT_TRUE(reader.prepare_split(build_split_options(file_path)).ok()); @@ -2559,6 +2691,7 @@ TEST(TableReaderTest, TableLevelCountUsesAssignedRowCount) { .runtime_state = &state, .scanner_profile = nullptr, .push_down_agg_type = TPushAggOp::type::COUNT, + .push_down_count_columns = std::vector {}, }) .ok()); auto split_options = build_split_options(file_path); @@ -3406,6 +3539,7 @@ TEST(TableReaderTest, PushDownCountOnlyUsesSelectedRowGroupInFileRange) { .runtime_state = &state, .scanner_profile = nullptr, .push_down_agg_type = TPushAggOp::type::COUNT, + .push_down_count_columns = std::vector {}, }) .ok()); ASSERT_TRUE(reader.prepare_split(build_split_options_for_row_group_mid(file_path, 2)).ok()); @@ -3445,6 +3579,7 @@ TEST(TableReaderTest, PushDownCountFallsBackWithTableConjunct) { .runtime_state = &state, .scanner_profile = nullptr, .push_down_agg_type = TPushAggOp::type::COUNT, + .push_down_count_columns = std::vector {}, }) .ok()); ASSERT_TRUE(reader.prepare_split(build_split_options(file_path)).ok()); @@ -3486,6 +3621,7 @@ TEST(TableReaderTest, PushDownCountFallsBackWithFilter) { .runtime_state = &state, .scanner_profile = nullptr, .push_down_agg_type = TPushAggOp::type::COUNT, + .push_down_count_columns = std::vector {}, }) .ok()); ASSERT_TRUE(reader.prepare_split(build_split_options(file_path)).ok()); diff --git a/regression-test/data/external_table_p0/tvf/test_file_scanner_v2_review_fixes.out b/regression-test/data/external_table_p0/tvf/test_file_scanner_v2_review_fixes.out index 746fd355f15392..c8312571c56e36 100644 --- a/regression-test/data/external_table_p0/tvf/test_file_scanner_v2_review_fixes.out +++ b/regression-test/data/external_table_p0/tvf/test_file_scanner_v2_review_fixes.out @@ -9,6 +9,9 @@ 1 10 100 3 10 300 +-- !count_evolved_struct -- +4 + -- !unprojected_unsupported_time -- 1 2 diff --git a/regression-test/suites/external_table_p0/tvf/test_file_scanner_v2_review_fixes.groovy b/regression-test/suites/external_table_p0/tvf/test_file_scanner_v2_review_fixes.groovy index fe39450d71d0e7..d62435ac1c38f2 100644 --- a/regression-test/suites/external_table_p0/tvf/test_file_scanner_v2_review_fixes.groovy +++ b/regression-test/suites/external_table_p0/tvf/test_file_scanner_v2_review_fixes.groovy @@ -70,6 +70,17 @@ suite("test_file_scanner_v2_review_fixes", "p0,external") { order_qt_struct_leaf_promotion_cold evolvedStructQuery order_qt_struct_leaf_promotion_warm evolvedStructQuery + // COUNT(col) sees a non-trivial mapping for the file whose STRUCT leaf is INT while the merged + // table type is BIGINT. It must fall back to the normal scan so schema-evolution casts and + // nullability checks run instead of counting footer values directly. + order_qt_count_evolved_struct """ + SELECT COUNT(col.a) + FROM local( + "file_path" = "${remotePath}/file_scanner_v2_struct_*.parquet", + "backend_id" = "${backendId}", + "format" = "parquet") + """ + // TIME_MILLIS is intentionally unsupported by the record reader. It must not prevent schema // construction when only the supported id column is projected. order_qt_unprojected_unsupported_time """ From 19b8d8458749ca7cf1e566a275390eed9965cdb8 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Tue, 14 Jul 2026 22:46:06 +0800 Subject: [PATCH 07/17] [fix](be) Harden external file aggregate pushdown ### What problem does this PR solve? Issue Number: None Related PR: #65548 Problem Summary: External-file COUNT pushdown could discard CAST semantics, let table-level row counts answer COUNT(col), and let the legacy scanner reinterpret ambiguous or column-count plans as COUNT(*). Native files with a partial length prefix could also be mistaken for clean EOF. Restrict metadata COUNT to explicit COUNT(*) semantics at each planner and reader boundary, preserve CAST evaluation on normal file scans, and report truncated Native prefixes and bodies as corruption before allocation. ### Release note Fix external-file COUNT correctness across cast expressions, rolling-upgrade plans, and table-level metadata, and reject truncated Native blocks. ### Check List (For Author) - Test: BE unit tests - ./run-be-ut.sh --run --filter="FileScannerTest.V1CountPushdownRequiresExplicitCountStarArguments:NativeV2ReaderTest.RejectsPartialBlockLengthAsCorruption:TableReaderTest.TableLevelCountRequiresExplicitCountStarArguments" -j 4 - Behavior changed: Yes; unsafe COUNT metadata pushdown now falls back to normal scanning, and truncated Native input returns a corruption error. - Does this need documentation: No --- be/src/exec/scan/file_scanner.h | 25 +++++++++- be/src/format_v2/native/native_reader.cpp | 20 ++++++++ be/src/format_v2/table_reader.cpp | 7 ++- be/test/exec/scan/file_scanner_v2_test.cpp | 17 +++++++ .../format_v2/native/native_reader_test.cpp | 28 +++++++++++ be/test/format_v2/table_reader_test.cpp | 50 +++++++++++++++++++ .../implementation/AggregateStrategies.java | 13 +++++ 7 files changed, 157 insertions(+), 3 deletions(-) diff --git a/be/src/exec/scan/file_scanner.h b/be/src/exec/scan/file_scanner.h index 87a54243f16c96..0cf432e8544703 100644 --- a/be/src/exec/scan/file_scanner.h +++ b/be/src/exec/scan/file_scanner.h @@ -21,6 +21,7 @@ #include #include +#include #include #include #include @@ -80,6 +81,10 @@ class FileScanner : public Scanner { const VExprContextSPtrs& TEST_runtime_filter_partition_prune_ctxs() const { return _runtime_filter_partition_prune_ctxs; } + static TPushAggOp::type TEST_effective_push_down_agg_type( + TPushAggOp::type agg_type, const std::optional>& count_slot_ids) { + return _effective_push_down_agg_type(agg_type, count_slot_ids); + } #endif FileScanner(RuntimeState* state, FileScanLocalState* parent, int64_t limit, @@ -334,9 +339,25 @@ class FileScanner : public Scanner { void _update_adaptive_batch_size_before_truncate(const Block& block); void _update_adaptive_batch_size_after_truncate(const Block& block); + static TPushAggOp::type _effective_push_down_agg_type( + TPushAggOp::type agg_type, const std::optional>& count_slot_ids) { + if (agg_type != TPushAggOp::type::COUNT) { + return agg_type; + } + // V1's CountReader can only emit rows for an upper COUNT(*). It cannot evaluate the NULL + // or CAST semantics of COUNT(col), so a non-empty argument list must use the normal reader. + // nullopt is an old FE plan that predates the argument field; treating it as empty would + // silently reinterpret unknown semantics as COUNT(*). + return count_slot_ids.has_value() && count_slot_ids->empty() ? TPushAggOp::type::COUNT + : TPushAggOp::type::NONE; + } + TPushAggOp::type _get_push_down_agg_type() const { - return _local_state == nullptr ? TPushAggOp::type::NONE - : _local_state->get_push_down_agg_type(); + if (_local_state == nullptr) { + return TPushAggOp::type::NONE; + } + return _effective_push_down_agg_type(_local_state->get_push_down_agg_type(), + _local_state->get_push_down_count_slot_ids()); } // enable the file meta cache only when diff --git a/be/src/format_v2/native/native_reader.cpp b/be/src/format_v2/native/native_reader.cpp index 867ea5c6a54c98..5d0984084a6d41 100644 --- a/be/src/format_v2/native/native_reader.cpp +++ b/be/src/format_v2/native/native_reader.cpp @@ -231,6 +231,16 @@ Status NativeReader::_read_next_pblock(std::string* buffer, bool* eof) const { return Status::OK(); } + const auto remaining_prefix_bytes = _file_size - _current_offset; + if (remaining_prefix_bytes < sizeof(uint64_t)) { + // Header-only is the one valid empty-file representation and returned above. Once any + // prefix byte exists, all eight bytes are mandatory. For example, `header + 4 bytes` is a + // truncated Native file, not EOF that FileScannerV2 may skip as an empty split. + return Status::InternalError( + "truncated native block length in file {} at offset {}, expect {}, remaining {}", + _file_description->path, _current_offset, sizeof(uint64_t), remaining_prefix_bytes); + } + uint64_t block_len = 0; Slice len_slice(reinterpret_cast(&block_len), sizeof(block_len)); size_t bytes_read = 0; @@ -255,6 +265,16 @@ Status NativeReader::_read_next_pblock(std::string* buffer, bool* eof) const { _file_description->path, _current_offset - sizeof(block_len)); } + const auto remaining_block_bytes = cast_set(_file_size - _current_offset); + if (block_len > remaining_block_bytes) { + // Validate before allocating `block_len` bytes. Besides reporting a precise corruption + // error, this prevents a truncated file with a damaged length prefix from requesting an + // allocation much larger than the physical file. + return Status::InternalError( + "truncated native block body in file {} at offset {}, expect {}, remaining {}", + _file_description->path, _current_offset, block_len, remaining_block_bytes); + } + buffer->assign(block_len, '\0'); Slice data_slice(buffer->data(), block_len); bytes_read = 0; diff --git a/be/src/format_v2/table_reader.cpp b/be/src/format_v2/table_reader.cpp index 3cbb58659316a5..4897e871cec3bb 100644 --- a/be/src/format_v2/table_reader.cpp +++ b/be/src/format_v2/table_reader.cpp @@ -876,7 +876,12 @@ Status TableReader::prepare_split(const SplitReadOptions& options) { // active and no predicate can arrive later. The metadata path can return several batches for // one split; after its first synthetic batch there is no way to recover the real rows if a // runtime filter arrives before the next scheduler turn. - if (_push_down_agg_type == TPushAggOp::type::COUNT && options.all_runtime_filters_applied && + // Table-level metadata only contains the number of rows; it cannot evaluate an expression or + // the NULL state of a COUNT argument. Require the new FE's explicit empty argument list, which + // means COUNT(*)/COUNT(1). A non-empty list means COUNT(col), while nullopt comes from an old FE + // whose COUNT semantics are unknown during a BE-first rolling upgrade. + if (_push_down_agg_type == TPushAggOp::type::COUNT && _push_down_count_columns.has_value() && + _push_down_count_columns->empty() && options.all_runtime_filters_applied && _conjuncts.empty() && options.current_range.__isset.table_format_params && options.current_range.table_format_params.__isset.table_level_row_count) { DORIS_CHECK(options.current_range.table_format_params.table_level_row_count >= -1); diff --git a/be/test/exec/scan/file_scanner_v2_test.cpp b/be/test/exec/scan/file_scanner_v2_test.cpp index 2723e9300de32f..5a21cfc7081177 100644 --- a/be/test/exec/scan/file_scanner_v2_test.cpp +++ b/be/test/exec/scan/file_scanner_v2_test.cpp @@ -99,6 +99,23 @@ TFileRangeDesc legacy_paimon_jni_range_without_reader_type() { return range; } +TEST(FileScannerTest, V1CountPushdownRequiresExplicitCountStarArguments) { + EXPECT_EQ(TPushAggOp::type::COUNT, FileScanner::TEST_effective_push_down_agg_type( + TPushAggOp::type::COUNT, std::vector {})); + + // A missing field is an old FE plan with unknown COUNT semantics, not COUNT(*). + EXPECT_EQ(TPushAggOp::type::NONE, FileScanner::TEST_effective_push_down_agg_type( + TPushAggOp::type::COUNT, std::nullopt)); + // V1 cannot evaluate COUNT(col) NULL/CAST semantics before replacing the reader with + // CountReader, so an explicit argument must use the normal scan path. + EXPECT_EQ(TPushAggOp::type::NONE, FileScanner::TEST_effective_push_down_agg_type( + TPushAggOp::type::COUNT, std::vector {7})); + + // The COUNT argument field must not affect other storage-layer aggregate operations. + EXPECT_EQ(TPushAggOp::type::MINMAX, FileScanner::TEST_effective_push_down_agg_type( + TPushAggOp::type::MINMAX, std::nullopt)); +} + struct RetryableCloseState { int close_calls = 0; }; diff --git a/be/test/format_v2/native/native_reader_test.cpp b/be/test/format_v2/native/native_reader_test.cpp index 422b2d5c37b2b2..2745ecf852c38b 100644 --- a/be/test/format_v2/native/native_reader_test.cpp +++ b/be/test/format_v2/native/native_reader_test.cpp @@ -351,6 +351,34 @@ TEST(NativeV2ReaderTest, RejectsTruncatedBlockDuringSchemaProbe) { static_cast(io::global_local_filesystem()->delete_file(path)); } +TEST(NativeV2ReaderTest, RejectsPartialBlockLengthAsCorruption) { + std::filesystem::create_directories("./log"); + RuntimeState state; + RuntimeProfile profile("native_v2_reader_partial_length_test"); + + std::string header; + header.append(DORIS_NATIVE_MAGIC, sizeof(DORIS_NATIVE_MAGIC)); + uint8_t version_buffer[sizeof(uint32_t)]; + encode_fixed32_le(version_buffer, DORIS_NATIVE_FORMAT_VERSION); + header.append(reinterpret_cast(version_buffer), sizeof(version_buffer)); + + for (size_t prefix_bytes = 1; prefix_bytes < sizeof(uint64_t); ++prefix_bytes) { + const auto path = "./log/native_v2_partial_length_" + std::to_string(prefix_bytes) + "_" + + UniqueId::gen_uid().to_string() + ".native"; + auto content = header; + content.append(prefix_bytes, '\0'); + ASSERT_TRUE(write_file(path, content).ok()); + + auto reader = create_reader(path, &state, &profile); + ASSERT_TRUE(reader->init(&state).ok()); + std::vector schema; + const auto status = reader->get_schema(&schema); + EXPECT_TRUE(status.is()) << status; + EXPECT_NE(status.to_string().find("truncated native block length"), std::string::npos); + static_cast(io::global_local_filesystem()->delete_file(path)); + } +} + TEST(NativeV2ReaderTest, RejectsZeroLengthBlockAndInvalidPBlock) { std::filesystem::create_directories("./log"); RuntimeState state; diff --git a/be/test/format_v2/table_reader_test.cpp b/be/test/format_v2/table_reader_test.cpp index f902b9119371cd..3cf14c3362bc61 100644 --- a/be/test/format_v2/table_reader_test.cpp +++ b/be/test/format_v2/table_reader_test.cpp @@ -2723,6 +2723,56 @@ TEST(TableReaderTest, TableLevelCountUsesAssignedRowCount) { std::filesystem::remove_all(test_dir); } +TEST(TableReaderTest, TableLevelCountRequiresExplicitCountStarArguments) { + const auto test_dir = std::filesystem::temp_directory_path() / + "doris_table_reader_table_count_arguments_test"; + std::filesystem::remove_all(test_dir); + std::filesystem::create_directories(test_dir); + + const auto file_path = (test_dir / "split.parquet").string(); + write_int_pair_parquet_file(file_path, {1, 2, 3}, {10, 20, 30}, {"one", "two", "three"}); + + std::vector projected_columns; + projected_columns.push_back(make_table_column(0, "id", std::make_shared())); + set_name_identifiers(&projected_columns); + + const std::vector>> unsafe_count_arguments { + std::nullopt, std::vector {GlobalIndex(0)}}; + for (const auto& count_arguments : unsafe_count_arguments) { + RuntimeState state {TQueryOptions(), TQueryGlobals()}; + TableReader reader; + ASSERT_TRUE(reader.init({ + .projected_columns = projected_columns, + .conjuncts = {}, + .format = FileFormat::PARQUET, + .scan_params = nullptr, + .io_ctx = nullptr, + .runtime_state = &state, + .scanner_profile = nullptr, + .push_down_agg_type = TPushAggOp::type::COUNT, + .push_down_count_columns = count_arguments, + }) + .ok()); + auto split_options = build_split_options(file_path); + // Five metadata rows deliberately disagree with the three physical rows. nullopt models an + // old FE, while the non-empty vector models COUNT(id); neither may be treated as COUNT(*). + set_table_level_row_count(&split_options, 5); + ASSERT_TRUE(reader.prepare_split(split_options).ok()); + + size_t total_rows = 0; + bool eos = false; + while (!eos) { + Block block = build_table_block(projected_columns); + ASSERT_TRUE(reader.get_block(&block, &eos).ok()); + total_rows += block.rows(); + } + EXPECT_EQ(3, total_rows); + ASSERT_TRUE(reader.close().ok()); + } + + std::filesystem::remove_all(test_dir); +} + TEST(TableReaderTest, PushDownMinMaxFromNewParquetReader) { const auto test_dir = std::filesystem::temp_directory_path() / "doris_table_reader_minmax_test"; std::filesystem::remove_all(test_dir); diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java index a106429facc605..e91d458a098c81 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java @@ -571,6 +571,7 @@ private LogicalAggregate storageLayerAggregate( Map, PushDownAggOp> supportedAgg = PushDownAggOp.supportedFunctions(); boolean containsCount = false; + boolean countHasCastArgument = false; Set checkNullSlots = new HashSet<>(); Set expressionAfterProject = new HashSet<>(); @@ -592,6 +593,7 @@ private LogicalAggregate storageLayerAggregate( checkNullSlots.add((SlotReference) arg0); expressionAfterProject.add(arg0); } else if (arg0 instanceof Cast) { + countHasCastArgument = true; Expression child0 = arg0.child(0); if (child0 instanceof SlotReference) { checkNullSlots.add((SlotReference) child0); @@ -668,6 +670,7 @@ private LogicalAggregate storageLayerAggregate( return canNotPush; } else { if (needCheckSlotNull) { + countHasCastArgument = true; checkNullSlots.add((SlotReference) argument.child(0)); } } @@ -678,6 +681,16 @@ private LogicalAggregate storageLayerAggregate( argumentsOfAggregateFunction = processedExpressions; } + // File aggregate metadata can describe COUNT(*) or COUNT(file_column), but it cannot + // describe the CAST wrapped around a COUNT argument. Dropping that CAST is incorrect even + // when the source column is NOT NULL. For example, a non-null DOUBLE value outside the INT + // range becomes NULL for CAST(double_col AS INT), so COUNT(CAST(double_col AS INT)) must + // exclude it while a footer-level COUNT(double_col) would include it. Keep OLAP's existing + // storage-layer behavior unchanged, and make external files evaluate the CAST normally. + if (logicalScan instanceof LogicalFileScan && countHasCastArgument) { + return canNotPush; + } + Set pushDownAggOps = functionClasses.stream() .map(supportedAgg::get) .collect(Collectors.toSet()); From 3347bb109f68e580038d06f2f1577831dcac8ef5 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Wed, 15 Jul 2026 10:07:15 +0800 Subject: [PATCH 08/17] [test](be) Mark Iceberg table-level count as COUNT star ### What problem does this PR solve? Issue Number: None Related PR: #65548 Problem Summary: The Iceberg table-level COUNT unit test omitted the semantic COUNT argument field. After metadata COUNT was restricted to explicit COUNT(*) plans, the test fell back to position-delete scanning with intentionally omitted reader context and aborted on a missing split cache. Mark the test plan with an explicit empty COUNT argument list so it continues to exercise the assigned table-level row count path. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter="IcebergV2ReaderTest.IcebergTableLevelCountUsesAssignedRowCountWithPositionDelete" -j 32 - Behavior changed: No; test-only correction. - Does this need documentation: No --- be/test/format_v2/table/iceberg_reader_test.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/be/test/format_v2/table/iceberg_reader_test.cpp b/be/test/format_v2/table/iceberg_reader_test.cpp index 0831c98750ecbf..fc1a121a95384c 100644 --- a/be/test/format_v2/table/iceberg_reader_test.cpp +++ b/be/test/format_v2/table/iceberg_reader_test.cpp @@ -1938,6 +1938,9 @@ TEST(IcebergV2ReaderTest, IcebergTableLevelCountUsesAssignedRowCountWithPosition .runtime_state = &state, .scanner_profile = nullptr, .push_down_agg_type = TPushAggOp::type::COUNT, + // An explicit empty argument list is the new FE marker for + // COUNT(*)/COUNT(1); nullopt intentionally exercises fallback. + .push_down_count_columns = std::vector {}, }) .ok()); From 6fb998d6934f9e10ae956ae2c420be65bfcea9da Mon Sep 17 00:00:00 2001 From: Gabriel Date: Wed, 15 Jul 2026 10:48:31 +0800 Subject: [PATCH 09/17] [fix](be) Localize nested predicates in file types ### What problem does this PR solve? Issue Number: None Related PR: #65548 Problem Summary: File-local predicate rewriting always cast an evolved nested struct leaf from its physical file type back to the table type. Although correct, this performed a conversion for every row even when the predicate literal could be represented exactly in the file type. Prefer rewriting binary and IN literals to the physical file leaf type when the file-to-table mapping is lossless and every literal round-trips exactly; otherwise retain the table literal and cast the file leaf. Clarify the separate COUNT(*) and COUNT(column) metadata semantics for both legacy and V2 file scanners. ### Release note Improve evolved nested struct predicate evaluation while preserving exact fallback semantics; clarify file COUNT pushdown behavior. ### Check List (For Author) - Test: Unit Test - ColumnMapperCastTest.* (23 tests passed) - FileScanner and TableReader COUNT pushdown tests (3 tests passed) - Behavior changed: Yes; safe nested predicates compare in the file type, while unsafe conversions fall back to table-type evaluation. - Does this need documentation: No --- be/src/exec/scan/file_scanner.h | 11 +- be/src/format_v2/column_mapper.cpp | 251 ++++++++++++++---- be/src/format_v2/table_reader.h | 4 +- be/test/format_v2/column_mapper_test.cpp | 139 +++++++++- .../test_file_scanner_v2_review_fixes.groovy | 10 +- 5 files changed, 340 insertions(+), 75 deletions(-) diff --git a/be/src/exec/scan/file_scanner.h b/be/src/exec/scan/file_scanner.h index 0cf432e8544703..f575cdae89263f 100644 --- a/be/src/exec/scan/file_scanner.h +++ b/be/src/exec/scan/file_scanner.h @@ -344,10 +344,13 @@ class FileScanner : public Scanner { if (agg_type != TPushAggOp::type::COUNT) { return agg_type; } - // V1's CountReader can only emit rows for an upper COUNT(*). It cannot evaluate the NULL - // or CAST semantics of COUNT(col), so a non-empty argument list must use the normal reader. - // nullopt is an old FE plan that predates the argument field; treating it as empty would - // silently reinterpret unknown semantics as COUNT(*). + // V1's CountReader receives only the file's total row count and emits that many synthetic + // rows. This is exact for COUNT(*)/COUNT(1), but it has no column metadata for NULL or CAST + // semantics. For example, a 10,000-row file with 9,015 non-null values must return 10,000 + // for COUNT(*) and 9,015 for COUNT(nullable_col); CountReader can produce only the former. + // Therefore a non-empty argument list must use the normal reader. nullopt is an old FE plan + // that predates the argument field; treating it as empty would silently reinterpret unknown + // semantics as COUNT(*). return count_slot_ids.has_value() && count_slot_ids->empty() ? TPushAggOp::type::COUNT : TPushAggOp::type::NONE; } diff --git a/be/src/format_v2/column_mapper.cpp b/be/src/format_v2/column_mapper.cpp index 1c2d96f7edbded..04e2de334e8ddf 100644 --- a/be/src/format_v2/column_mapper.cpp +++ b/be/src/format_v2/column_mapper.cpp @@ -844,6 +844,192 @@ static bool rewrite_struct_element_path_to_file_expr( return true; } +static VExprSPtr cast_file_expr_to_table_type(const VExprSPtr& file_expr, + const DataTypePtr& table_type, + RewriteContext* rewrite_context) { + DORIS_CHECK(file_expr != nullptr); + DORIS_CHECK(table_type != nullptr); + DORIS_CHECK(rewrite_context != nullptr); + auto cast_expr = Cast::create_shared(table_type); + cast_expr->add_child(file_expr); + rewrite_context->add_created_expr(cast_expr); + return cast_expr; +} + +// Prefer comparing in the physical file leaf type when a table predicate uses a promoted struct +// child. For example, with table STRUCT, old-file STRUCT, and `s.a = 10`, the +// localized predicate should be `file_s.a::INT = 10::INT`, not +// `CAST(file_s.a::INT AS BIGINT) = 10::BIGINT`. Converting one literal avoids a cast for every row. +// +// This rewrite is valid only when every possible file value survives file-to-table conversion and +// the particular literal survives a table-to-file-to-table round trip. A value such as BIGINT +// 2147483648 cannot be represented by an INT file leaf, so that case deliberately falls back to +// `CAST(file_s.a AS BIGINT) = 2147483648`, which preserves the original table-level semantics. +static bool rewrite_binary_struct_literal_predicate( + const VExprSPtr& expr, const std::vector& filter_mappings, + const std::map& global_to_file_slot, + RewriteContext* rewrite_context) { + if (!is_binary_comparison_predicate(expr)) { + return false; + } + auto children = expr->children(); + int struct_child_idx = -1; + int literal_child_idx = -1; + if (is_struct_element_expr(children[0])) { + struct_child_idx = 0; + literal_child_idx = 1; + } else if (is_struct_element_expr(children[1])) { + struct_child_idx = 1; + literal_child_idx = 0; + } else { + return false; + } + + const auto table_leaf_type = children[struct_child_idx]->data_type(); + DORIS_CHECK(table_leaf_type != nullptr); + auto table_literal = unwrap_literal_for_file_cast(children[literal_child_idx], table_leaf_type); + if (table_literal == nullptr || + !rewrite_struct_element_path_to_file_expr(children[struct_child_idx], filter_mappings, + global_to_file_slot, rewrite_context)) { + return false; + } + + const auto file_leaf_type = children[struct_child_idx]->data_type(); + DORIS_CHECK(file_leaf_type != nullptr); + const FileSlotRewriteInfo leaf_rewrite_info { + .block_position = 0, + .file_type = file_leaf_type, + .table_type = table_leaf_type, + .file_column_name = {}, + }; + auto file_literal = + rewrite_literal_to_file_type(table_literal, leaf_rewrite_info, rewrite_context); + if (file_literal != nullptr) { + children[literal_child_idx] = std::move(file_literal); + } else { + children[struct_child_idx] = cast_file_expr_to_table_type(children[struct_child_idx], + table_leaf_type, rewrite_context); + children[literal_child_idx] = original_table_literal(table_literal, rewrite_context); + } + expr->set_children(std::move(children)); + return true; +} + +// IN must use one comparison type for its probe and every candidate. Rewrite the complete literal +// set only when all values are exactly representable in the file leaf type; one unsafe value makes +// the whole predicate fall back to a table-type cast. For example, an INT file leaf can evaluate +// `BIGINT IN (10, 20)` as `INT IN (10, 20)`, but `BIGINT IN (10, 2147483648)` must stay BIGINT. +static bool rewrite_in_struct_literal_predicate( + const VExprSPtr& expr, const std::vector& filter_mappings, + const std::map& global_to_file_slot, + RewriteContext* rewrite_context) { + if (expr->node_type() != TExprNodeType::IN_PRED || expr->get_num_children() < 2 || + !is_struct_element_expr(expr->children()[0])) { + return false; + } + auto children = expr->children(); + const auto table_leaf_type = children[0]->data_type(); + DORIS_CHECK(table_leaf_type != nullptr); + VExprSPtrs table_literals; + table_literals.reserve(children.size() - 1); + for (size_t child_idx = 1; child_idx < children.size(); ++child_idx) { + auto table_literal = unwrap_literal_for_file_cast(children[child_idx], table_leaf_type); + if (table_literal == nullptr) { + return false; + } + table_literals.push_back(std::move(table_literal)); + } + if (!rewrite_struct_element_path_to_file_expr(children[0], filter_mappings, global_to_file_slot, + rewrite_context)) { + return false; + } + + const auto file_leaf_type = children[0]->data_type(); + DORIS_CHECK(file_leaf_type != nullptr); + const FileSlotRewriteInfo leaf_rewrite_info { + .block_position = 0, + .file_type = file_leaf_type, + .table_type = table_leaf_type, + .file_column_name = {}, + }; + VExprSPtrs file_literals; + file_literals.reserve(table_literals.size()); + for (const auto& table_literal : table_literals) { + auto file_literal = + rewrite_literal_to_file_type(table_literal, leaf_rewrite_info, rewrite_context); + if (file_literal == nullptr) { + children[0] = + cast_file_expr_to_table_type(children[0], table_leaf_type, rewrite_context); + for (size_t literal_idx = 0; literal_idx < table_literals.size(); ++literal_idx) { + children[literal_idx + 1] = + original_table_literal(table_literals[literal_idx], rewrite_context); + } + expr->set_children(std::move(children)); + return true; + } + file_literals.push_back(std::move(file_literal)); + } + + for (size_t literal_idx = 0; literal_idx < file_literals.size(); ++literal_idx) { + children[literal_idx + 1] = std::move(file_literals[literal_idx]); + } + expr->set_children(std::move(children)); + return true; +} + +static VExprSPtr rewrite_struct_or_slot_expr_to_file_expr( + const VExprSPtr& expr, + const std::map& global_to_file_slot, + const std::vector& filter_mappings, RewriteContext* rewrite_context, + bool* can_localize) { + if (is_struct_element_expr(expr)) { + const auto table_leaf_type = expr->data_type(); + if (!rewrite_struct_element_path_to_file_expr(expr, filter_mappings, global_to_file_slot, + rewrite_context)) { + // The scanner still evaluates the original table-level conjunct after TableReader + // finalizes the output block. Skipping an unlocalizable file conjunct is therefore + // safer than preparing a partially rewritten expression against the wrong struct + // layout. In particular, do not generate file-local conjuncts for computed complex + // parents such as `element_at(element_at(map_values(m), 1), 'field')`; only direct + // slot-rooted struct chains are supported here. + *can_localize = false; + return expr; + } + DORIS_CHECK(table_leaf_type != nullptr); + DORIS_CHECK(expr->data_type() != nullptr); + if (!expr->data_type()->equals(*table_leaf_type)) { + // Path localization changes the leaf to the physical file type. For example, after an + // Iceberg evolution from STRUCT to STRUCT, the localized old-file + // predicate is initially `element_at(file_col, 'a')::INT = 10::BIGINT`. Cast only the + // leaf back to BIGINT so the comparison has matching operands without forcing a cast + // of the entire evolved struct (whose children may also have been added or reordered). + return cast_file_expr_to_table_type(expr, table_leaf_type, rewrite_context); + } + return expr; + } + + DORIS_CHECK(expr->is_slot_ref()); + const auto* slot_ref = assert_cast(expr.get()); + const auto rewrite_it = global_to_file_slot.find(slot_ref_global_index(*slot_ref)); + if (rewrite_it == global_to_file_slot.end()) { + return expr; + } + const auto& rewrite_info = rewrite_it->second; + auto file_slot = create_file_slot_ref(*slot_ref, rewrite_info, rewrite_context); + if (rewrite_info.file_type->equals(*rewrite_info.table_type)) { + return file_slot; + } + if (needs_complex_file_slot_cast(rewrite_info.file_type, rewrite_info.table_type)) { + // Generic file-local expressions cannot safely cast an evolved complex file slot back to + // the table type. For example, ARRAY_CONTAINS(MAP_KEYS(m), 'person5') only reads map keys, + // but CAST(file_m AS table_m) first forces an incompatible old value struct into the new + // layout. Keep such predicates at table level, after TableReader materializes evolution. + *can_localize = false; + return expr; + } + return cast_file_expr_to_table_type(file_slot, rewrite_info.table_type, rewrite_context); +} + static VExprSPtr rewrite_table_expr_to_file_expr( const VExprSPtr& expr, const std::map& global_to_file_slot, @@ -875,67 +1061,18 @@ static VExprSPtr rewrite_table_expr_to_file_expr( if (rewrite_in_slot_literal_predicate(expr, global_to_file_slot, rewrite_context)) { return expr; } - if (is_struct_element_expr(expr)) { - const auto table_leaf_type = expr->data_type(); - if (!rewrite_struct_element_path_to_file_expr(expr, filter_mappings, global_to_file_slot, - rewrite_context)) { - // The scanner still evaluates the original table-level conjunct after TableReader - // finalizes the output block. Skipping an unlocalizable file conjunct is therefore - // safer than preparing a partially rewritten expression against the wrong struct - // layout. In particular, do not generate file-local conjuncts for computed complex - // parents such as `element_at(element_at(map_values(m), 1), 'field')`; only direct - // slot-rooted struct chains are supported here. - *can_localize = false; - return expr; - } - DORIS_CHECK(table_leaf_type != nullptr); - DORIS_CHECK(expr->data_type() != nullptr); - if (!expr->data_type()->equals(*table_leaf_type)) { - // Path localization changes the leaf to the physical file type. For example, after an - // Iceberg evolution from STRUCT to STRUCT, the localized old-file - // predicate is initially `element_at(file_col, 'a')::INT = 10::BIGINT`. Cast only the - // leaf back to BIGINT so the comparison has matching operands without forcing a cast - // of the entire evolved struct (whose children may also have been added or reordered). - auto cast_expr = Cast::create_shared(table_leaf_type); - cast_expr->add_child(expr); - rewrite_context->add_created_expr(cast_expr); - return cast_expr; - } + if (rewrite_binary_struct_literal_predicate(expr, filter_mappings, global_to_file_slot, + rewrite_context)) { return expr; } - if (expr->is_slot_ref()) { - const auto* slot_ref = assert_cast(expr.get()); - const auto rewrite_it = global_to_file_slot.find(slot_ref_global_index(*slot_ref)); - if (rewrite_it != global_to_file_slot.end()) { - const auto& rewrite_info = rewrite_it->second; - auto file_slot = create_file_slot_ref(*slot_ref, rewrite_info, rewrite_context); - if (rewrite_info.file_type->equals(*rewrite_info.table_type)) { - return file_slot; - } - if (needs_complex_file_slot_cast(rewrite_info.file_type, rewrite_info.table_type)) { - // Generic file-local expressions cannot safely cast an evolved complex file slot - // back to the table type. Example: - // - // table filter: ARRAY_CONTAINS(MAP_KEYS(m), 'person5') - // old file: m MAP> - // table: m MAP> - // - // Although MAP_KEYS only reads the key column, wrapping the file slot as - // `CAST(file_m AS table_m)` forces the value struct cast first and fails because - // the old and new value structs have different fields. Keep such filters at the - // table level, where TableReader materializes the evolved complex value before - // Scanner evaluates the original conjunct. Direct slot-rooted struct child paths - // are handled by rewrite_struct_element_path_to_file_expr() above. - *can_localize = false; - return expr; - } - auto cast_expr = Cast::create_shared(rewrite_info.table_type); - cast_expr->add_child(std::move(file_slot)); - rewrite_context->add_created_expr(cast_expr); - return cast_expr; - } + if (rewrite_in_struct_literal_predicate(expr, filter_mappings, global_to_file_slot, + rewrite_context)) { return expr; } + if (is_struct_element_expr(expr) || expr->is_slot_ref()) { + return rewrite_struct_or_slot_expr_to_file_expr(expr, global_to_file_slot, filter_mappings, + rewrite_context, can_localize); + } // The input is a split-local cloned tree. A previous split-local clone may already have // inserted Cast(slot). Keep that rewrite idempotent: rewrite the cast child from table slot to // the current split's file slot, and drop the cast when the current split no longer needs it. diff --git a/be/src/format_v2/table_reader.h b/be/src/format_v2/table_reader.h index 558d995a86a90a..49725bd7ccd601 100644 --- a/be/src/format_v2/table_reader.h +++ b/be/src/format_v2/table_reader.h @@ -1493,7 +1493,9 @@ class TableReader { DORIS_CHECK(_push_down_count_columns.has_value()); // An empty explicit list is the semantic signal for COUNT(*). Do not inspect the // mapping count: `SELECT COUNT(*) FROM t` may still project one nullable column because - // the planner keeps a placeholder slot, and counting that slot would return COUNT(col). + // the planner keeps a placeholder slot. In a 10,000-row file where that arbitrary slot + // has 9,015 non-null values, passing the slot would ask Parquet/ORC metadata for + // COUNT(slot)=9,015 instead of the required row count 10,000. if (!_push_down_count_columns->empty()) { const auto& mapping = _push_down_count_mapping(); DORIS_CHECK(mapping.file_local_id.has_value()); diff --git a/be/test/format_v2/column_mapper_test.cpp b/be/test/format_v2/column_mapper_test.cpp index 738e19261c216c..73ea70a122abcc 100644 --- a/be/test/format_v2/column_mapper_test.cpp +++ b/be/test/format_v2/column_mapper_test.cpp @@ -3062,9 +3062,10 @@ TEST(ColumnMapperScanRequestTest, NestedElementAtConjunctUsesFileChildTypeForRen } // Scenario: Iceberg promotes a nested struct leaf from INT to BIGINT while an old file still -// stores INT. The localized element_at must be cast back to the table leaf type so the comparison -// is prepared and executed with matching operand types. -TEST_F(ColumnMapperCastTest, NestedElementAtConjunctCastsPromotedFileLeafToTableType) { +// stores INT. Because 15 is exactly representable as INT and every INT survives promotion to +// BIGINT, localize `s.b::BIGINT > 15::BIGINT` as `file_s.b::INT > 15::INT`. Rewriting one literal +// avoids casting every file value while preserving the table predicate exactly. +TEST_F(ColumnMapperCastTest, NestedElementAtConjunctRewritesExactLiteralToFileType) { const auto file_int_type = i32(); const auto table_bigint_type = i64(); @@ -3089,12 +3090,12 @@ TEST_F(ColumnMapperCastTest, NestedElementAtConjunctCastsPromotedFileLeafToTable ASSERT_EQ(request.conjuncts.size(), 1); const auto& localized_root = request.conjuncts[0]->root(); ASSERT_EQ(localized_root->get_num_children(), 2); - const auto& localized_cast = localized_root->children()[0]; - ASSERT_NE(dynamic_cast(localized_cast.get()), nullptr); - EXPECT_TRUE(localized_cast->data_type()->equals(*table_bigint_type)); - ASSERT_EQ(localized_cast->get_num_children(), 1); - EXPECT_EQ(localized_cast->children()[0]->expr_name(), "element_at"); - EXPECT_TRUE(localized_cast->children()[0]->data_type()->equals(*file_int_type)); + const auto& localized_leaf = localized_root->children()[0]; + EXPECT_EQ(localized_leaf->expr_name(), "element_at"); + EXPECT_TRUE(localized_leaf->data_type()->equals(*file_int_type)); + const auto& localized_literal = localized_root->children()[1]; + EXPECT_TRUE(localized_literal->is_literal()); + EXPECT_TRUE(localized_literal->data_type()->equals(*file_int_type)); auto values = ColumnInt32::create(); values->insert_value(10); @@ -3118,6 +3119,126 @@ TEST_F(ColumnMapperCastTest, NestedElementAtConjunctCastsPromotedFileLeafToTable conjunct->close(); } +// Scenario: the table literal is outside the old file leaf's INT range. Rewriting +// BIGINT 2147483648 to INT would change the predicate, so keep the literal as BIGINT and cast the +// file leaf instead: `CAST(file_s.b::INT AS BIGINT) = 2147483648::BIGINT`. +TEST_F(ColumnMapperCastTest, NestedElementAtConjunctFallsBackForOutOfRangeLiteral) { + const auto file_int_type = i32(); + const auto table_bigint_type = i64(); + + auto table_b = field_id_col("b", 11, table_bigint_type); + auto table_struct = struct_col("s", 10, {table_b}); + auto file_b = field_id_col("b", 11, file_int_type, 0); + auto file_struct = struct_col("s", 10, {file_b}, 5); + auto table_leaf = executable_struct_element( + table_slot(0, 0, table_struct.type, table_struct.name), table_bigint_type, "b"); + auto filter_expr = executable_binary_predicate( + TExprOpcode::EQ, table_leaf, + literal(table_bigint_type, Field::create_field(2147483648LL))); + TableFilter filter {.conjunct = VExprContext::create_shared(filter_expr), + .global_indices = {GlobalIndex(0)}}; + + TableColumnMapper mapper({.mode = TableColumnMappingMode::BY_FIELD_ID}); + ASSERT_TRUE(mapper.create_mapping({table_struct}, {}, {file_struct}).ok()); + FileScanRequest request; + ASSERT_TRUE(mapper.create_scan_request({filter}, {table_struct}, &request, &state).ok()); + + ASSERT_EQ(request.conjuncts.size(), 1); + const auto& localized_root = request.conjuncts[0]->root(); + ASSERT_EQ(localized_root->get_num_children(), 2); + const auto& localized_cast = localized_root->children()[0]; + ASSERT_NE(dynamic_cast(localized_cast.get()), nullptr); + EXPECT_TRUE(localized_cast->data_type()->equals(*table_bigint_type)); + ASSERT_EQ(localized_cast->get_num_children(), 1); + EXPECT_EQ(localized_cast->children()[0]->expr_name(), "element_at"); + EXPECT_TRUE(localized_cast->children()[0]->data_type()->equals(*file_int_type)); + EXPECT_TRUE(localized_root->children()[1]->data_type()->equals(*table_bigint_type)); +} + +// Scenario: the struct leaf is on the right side of the comparison. Literal localization must not +// depend on operand order: `15::BIGINT > s.b::BIGINT` becomes `15::INT > file_s.b::INT`. +TEST_F(ColumnMapperCastTest, NestedElementAtConjunctRewritesReverseComparisonLiteral) { + const auto file_int_type = i32(); + const auto table_bigint_type = i64(); + + auto table_b = field_id_col("b", 11, table_bigint_type); + auto table_struct = struct_col("s", 10, {table_b}); + auto file_b = field_id_col("b", 11, file_int_type, 0); + auto file_struct = struct_col("s", 10, {file_b}, 5); + auto table_leaf = executable_struct_element( + table_slot(0, 0, table_struct.type, table_struct.name), table_bigint_type, "b"); + auto filter_expr = executable_binary_predicate( + TExprOpcode::GT, literal(table_bigint_type, Field::create_field(15)), + table_leaf); + TableFilter filter {.conjunct = VExprContext::create_shared(filter_expr), + .global_indices = {GlobalIndex(0)}}; + + TableColumnMapper mapper({.mode = TableColumnMappingMode::BY_FIELD_ID}); + ASSERT_TRUE(mapper.create_mapping({table_struct}, {}, {file_struct}).ok()); + FileScanRequest request; + ASSERT_TRUE(mapper.create_scan_request({filter}, {table_struct}, &request, &state).ok()); + + ASSERT_EQ(request.conjuncts.size(), 1); + const auto& localized_root = request.conjuncts[0]->root(); + EXPECT_TRUE(localized_root->children()[0]->data_type()->equals(*file_int_type)); + EXPECT_TRUE(localized_root->children()[0]->is_literal()); + EXPECT_EQ(localized_root->children()[1]->expr_name(), "element_at"); + EXPECT_TRUE(localized_root->children()[1]->data_type()->equals(*file_int_type)); +} + +// Scenario: IN uses one probe type for every candidate. All exact literals may move to the INT +// file domain, but one out-of-range literal makes the complete IN predicate fall back to BIGINT. +TEST_F(ColumnMapperCastTest, NestedElementAtInPredicateUsesAllOrNothingLiteralRewrite) { + const auto file_int_type = i32(); + const auto table_bigint_type = i64(); + auto table_b = field_id_col("b", 11, table_bigint_type); + auto table_struct = struct_col("s", 10, {table_b}); + auto file_b = field_id_col("b", 11, file_int_type, 0); + auto file_struct = struct_col("s", 10, {file_b}, 5); + + const auto build_filter = [&](int64_t second_value) { + auto table_leaf = executable_struct_element( + table_slot(0, 0, table_struct.type, table_struct.name), table_bigint_type, "b"); + auto predicate = create_in_predicate(); + predicate->add_child(table_leaf); + predicate->add_child(literal(table_bigint_type, Field::create_field(10))); + predicate->add_child( + literal(table_bigint_type, Field::create_field(second_value))); + return TableFilter {.conjunct = VExprContext::create_shared(predicate), + .global_indices = {GlobalIndex(0)}}; + }; + + TableColumnMapper exact_mapper({.mode = TableColumnMappingMode::BY_FIELD_ID}); + ASSERT_TRUE(exact_mapper.create_mapping({table_struct}, {}, {file_struct}).ok()); + FileScanRequest exact_request; + ASSERT_TRUE( + exact_mapper + .create_scan_request({build_filter(20)}, {table_struct}, &exact_request, &state) + .ok()); + ASSERT_EQ(exact_request.conjuncts.size(), 1); + const auto& exact_root = exact_request.conjuncts[0]->root(); + EXPECT_EQ(exact_root->children()[0]->expr_name(), "element_at"); + for (const auto& child : exact_root->children()) { + EXPECT_TRUE(child->data_type()->equals(*file_int_type)); + } + + TableColumnMapper fallback_mapper({.mode = TableColumnMappingMode::BY_FIELD_ID}); + ASSERT_TRUE(fallback_mapper.create_mapping({table_struct}, {}, {file_struct}).ok()); + FileScanRequest fallback_request; + ASSERT_TRUE(fallback_mapper + .create_scan_request({build_filter(2147483648LL)}, {table_struct}, + &fallback_request, &state) + .ok()); + ASSERT_EQ(fallback_request.conjuncts.size(), 1); + const auto& fallback_root = fallback_request.conjuncts[0]->root(); + const auto& fallback_cast = fallback_root->children()[0]; + ASSERT_NE(dynamic_cast(fallback_cast.get()), nullptr); + EXPECT_TRUE(fallback_cast->data_type()->equals(*table_bigint_type)); + EXPECT_TRUE(fallback_cast->children()[0]->data_type()->equals(*file_int_type)); + EXPECT_TRUE(fallback_root->children()[1]->data_type()->equals(*table_bigint_type)); + EXPECT_TRUE(fallback_root->children()[2]->data_type()->equals(*table_bigint_type)); +} + // Scenario: output projection reads one struct child while the row filter reads a different nested // struct child. File-local conjunct rewrite must use the merged scan projection type. In the SQL // shape below, `SELECT element_at(s, 'c') WHERE element_at(element_at(s, 'b'), 'cc') LIKE ...` diff --git a/regression-test/suites/external_table_p0/tvf/test_file_scanner_v2_review_fixes.groovy b/regression-test/suites/external_table_p0/tvf/test_file_scanner_v2_review_fixes.groovy index d62435ac1c38f2..6fa29c321c8e72 100644 --- a/regression-test/suites/external_table_p0/tvf/test_file_scanner_v2_review_fixes.groovy +++ b/regression-test/suites/external_table_p0/tvf/test_file_scanner_v2_review_fixes.groovy @@ -54,10 +54,12 @@ suite("test_file_scanner_v2_review_fixes", "p0,external") { ORDER BY id """ - // The first file defines STRUCT, while the second stores a as INT. The predicate is - // localized independently for each split, so the INT leaf must be cast back to BIGINT before - // comparison. Repeating the multi-file read also exercises per-reader page-cache range state: - // closing one file must not leak its range directory into the next file or the warm scan. + // The first file defines STRUCT, while the second stores a as INT. Localization is + // split-specific: the BIGINT file compares BIGINT values, while the old INT file safely rewrites + // the exactly representable BIGINT literal 10 to INT and compares in the physical file type. + // If a literal cannot round-trip through INT, the mapper instead casts the INT data to BIGINT. + // Repeating the multi-file read also verifies that neither predicate rewrites nor page-cache + // range state leak from one file schema into the next file or the warm scan. def evolvedStructQuery = """ SELECT id, col.a, col.b FROM local( From 6260e28e377f2d30835c185910b84972eab23dc0 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Wed, 15 Jul 2026 13:49:52 +0800 Subject: [PATCH 10/17] [fix](be) Preserve file predicate schema contracts ### What problem does this PR solve? Issue Number: None Related PR: #65548 Problem Summary: File-local nested predicate rewriting ignored nullable-file to required-table schema contracts, allowing NULL rows to be filtered before TableReader could reject them. Iceberg equality-delete also used an arbitrary physical column as a row-count carrier when all keys were missing, so an unsupported unprojected Parquet leaf could fail an otherwise valid projection. Keep such nested predicates above TableReader and use virtual row position as the equality-delete carrier. ### Release note Fix schema-evolved nested predicate validation and Iceberg equality-delete scans with unsupported unprojected Parquet columns. ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter="ColumnMapperCastTest.NestedElementAt*:IcebergV2ReaderTest.IcebergEqualityDelete*" -j 96 (19 tests passed) - Behavior changed: Yes. Unsafe nested predicates remain table-level, and missing-key equality deletes use virtual row position for batch sizing. - Does this need documentation: No --- be/src/format_v2/column_mapper.cpp | 29 ++++++++ be/src/format_v2/table/iceberg_reader.cpp | 15 ++-- be/test/format_v2/column_mapper_test.cpp | 32 ++++++++ .../format_v2/table/iceberg_reader_test.cpp | 73 +++++++++++++++++++ 4 files changed, 141 insertions(+), 8 deletions(-) diff --git a/be/src/format_v2/column_mapper.cpp b/be/src/format_v2/column_mapper.cpp index 04e2de334e8ddf..5008e0bdb20669 100644 --- a/be/src/format_v2/column_mapper.cpp +++ b/be/src/format_v2/column_mapper.cpp @@ -790,6 +790,19 @@ static bool collect_struct_element_chain(const VExprSPtr& expr, std::vector, table + // STRUCT, rows [NULL, 20], and `s.a > 10`, filtering in the file domain would drop + // NULL first and hide the table-contract violation. The reverse direction is safe: a required + // file value can always be wrapped as a nullable table value after filtering. + return !file_type->is_nullable() || table_type->is_nullable(); +} + static bool rewrite_struct_element_path_to_file_expr( const VExprSPtr& expr, const std::vector& mappings, const std::map& global_to_file_slot, @@ -816,6 +829,22 @@ static bool rewrite_struct_element_path_to_file_expr( return false; } + // Check every value-producing level, including the root struct. A nullable parent also makes + // a child access nullable even when the child type itself is required, so checking only the + // final leaf is insufficient. If any file level is more nullable than its table counterpart, + // keep the complete predicate above TableReader so schema validation observes all NULLs before + // row filtering. + if (!can_filter_before_table_nullability_alignment(rewrite_it->second.file_type, + rewrite_it->second.table_type)) { + return false; + } + for (size_t idx = 0; idx < struct_element_chain.size(); ++idx) { + if (!can_filter_before_table_nullability_alignment( + resolved.file_child_types[idx], struct_element_chain[idx]->data_type())) { + return false; + } + } + // File-local conjuncts are prepared against the file-reader Block, so both the root slot and // every struct selector must be expressed in file schema terms. For a renamed Iceberg field, // keeping the table selector would prepare `element_at(file_struct, 'renamed')` and diff --git a/be/src/format_v2/table/iceberg_reader.cpp b/be/src/format_v2/table/iceberg_reader.cpp index 962a414a4c23f1..75df8e4bd6cf20 100644 --- a/be/src/format_v2/table/iceberg_reader.cpp +++ b/be/src/format_v2/table/iceberg_reader.cpp @@ -568,14 +568,13 @@ void IcebergTableReader::_append_equality_delete_row_count_carrier( DORIS_CHECK(request != nullptr); // Columnar readers establish a filter batch's row count from predicate columns. If all // equality keys are missing, the predicate consists only of NULL literals and the filter block - // would otherwise have zero rows. Read one physical column eagerly as a row-count carrier; - // normal final materialization ignores this hidden dependency. - const auto carrier_it = std::ranges::find_if( - _data_reader.file_schema, [](const format::ColumnDefinition& field) { - return field.column_type == format::ColumnType::DATA_COLUMN; - }); - DORIS_CHECK(carrier_it != _data_reader.file_schema.end()); - _append_file_scan_column(request, format::LocalColumnId(carrier_it->file_local_id()), + // would otherwise have zero rows. Use the virtual row-position column as the carrier instead + // of an arbitrary physical column. For example, a data file may start with an unsupported + // TIME_MILLIS leaf while the query projects only a supported `id`; selecting that TIME leaf as + // a hidden carrier would make Parquet reject a column the query never requested. Row position + // has one value per input row in both Parquet and ORC, is already used by delete predicates, + // and is explicitly excluded from physical logical-type validation. + _append_file_scan_column(request, format::LocalColumnId(format::ROW_POSITION_COLUMN_ID), &request->predicate_columns); } diff --git a/be/test/format_v2/column_mapper_test.cpp b/be/test/format_v2/column_mapper_test.cpp index 73ea70a122abcc..82189ad8acb7cf 100644 --- a/be/test/format_v2/column_mapper_test.cpp +++ b/be/test/format_v2/column_mapper_test.cpp @@ -3119,6 +3119,38 @@ TEST_F(ColumnMapperCastTest, NestedElementAtConjunctRewritesExactLiteralToFileTy conjunct->close(); } +// Scenario: an old file allows NULL for a nested leaf that the current table declares required. +// Although every non-NULL INT value and the literal 15 can be promoted to BIGINT exactly, the +// predicate must stay above TableReader. If `file_s.b > 15` ran first for rows [NULL, 20], it would +// discard NULL and prevent table-schema materialization from reporting the nullable-to-required +// contract violation. +TEST_F(ColumnMapperCastTest, + NestedElementAtConjunctStaysTableLevelForNullableFileLeafMappedToRequiredTableLeaf) { + const auto file_nullable_int_type = make_nullable(i32()); + const auto table_bigint_type = i64(); + + auto table_b = field_id_col("b", 11, table_bigint_type); + auto table_struct = struct_col("s", 10, {table_b}); + auto file_b = field_id_col("b", 11, file_nullable_int_type, 0); + auto file_struct = struct_col("s", 10, {file_b}, 5); + + auto table_leaf = executable_struct_element( + table_slot(0, 0, table_struct.type, table_struct.name), table_bigint_type, "b"); + auto filter_expr = executable_binary_predicate( + TExprOpcode::GT, table_leaf, + literal(table_bigint_type, Field::create_field(15))); + TableFilter filter {.conjunct = VExprContext::create_shared(filter_expr), + .global_indices = {GlobalIndex(0)}}; + + TableColumnMapper mapper({.mode = TableColumnMappingMode::BY_FIELD_ID}); + ASSERT_TRUE(mapper.create_mapping({table_struct}, {}, {file_struct}).ok()); + + FileScanRequest request; + ASSERT_TRUE(mapper.create_scan_request({filter}, {table_struct}, &request, &state).ok()); + ASSERT_EQ(request.predicate_columns.size(), 1); + EXPECT_TRUE(request.conjuncts.empty()); +} + // Scenario: the table literal is outside the old file leaf's INT range. Rewriting // BIGINT 2147483648 to INT would change the predicate, so keep the literal as BIGINT and cast the // file leaf instead: `CAST(file_s.b::INT AS BIGINT) = 2147483648::BIGINT`. diff --git a/be/test/format_v2/table/iceberg_reader_test.cpp b/be/test/format_v2/table/iceberg_reader_test.cpp index fc1a121a95384c..cbcf6d1725676d 100644 --- a/be/test/format_v2/table/iceberg_reader_test.cpp +++ b/be/test/format_v2/table/iceberg_reader_test.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -462,6 +463,39 @@ void write_single_int_parquet_file(const std::string& file_path, const std::stri builder.build())); } +void write_unsupported_time_first_int_parquet_file(const std::string& file_path, + const std::vector& ids) { + auto file_result = arrow::io::FileOutputStream::Open(file_path); + ASSERT_TRUE(file_result.ok()) << file_result.status(); + std::shared_ptr out = *file_result; + + // Arrow writes time32 as isAdjustedToUTC=false, which Doris supports. Use Parquet's low-level + // writer so the first physical column is the deliberately unsupported adjusted TIME_MILLIS + // shape from the review report. The query will project only the following supported `id`. + const auto unsupported_time = ::parquet::schema::PrimitiveNode::Make( + "unsupported_time", ::parquet::Repetition::REQUIRED, + ::parquet::LogicalType::Time(true, ::parquet::LogicalType::TimeUnit::MILLIS), + ::parquet::Type::INT32); + const auto id = ::parquet::schema::PrimitiveNode::Make("id", ::parquet::Repetition::REQUIRED, + ::parquet::Type::INT32); + const auto schema_node = ::parquet::schema::GroupNode::Make( + "schema", ::parquet::Repetition::REQUIRED, {unsupported_time, id}); + const auto schema = std::static_pointer_cast<::parquet::schema::GroupNode>(schema_node); + + auto writer = ::parquet::ParquetFileWriter::Open(out, schema); + auto* row_group = writer->AppendRowGroup(); + auto* time_writer = static_cast<::parquet::Int32Writer*>(row_group->NextColumn()); + std::vector times(ids.size(), 1000); + const auto row_count = cast_set(ids.size()); + EXPECT_EQ(time_writer->WriteBatch(row_count, nullptr, nullptr, times.data()), row_count); + time_writer->Close(); + auto* id_writer = static_cast<::parquet::Int32Writer*>(row_group->NextColumn()); + EXPECT_EQ(id_writer->WriteBatch(row_count, nullptr, nullptr, ids.data()), row_count); + id_writer->Close(); + row_group->Close(); + writer->Close(); +} + void write_two_int_parquet_file(const std::string& file_path, const std::string& first_name, const std::vector& first_values, std::optional first_field_id, @@ -2150,6 +2184,45 @@ TEST(IcebergV2ReaderTest, IcebergEqualityDeleteMatchesNullForMissingDataColumn) std::filesystem::remove_all(test_dir); } +TEST(IcebergV2ReaderTest, IcebergEqualityDeleteMissingKeyDoesNotReadUnsupportedUnprojectedCarrier) { + const auto test_dir = std::filesystem::temp_directory_path() / + "doris_iceberg_equality_delete_virtual_carrier_test"; + std::filesystem::remove_all(test_dir); + std::filesystem::create_directories(test_dir); + + const auto file_path = (test_dir / "split.parquet").string(); + const auto delete_file_path = (test_dir / "equality-delete.parquet").string(); + write_unsupported_time_first_int_parquet_file(file_path, {1, 2, 3}); + write_iceberg_equality_delete_parquet_file(delete_file_path, 1, 7, "added_column"); + + std::vector projected_columns; + projected_columns.push_back(make_table_column(0, "id", std::make_shared())); + + RuntimeProfile profile("test_profile"); + RuntimeState state {TQueryOptions(), TQueryGlobals()}; + auto scan_params = make_local_parquet_scan_params(); + io::FileReaderStats file_reader_stats; + io::FileCacheStatistics file_cache_stats; + auto io_ctx = make_io_context(&file_reader_stats, &file_cache_stats); + ShardedKVCache cache(1); + doris::format::iceberg::IcebergTableReader reader; + init_iceberg_reader(&reader, projected_columns, &scan_params, io_ctx, &state, &profile); + + auto split_options = build_split_options(file_path); + split_options.cache = &cache; + split_options.current_range.__set_table_format_params(make_iceberg_table_format_desc( + file_path, {make_iceberg_equality_delete_file(delete_file_path, {1})})); + ASSERT_TRUE(reader.prepare_split(split_options).ok()); + + // The missing data key is NULL, so it does not match delete key 7. More importantly, the + // hidden row-count dependency must use virtual row position instead of the first physical + // TIME_MILLIS column, which is unsupported and was not requested by this `id` projection. + EXPECT_EQ(read_iceberg_ids(&reader, projected_columns), std::vector({1, 2, 3})); + + ASSERT_TRUE(reader.close().ok()); + std::filesystem::remove_all(test_dir); +} + TEST(IcebergV2ReaderTest, IcebergEqualityDeleteMatchesInitialDefaultForMissingDataColumn) { const auto test_dir = std::filesystem::temp_directory_path() / "doris_iceberg_equality_delete_missing_default_test"; From 128b573c00ecef671ef017a260cfb4fc1a6d4ab4 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Wed, 15 Jul 2026 17:33:47 +0800 Subject: [PATCH 11/17] [fix](fe) Preserve COUNT column file splits ### What problem does this PR solve? Issue Number: None Related PR: #65548 Problem Summary: Iceberg and Paimon table-level COUNT planning used only the aggregate opcode, so COUNT(column) could discard most real data splits and attach a metadata row count. Backend correctly rejects that shortcut for explicit COUNT arguments and falls back to reading the column, but then scans only the retained representatives and undercounts multi-split tables. Restrict FE metadata COUNT split reduction to the explicit COUNT(*) marker and preserve all real splits for COUNT(column). ### Release note Fix incorrect COUNT(column) results on multi-split Iceberg and Paimon tables when table-level count pushdown is enabled. ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.datasource.iceberg.source.IcebergScanNodeTest,org.apache.doris.datasource.paimon.source.PaimonScanNodeTest (23 tests passed) - Behavior changed: Yes. Only COUNT(*)/COUNT(1) may replace real Iceberg or Paimon splits with metadata count representatives; COUNT(column) keeps all real splits. - Does this need documentation: No --- .../apache/doris/datasource/FileScanNode.java | 15 ++++++ .../iceberg/source/IcebergScanNode.java | 4 +- .../paimon/source/PaimonScanNode.java | 5 +- .../iceberg/source/IcebergScanNodeTest.java | 51 +++++++++++++++++++ .../paimon/source/PaimonScanNodeTest.java | 50 ++++++++++++++++++ 5 files changed, 120 insertions(+), 5 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/FileScanNode.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/FileScanNode.java index bfcbb24489176e..53b0d11c5c7283 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/FileScanNode.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/FileScanNode.java @@ -105,6 +105,21 @@ protected void setPushDownCount(long count) { tableLevelRowCount = count; } + /** + * Return whether FE may replace real table-format splits with metadata COUNT splits. + * + *

The aggregate opcode alone is insufficient because both {@code COUNT(*)} and + * {@code COUNT(col)} use {@link TPushAggOp#COUNT}. The semantic argument list distinguishes + * them: it is empty only for {@code COUNT(*)}/{@code COUNT(1)}. For example, if an Iceberg + * table has 100 data files, retaining one representative split is correct for a snapshot + * {@code COUNT(*)}. Doing that for {@code COUNT(required_col)} is unsafe: BE deliberately + * falls back to reading the column, but it would then see only the representative file and + * undercount the table. + */ + protected boolean isTableLevelCountStarPushdown() { + return pushDownAggNoGroupingOp == TPushAggOp.COUNT && pushDownCountSlotIds.isEmpty(); + } + private long getPushDownCount() { return tableLevelRowCount; } diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java index b592883f860344..00e6362350baf3 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java @@ -63,7 +63,6 @@ import org.apache.doris.thrift.TIcebergDeleteFileDesc; import org.apache.doris.thrift.TIcebergFileDesc; import org.apache.doris.thrift.TPlanNode; -import org.apache.doris.thrift.TPushAggOp; import org.apache.doris.thrift.TTableFormatFileDesc; import com.google.common.annotations.VisibleForTesting; @@ -1338,8 +1337,7 @@ public boolean isBatchMode() { if (cached != null) { return cached; } - TPushAggOp aggOp = getPushDownAggNoGroupingOp(); - if (aggOp.equals(TPushAggOp.COUNT)) { + if (isTableLevelCountStarPushdown()) { try { countFromSnapshot = getCountFromSnapshot(); } catch (UserException e) { diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java index da3e50a6be46e4..990e79923f0a93 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java @@ -48,7 +48,6 @@ import org.apache.doris.thrift.TPaimonDeletionFileDesc; import org.apache.doris.thrift.TPaimonFileDesc; import org.apache.doris.thrift.TPaimonReaderType; -import org.apache.doris.thrift.TPushAggOp; import org.apache.doris.thrift.TTableFormatFileDesc; import com.google.common.annotations.VisibleForTesting; @@ -405,7 +404,9 @@ public List getSplits(int numBackends) throws UserException { ++paimonSplitNum; } - boolean applyCountPushdown = getPushDownAggNoGroupingOp() == TPushAggOp.COUNT; + // Merged row counts contain only COUNT(*) semantics. COUNT(col) must keep every DataSplit + // because BE will read the argument column to account for NULL and schema-mapping rules. + boolean applyCountPushdown = isTableLevelCountStarPushdown(); // Used to avoid repeatedly calculating partition info map for the same // partition data. // And for counting the number of selected partitions for this paimon table. diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/iceberg/source/IcebergScanNodeTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/iceberg/source/IcebergScanNodeTest.java index d4273f6faa4cc5..3505a5c5d11e42 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/iceberg/source/IcebergScanNodeTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/iceberg/source/IcebergScanNodeTest.java @@ -32,6 +32,7 @@ import org.apache.doris.system.Backend; import org.apache.doris.thrift.TFileRangeDesc; import org.apache.doris.thrift.TIcebergDeleteFileDesc; +import org.apache.doris.thrift.TPushAggOp; import org.apache.iceberg.DataFile; import org.apache.iceberg.FileFormat; @@ -154,6 +155,56 @@ public void testSystemTableProjectionRejectsUnmaterializedFilterColumn() throws } } + private static class CountPlanningIcebergScanNode extends IcebergScanNode { + private final TableScan tableScan; + private final long snapshotCount; + private int snapshotCountCalls; + + CountPlanningIcebergScanNode(SessionVariable sv, TableScan tableScan, long snapshotCount) { + super(new PlanNodeId(0), new TupleDescriptor(new TupleId(0)), sv, ScanContext.EMPTY); + this.tableScan = tableScan; + this.snapshotCount = snapshotCount; + } + + @Override + public TableScan createTableScan() { + return tableScan; + } + + @Override + public long getCountFromSnapshot() { + ++snapshotCountCalls; + return snapshotCount; + } + } + + @Test + public void testTableLevelCountSplitPlanningRequiresCountStar() { + SessionVariable sv = Mockito.mock(SessionVariable.class); + Mockito.when(sv.getEnableExternalTableBatchMode()).thenReturn(false); + TableScan tableScan = Mockito.mock(TableScan.class); + Mockito.when(tableScan.snapshot()).thenReturn(Mockito.mock(Snapshot.class)); + + // COUNT(required_col) carries a non-empty semantic argument list. Even though its result + // equals COUNT(*) for valid data, BE intentionally reads the column to enforce schema + // contracts. FE must therefore leave all real file tasks available to that fallback. + CountPlanningIcebergScanNode countColumnNode = + new CountPlanningIcebergScanNode(sv, tableScan, 30_000); + countColumnNode.setPushDownAggNoGrouping(TPushAggOp.COUNT); + countColumnNode.setPushDownCountSlotIds(Collections.singletonList(new SlotId(7))); + Assert.assertFalse(countColumnNode.isBatchMode()); + Assert.assertEquals(0, countColumnNode.snapshotCountCalls); + + // COUNT(*) has an explicitly empty argument list, so snapshot row count remains eligible + // and doGetSplits may retain only representative tasks for parallel materialization. + CountPlanningIcebergScanNode countStarNode = + new CountPlanningIcebergScanNode(sv, tableScan, 30_000); + countStarNode.setPushDownAggNoGrouping(TPushAggOp.COUNT); + countStarNode.setPushDownCountSlotIds(Collections.emptyList()); + Assert.assertFalse(countStarNode.isBatchMode()); + Assert.assertEquals(1, countStarNode.snapshotCountCalls); + } + @Test public void testInitialDefaultMetadataUsesSnapshotSchema() throws Exception { Schema snapshotSchema = new Schema(Types.NestedField.optional("historical_binary") diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/source/PaimonScanNodeTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/source/PaimonScanNodeTest.java index 034be2185e20f9..9f8583ed1bc860 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/source/PaimonScanNodeTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/source/PaimonScanNodeTest.java @@ -17,6 +17,7 @@ package org.apache.doris.datasource.paimon.source; +import org.apache.doris.analysis.SlotId; import org.apache.doris.analysis.TupleDescriptor; import org.apache.doris.analysis.TupleId; import org.apache.doris.common.ExceptionChecker; @@ -35,6 +36,7 @@ import org.apache.doris.qe.SessionVariable; import org.apache.doris.thrift.TFileRangeDesc; import org.apache.doris.thrift.TFileScanRangeParams; +import org.apache.doris.thrift.TPushAggOp; import org.apache.paimon.data.BinaryRow; import org.apache.paimon.io.DataFileMeta; @@ -68,6 +70,38 @@ public class PaimonScanNodeTest { @Mock private PaimonFileExternalCatalog paimonFileExternalCatalog; + @Test + public void testCountColumnKeepsAllSplitsWhileCountStarUsesMergedRowCount() throws UserException { + PaimonScanNode node = Mockito.spy(newTestNode(new PlanNodeId(1), new TupleId(3), sv)); + node.setSource(mockPaimonSourceWithPartitionKeys(Collections.emptyList())); + List dataSplits = Arrays.asList( + mockCountDataSplit("f1.parquet", 4_000), + mockCountDataSplit("f2.parquet", 5_000), + mockCountDataSplit("f3.parquet", 6_000)); + Mockito.doReturn(dataSplits).when(node).getPaimonSplitFromAPI(); + Mockito.when(sv.isForceJniScanner()).thenReturn(true); + Mockito.when(sv.getIgnoreSplitType()).thenReturn("NONE"); + Mockito.when(sv.getParallelExecInstanceNum(ArgumentMatchers.nullable(String.class))).thenReturn(1); + + // Before the fix, the raw COUNT opcode made this path keep only parallel representative + // splits and attach the 15,000 metadata rows. BE rejects that shortcut for COUNT(col), so + // it would scan only those representatives and silently miss the discarded DataSplits. + node.setPushDownAggNoGrouping(TPushAggOp.COUNT); + node.setPushDownCountSlotIds(Collections.singletonList(new SlotId(7))); + List countColumnSplits = node.getSplits(1); + Assert.assertEquals(3, countColumnSplits.size()); + for (org.apache.doris.spi.Split split : countColumnSplits) { + Assert.assertFalse(((PaimonSplit) split).getRowCount().isPresent()); + } + + // COUNT(*) remains metadata-only. The 15,000 rows exceed the parallel threshold, so one + // configured execution instance retains one representative split carrying the full sum. + node.setPushDownCountSlotIds(Collections.emptyList()); + List countStarSplits = node.getSplits(1); + Assert.assertEquals(1, countStarSplits.size()); + Assert.assertEquals(Optional.of(15_000L), ((PaimonSplit) countStarSplits.get(0)).getRowCount()); + } + @Test public void testSplitWeight() throws UserException { @@ -692,4 +726,20 @@ private DataSplit createDataSplit(String fileName) { .withDataFiles(Collections.singletonList(dataFileMeta)) .build(); } + + private DataSplit mockCountDataSplit(String fileName, long rowCount) { + DataFileMeta dataFileMeta = DataFileMeta.forAppend(fileName, 64L * 1024 * 1024, rowCount, + SimpleStats.EMPTY_STATS, 1L, 1L, 1L, Collections.emptyList(), null, + FileSource.APPEND, Collections.emptyList(), null, null, + Collections.emptyList()); + DataSplit dataSplit = Mockito.mock(DataSplit.class); + Mockito.when(dataSplit.rowCount()).thenReturn(rowCount); + Mockito.when(dataSplit.mergedRowCountAvailable()).thenReturn(true); + Mockito.when(dataSplit.mergedRowCount()).thenReturn(rowCount); + Mockito.when(dataSplit.partition()).thenReturn(BinaryRow.singleColumn(1)); + Mockito.when(dataSplit.dataFiles()).thenReturn(Collections.singletonList(dataFileMeta)); + Mockito.when(dataSplit.convertToRawFiles()).thenReturn(Optional.empty()); + Mockito.when(dataSplit.deletionFiles()).thenReturn(Optional.empty()); + return dataSplit; + } } From e2db6d456cc3fe97c5953c171d82334c3f7a46d1 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Wed, 15 Jul 2026 21:36:19 +0800 Subject: [PATCH 12/17] [fix](be) Ignore COUNT star placeholder validation ### What problem does this PR solve? Issue Number: None Related PR: #65548 Problem Summary: Explicit COUNT(*) keeps a planner placeholder scan slot even though file aggregate metadata needs no column. TableReader opened the normal Parquet scan request before aggregate materialization, so an unsupported logical leaf such as adjusted TIME_MILLIS could be rejected as if it were user-projected and make COUNT(*) fail. Mark non-predicate columns as placeholders only after the existing COUNT(*) aggregate safety gate succeeds, and skip unsupported-type validation only for those placeholders while preserving strict predicate, COUNT(column), and ordinary projection validation. ### Release note Fix COUNT(*) failures on Parquet files where column pruning retains an unsupported logical-type placeholder. ### Check List (For Author) - Test: Unit Test - run-be-ut.sh targeted 4 tests (all passed) - Behavior changed: Yes. Eligible COUNT(*) metadata pushdown ignores planner-only non-predicate placeholder validation; actual projections and COUNT(column) remain unchanged. - Does this need documentation: No --- be/src/format_v2/file_reader.cpp | 4 ++- be/src/format_v2/file_reader.h | 6 ++++ be/src/format_v2/parquet/parquet_reader.cpp | 6 ++-- be/src/format_v2/table_reader.h | 10 ++++++ .../format_v2/parquet/parquet_scan_test.cpp | 32 +++++++++++++++++++ be/test/format_v2/table_reader_test.cpp | 4 +++ 6 files changed, 59 insertions(+), 3 deletions(-) diff --git a/be/src/format_v2/file_reader.cpp b/be/src/format_v2/file_reader.cpp index 4f5b247c791efd..910566d46e3fa6 100644 --- a/be/src/format_v2/file_reader.cpp +++ b/be/src/format_v2/file_reader.cpp @@ -62,7 +62,9 @@ std::string FileScanRequest::debug_string() const { out << column_id << ":" << block_position; } out << "}, conjunct_count=" << conjuncts.size() - << ", delete_conjunct_count=" << delete_conjuncts.size() << "}"; + << ", delete_conjunct_count=" << delete_conjuncts.size() + << ", non_predicate_columns_are_count_star_placeholders=" + << non_predicate_columns_are_count_star_placeholders << "}"; return out.str(); } diff --git a/be/src/format_v2/file_reader.h b/be/src/format_v2/file_reader.h index 59f684121ce1e3..4c31dddf76d764 100644 --- a/be/src/format_v2/file_reader.h +++ b/be/src/format_v2/file_reader.h @@ -77,6 +77,12 @@ struct FileScanRequest { // Delete predicates converted to file-local expressions. A TRUE result means that the row is // deleted, so readers must invert each result when building their keep filter. VExprContextSPtrs delete_conjuncts; + // True only when TableReader has proved that an explicit COUNT(*) can be answered by the + // physical reader's aggregate metadata path. Nereids still retains one scan slot after column + // pruning, but every non-predicate column in this request is then a row-producing placeholder, + // not a user-requested value. Readers may skip semantic validation of those placeholders before + // aggregate materialization; predicate columns must always be validated normally. + bool non_predicate_columns_are_count_star_placeholders = false; }; // Helper for constructing the scan-column layout in FileScanRequest. diff --git a/be/src/format_v2/parquet/parquet_reader.cpp b/be/src/format_v2/parquet/parquet_reader.cpp index 1c5d00e7d76e1c..0eed63d818aa97 100644 --- a/be/src/format_v2/parquet/parquet_reader.cpp +++ b/be/src/format_v2/parquet/parquet_reader.cpp @@ -166,8 +166,10 @@ Status validate_requested_columns_supported( for (const auto& column : request.predicate_columns) { RETURN_IF_ERROR(validate_scan_column(column)); } - for (const auto& column : request.non_predicate_columns) { - RETURN_IF_ERROR(validate_scan_column(column)); + if (!request.non_predicate_columns_are_count_star_placeholders) { + for (const auto& column : request.non_predicate_columns) { + RETURN_IF_ERROR(validate_scan_column(column)); + } } return Status::OK(); } diff --git a/be/src/format_v2/table_reader.h b/be/src/format_v2/table_reader.h index 49725bd7ccd601..035d073c4555c3 100644 --- a/be/src/format_v2/table_reader.h +++ b/be/src/format_v2/table_reader.h @@ -453,6 +453,16 @@ class TableReader { VLOG_DEBUG << "TableReader debug: " << debug_string(); } RETURN_IF_ERROR(_open_mapping_exprs()); + // COUNT(*) has no semantic column argument, but Nereids retains one minimum-width scan + // slot so the scan node still has an output tuple. For example, in a Parquet file whose + // first and only column is unsupported TIME_MILLIS, validating that arbitrary placeholder + // would fail before the empty aggregate request can count rows from footer metadata. Mark + // placeholders only after the same safety gate used by aggregate materialization succeeds; + // COUNT(col), filters, deletes and pending runtime filters keep normal column validation. + file_request->non_predicate_columns_are_count_star_placeholders = + _push_down_agg_type == TPushAggOp::type::COUNT && + _push_down_count_columns.has_value() && _push_down_count_columns->empty() && + _supports_aggregate_pushdown(_push_down_agg_type); RETURN_IF_ERROR(_data_reader.reader->open(file_request)); RETURN_IF_ERROR(_init_reader_condition_cache(*file_request)); return Status::OK(); diff --git a/be/test/format_v2/parquet/parquet_scan_test.cpp b/be/test/format_v2/parquet/parquet_scan_test.cpp index 6b1ae8918cad4d..a82f6d43f4bbcd 100644 --- a/be/test/format_v2/parquet/parquet_scan_test.cpp +++ b/be/test/format_v2/parquet/parquet_scan_test.cpp @@ -806,6 +806,38 @@ TEST_F(ParquetScanTest, AggregateCountRejectsRequiredUnsupportedScalarProjection std::string::npos); } +TEST_F(ParquetScanTest, CountStarIgnoresUnsupportedPlannerPlaceholder) { + write_required_adjusted_time_parquet_file(_file_path); + RuntimeState state {TQueryOptions(), TQueryGlobals()}; + + // A normal projection still requests the TIME_MILLIS value and must fail at open, before + // row-group pruning or physical INT32 statistics can hide the unsupported logical type. + auto projected_reader = create_reader(); + ASSERT_TRUE(projected_reader->init(&state).ok()); + auto projected_request = std::make_shared(); + format::FileScanRequestBuilder projected_builder(projected_request.get()); + ASSERT_TRUE(projected_builder.add_non_predicate_column(format::LocalColumnId(0)).ok()); + const auto projected_status = projected_reader->open(projected_request); + EXPECT_TRUE(projected_status.is()) << projected_status; + + // COUNT(*) carries the same retained scan slot, but TableReader marks it as a planner + // placeholder after proving metadata aggregate pushdown is safe. The empty aggregate request + // counts both rows without interpreting or materializing the unsupported TIME_MILLIS values. + auto count_star_reader = create_reader(); + ASSERT_TRUE(count_star_reader->init(&state).ok()); + auto count_star_request = std::make_shared(); + format::FileScanRequestBuilder count_star_builder(count_star_request.get()); + ASSERT_TRUE(count_star_builder.add_non_predicate_column(format::LocalColumnId(0)).ok()); + count_star_request->non_predicate_columns_are_count_star_placeholders = true; + ASSERT_TRUE(count_star_reader->open(count_star_request).ok()); + + format::FileAggregateRequest aggregate_request; + aggregate_request.agg_type = TPushAggOp::COUNT; + format::FileAggregateResult aggregate_result; + ASSERT_TRUE(count_star_reader->get_aggregate_result(aggregate_request, &aggregate_result).ok()); + EXPECT_EQ(aggregate_result.count, 2); +} + TEST_F(ParquetScanTest, AggregateMinMaxRejectsInexactBinaryStatistics) { write_binary_minmax_parquet_file(_file_path); auto reader = create_reader(); diff --git a/be/test/format_v2/table_reader_test.cpp b/be/test/format_v2/table_reader_test.cpp index 3cf14c3362bc61..38679e40d93a52 100644 --- a/be/test/format_v2/table_reader_test.cpp +++ b/be/test/format_v2/table_reader_test.cpp @@ -1761,6 +1761,8 @@ TEST(TableReaderTest, PushDownCountRecordsReaderRowsBeforeClosingReader) { EXPECT_EQ(block.rows(), 3); EXPECT_EQ(file_reader_stats.read_rows, 3); EXPECT_EQ(fake_state->close_count, 1); + ASSERT_TRUE(fake_state->last_request != nullptr); + EXPECT_FALSE(fake_state->last_request->non_predicate_columns_are_count_star_placeholders); ASSERT_TRUE(fake_state->last_aggregate_request.has_value()); ASSERT_EQ(fake_state->last_aggregate_request->columns.size(), 1); // A primitive COUNT(col) projection must reach the file reader just like a complex one. @@ -1805,6 +1807,8 @@ TEST(TableReaderTest, PushDownCountStarIgnoresProjectedPlaceholderColumn) { ASSERT_TRUE(reader.get_block(&block, &eos).ok()); EXPECT_FALSE(eos); EXPECT_EQ(block.rows(), 3); + ASSERT_TRUE(fake_state->last_request != nullptr); + EXPECT_TRUE(fake_state->last_request->non_predicate_columns_are_count_star_placeholders); ASSERT_TRUE(fake_state->last_aggregate_request.has_value()); // Passing nullable_id here would implement COUNT(nullable_id) and reproduce the external ORC // and Parquet failures where footer row counts were reduced by null values. From bae00cdddf12f30eb0bddde9227b0f17bdc7d13e Mon Sep 17 00:00:00 2001 From: Gabriel Date: Wed, 15 Jul 2026 22:54:51 +0800 Subject: [PATCH 13/17] [fix](be) Preserve COUNT star placeholders on fallback ### What problem does this PR solve? Issue Number: None Related PR: #65548 Problem Summary: Explicit COUNT(*) keeps a planner-only scan slot after column pruning. The previous implementation marked that slot only when metadata aggregate pushdown was eligible. Position deletes, deletion vectors, equality deletes, predicates, or pending runtime filters disable aggregate pushdown and fall back to row scanning, causing Parquet to validate and decode the arbitrary placeholder again. An unsupported logical leaf such as adjusted TIME_MILLIS could therefore reject COUNT(*) even though only the surviving row count was needed. Record exact COUNT(*) placeholder column ids independently of aggregate eligibility, keep real predicate and delete columns strict, and synthesize default values for surviving placeholder rows without physical column IO. ### Release note Fix COUNT(*) fallback scans over Parquet files whose planner-only placeholder uses an unsupported logical type. ### Check List (For Author) - Test: Unit Test - 4 targeted BE ASAN unit tests passed - Behavior changed: Yes. COUNT(*) fallback scans no longer validate or read planner-only non-predicate placeholders; predicates, delete keys, COUNT(column), and normal projections remain unchanged. - Does this need documentation: No --- be/src/format_v2/file_reader.cpp | 9 ++- be/src/format_v2/file_reader.h | 18 ++++-- be/src/format_v2/parquet/parquet_reader.cpp | 8 ++- be/src/format_v2/parquet/parquet_scan.cpp | 45 ++++++++++++-- be/src/format_v2/table_reader.h | 24 +++++--- .../format_v2/parquet/parquet_scan_test.cpp | 61 ++++++++++++++++--- be/test/format_v2/table_reader_test.cpp | 9 ++- 7 files changed, 138 insertions(+), 36 deletions(-) diff --git a/be/src/format_v2/file_reader.cpp b/be/src/format_v2/file_reader.cpp index 910566d46e3fa6..b2603a17958eae 100644 --- a/be/src/format_v2/file_reader.cpp +++ b/be/src/format_v2/file_reader.cpp @@ -63,8 +63,13 @@ std::string FileScanRequest::debug_string() const { } out << "}, conjunct_count=" << conjuncts.size() << ", delete_conjunct_count=" << delete_conjuncts.size() - << ", non_predicate_columns_are_count_star_placeholders=" - << non_predicate_columns_are_count_star_placeholders << "}"; + << ", count_star_placeholder_columns={"; + const char* delimiter = ""; + for (const auto column_id : count_star_placeholder_columns) { + out << delimiter << column_id.value(); + delimiter = ","; + } + out << "}}"; return out.str(); } diff --git a/be/src/format_v2/file_reader.h b/be/src/format_v2/file_reader.h index 4c31dddf76d764..96d940702486b4 100644 --- a/be/src/format_v2/file_reader.h +++ b/be/src/format_v2/file_reader.h @@ -77,12 +77,18 @@ struct FileScanRequest { // Delete predicates converted to file-local expressions. A TRUE result means that the row is // deleted, so readers must invert each result when building their keep filter. VExprContextSPtrs delete_conjuncts; - // True only when TableReader has proved that an explicit COUNT(*) can be answered by the - // physical reader's aggregate metadata path. Nereids still retains one scan slot after column - // pruning, but every non-predicate column in this request is then a row-producing placeholder, - // not a user-requested value. Readers may skip semantic validation of those placeholders before - // aggregate materialization; predicate columns must always be validated normally. - bool non_predicate_columns_are_count_star_placeholders = false; + // File-local ids retained only because Nereids keeps a minimum-width output tuple for an + // explicit COUNT(*). These columns have no semantic value: for example, after pruning a scan + // may retain an unsupported TIME_MILLIS leaf even though COUNT(*) only needs one row per + // surviving input row. A reader may synthesize defaults instead of reading a marked column + // while it remains non-predicate. If filters or equality deletes promote the same id to + // predicate_columns, the value is semantically required and must still be validated and read. + std::vector count_star_placeholder_columns; + + bool is_count_star_placeholder(LocalColumnId column_id) const { + return std::ranges::find(count_star_placeholder_columns, column_id) != + count_star_placeholder_columns.end(); + } }; // Helper for constructing the scan-column layout in FileScanRequest. diff --git a/be/src/format_v2/parquet/parquet_reader.cpp b/be/src/format_v2/parquet/parquet_reader.cpp index 0eed63d818aa97..cd84ab74ef953e 100644 --- a/be/src/format_v2/parquet/parquet_reader.cpp +++ b/be/src/format_v2/parquet/parquet_reader.cpp @@ -114,7 +114,9 @@ void collect_request_leaf_column_ids( collect_scan_column(column); } for (const auto& column : request.non_predicate_columns) { - collect_scan_column(column); + if (!request.is_count_star_placeholder(column.column_id())) { + collect_scan_column(column); + } } } @@ -166,8 +168,8 @@ Status validate_requested_columns_supported( for (const auto& column : request.predicate_columns) { RETURN_IF_ERROR(validate_scan_column(column)); } - if (!request.non_predicate_columns_are_count_star_placeholders) { - for (const auto& column : request.non_predicate_columns) { + for (const auto& column : request.non_predicate_columns) { + if (!request.is_count_star_placeholder(column.column_id())) { RETURN_IF_ERROR(validate_scan_column(column)); } } diff --git a/be/src/format_v2/parquet/parquet_scan.cpp b/be/src/format_v2/parquet/parquet_scan.cpp index 1abddc777aa442..eafb741a53161d 100644 --- a/be/src/format_v2/parquet/parquet_scan.cpp +++ b/be/src/format_v2/parquet/parquet_scan.cpp @@ -177,11 +177,41 @@ std::vector request_scan_columns(const format::FileSca scan_columns.reserve(request.predicate_columns.size() + request.non_predicate_columns.size()); scan_columns.insert(scan_columns.end(), request.predicate_columns.begin(), request.predicate_columns.end()); - scan_columns.insert(scan_columns.end(), request.non_predicate_columns.begin(), - request.non_predicate_columns.end()); + for (const auto& column : request.non_predicate_columns) { + if (!request.is_count_star_placeholder(column.column_id())) { + scan_columns.push_back(column); + } + } return scan_columns; } +std::vector physical_non_predicate_columns( + const format::FileScanRequest& request) { + std::vector columns; + columns.reserve(request.non_predicate_columns.size()); + for (const auto& column : request.non_predicate_columns) { + if (!request.is_count_star_placeholder(column.column_id())) { + columns.push_back(column); + } + } + return columns; +} + +void materialize_count_star_placeholders(const format::FileScanRequest& request, size_t rows, + Block* file_block) { + DORIS_CHECK(file_block != nullptr); + for (const auto& column : request.non_predicate_columns) { + if (!request.is_count_star_placeholder(column.column_id())) { + continue; + } + const auto block_position = request.local_positions.at(column.column_id()).value(); + auto placeholder = file_block->get_by_position(block_position).column->assert_mutable(); + DCHECK(placeholder->empty()); + placeholder->insert_many_defaults(rows); + file_block->replace_by_position(block_position, std::move(placeholder)); + } +} + std::vector build_row_group_prefetch_ranges( const ::parquet::FileMetaData& metadata, const std::vector>& file_schema, @@ -750,6 +780,9 @@ Status ParquetScanScheduler::open_next_row_group( } for (const auto& col : request.non_predicate_columns) { const auto local_id = col.local_id(); + if (request.is_count_star_placeholder(col.column_id())) { + continue; + } if (local_id == format::ROW_POSITION_COLUMN_ID) { _current_non_predicate_columns[local_id] = column_reader_factory.create_row_position_column_reader( @@ -775,7 +808,8 @@ Status ParquetScanScheduler::open_next_row_group( // With no row-level filters there is no lazy-read decision to wait for, so start warming // output chunks immediately after their readers are created. Filtered scans still defer // this until at least one row survives the predicate phase. - prefetch_current_row_group_columns(file_context, file_schema, request.non_predicate_columns, + prefetch_current_row_group_columns(file_context, file_schema, + physical_non_predicate_columns(request), &_current_non_predicate_prefetched); } *has_row_group = true; @@ -1374,6 +1408,7 @@ Status ParquetScanScheduler::read_current_row_group_batch( _raw_rows_read += batch_rows; if (_current_predicate_columns.empty() && _current_non_predicate_columns.empty()) { *rows = static_cast(batch_rows); + materialize_count_star_placeholders(request, *rows, file_block); if (_scan_profile.selected_rows != nullptr) { COUNTER_UPDATE(_scan_profile.selected_rows, batch_rows); } @@ -1431,7 +1466,8 @@ Status ParquetScanScheduler::read_current_row_group_batch( // Do not prefetch lazy output columns until at least one row survives filtering. This is // the same decision point where the v2 reader switches from predicate-only reads to // materializing non-predicate columns, so fully filtered batches avoid unnecessary IO. - prefetch_current_row_group_columns(file_context, file_schema, request.non_predicate_columns, + prefetch_current_row_group_columns(file_context, file_schema, + physical_non_predicate_columns(request), &_current_non_predicate_prefetched); } @@ -1472,6 +1508,7 @@ Status ParquetScanScheduler::read_current_row_group_batch( file_block->replace_by_position(block_position, std::move(column)); } } + materialize_count_star_placeholders(request, selected_rows, file_block); *rows = static_cast(selected_rows); return Status::OK(); } diff --git a/be/src/format_v2/table_reader.h b/be/src/format_v2/table_reader.h index 035d073c4555c3..e3c3ca9a07dc90 100644 --- a/be/src/format_v2/table_reader.h +++ b/be/src/format_v2/table_reader.h @@ -402,6 +402,20 @@ class TableReader { RETURN_IF_ERROR(close_current_reader()); return Status::OK(); } + // COUNT(*) has no semantic column argument, but Nereids retains a minimum-width scan slot + // so the scan node still has an output tuple. Record only the current non-predicate file + // columns before table-format hooks add row-position or equality-delete dependencies. This + // marker is independent of aggregate eligibility: with position deletes, for example, + // metadata COUNT must fall back to reading rows, but an arbitrary unsupported TIME_MILLIS + // placeholder still must not be validated or decoded merely to carry the surviving count. + if (_push_down_agg_type == TPushAggOp::type::COUNT && + _push_down_count_columns.has_value() && _push_down_count_columns->empty()) { + file_request->count_star_placeholder_columns.reserve( + file_request->non_predicate_columns.size()); + for (const auto& column : file_request->non_predicate_columns) { + file_request->count_star_placeholder_columns.push_back(column.column_id()); + } + } RETURN_IF_ERROR(customize_file_scan_request(file_request.get())); RETURN_IF_ERROR(_open_local_filter_exprs(*file_request)); _data_reader.file_block_layout.clear(); @@ -453,16 +467,6 @@ class TableReader { VLOG_DEBUG << "TableReader debug: " << debug_string(); } RETURN_IF_ERROR(_open_mapping_exprs()); - // COUNT(*) has no semantic column argument, but Nereids retains one minimum-width scan - // slot so the scan node still has an output tuple. For example, in a Parquet file whose - // first and only column is unsupported TIME_MILLIS, validating that arbitrary placeholder - // would fail before the empty aggregate request can count rows from footer metadata. Mark - // placeholders only after the same safety gate used by aggregate materialization succeeds; - // COUNT(col), filters, deletes and pending runtime filters keep normal column validation. - file_request->non_predicate_columns_are_count_star_placeholders = - _push_down_agg_type == TPushAggOp::type::COUNT && - _push_down_count_columns.has_value() && _push_down_count_columns->empty() && - _supports_aggregate_pushdown(_push_down_agg_type); RETURN_IF_ERROR(_data_reader.reader->open(file_request)); RETURN_IF_ERROR(_init_reader_condition_cache(*file_request)); return Status::OK(); diff --git a/be/test/format_v2/parquet/parquet_scan_test.cpp b/be/test/format_v2/parquet/parquet_scan_test.cpp index a82f6d43f4bbcd..36cc99ebf106c0 100644 --- a/be/test/format_v2/parquet/parquet_scan_test.cpp +++ b/be/test/format_v2/parquet/parquet_scan_test.cpp @@ -46,6 +46,8 @@ #include "core/field.h" #include "exprs/vexpr.h" #include "exprs/vexpr_context.h" +#include "exprs/vslot_ref.h" +#include "format_v2/expr/delete_predicate.h" #include "format_v2/file_reader.h" #include "format_v2/parquet/parquet_column_schema.h" #include "format_v2/parquet/parquet_reader.h" @@ -820,22 +822,63 @@ TEST_F(ParquetScanTest, CountStarIgnoresUnsupportedPlannerPlaceholder) { const auto projected_status = projected_reader->open(projected_request); EXPECT_TRUE(projected_status.is()) << projected_status; - // COUNT(*) carries the same retained scan slot, but TableReader marks it as a planner - // placeholder after proving metadata aggregate pushdown is safe. The empty aggregate request - // counts both rows without interpreting or materializing the unsupported TIME_MILLIS values. + // Metadata COUNT(*) carries the same retained scan slot, but its aggregate request has no + // semantic column. The placeholder must not make open fail before footer row counts are used. + auto metadata_count_reader = create_reader(); + ASSERT_TRUE(metadata_count_reader->init(&state).ok()); + auto metadata_count_request = std::make_shared(); + format::FileScanRequestBuilder metadata_count_builder(metadata_count_request.get()); + ASSERT_TRUE(metadata_count_builder.add_non_predicate_column(format::LocalColumnId(0)).ok()); + metadata_count_request->count_star_placeholder_columns.push_back(format::LocalColumnId(0)); + ASSERT_TRUE(metadata_count_reader->open(metadata_count_request).ok()); + + format::FileAggregateRequest aggregate_request; + aggregate_request.agg_type = TPushAggOp::COUNT; + format::FileAggregateResult aggregate_result; + ASSERT_TRUE( + metadata_count_reader->get_aggregate_result(aggregate_request, &aggregate_result).ok()); + EXPECT_EQ(aggregate_result.count, 2); + + // The marker is independent of aggregate eligibility. Simulate a position delete, + // which disables metadata COUNT and requires the reader to produce surviving rows. Parquet + // reads only the virtual row position, filters row 1, and fills a default placeholder for row 0 + // without validating or decoding either unsupported TIME_MILLIS value. auto count_star_reader = create_reader(); ASSERT_TRUE(count_star_reader->init(&state).ok()); auto count_star_request = std::make_shared(); format::FileScanRequestBuilder count_star_builder(count_star_request.get()); ASSERT_TRUE(count_star_builder.add_non_predicate_column(format::LocalColumnId(0)).ok()); - count_star_request->non_predicate_columns_are_count_star_placeholders = true; + ASSERT_TRUE(count_star_builder + .add_predicate_column(format::LocalColumnId(format::ROW_POSITION_COLUMN_ID)) + .ok()); + count_star_request->count_star_placeholder_columns.push_back(format::LocalColumnId(0)); + + const std::vector deleted_rows {1}; + auto delete_predicate = std::make_shared(deleted_rows); + const auto row_position = count_star_request->local_positions.at( + format::LocalColumnId(format::ROW_POSITION_COLUMN_ID)); + delete_predicate->add_child(VSlotRef::create_shared( + cast_set(row_position.value()), cast_set(row_position.value()), -1, + std::make_shared(), format::ROW_POSITION_COLUMN_NAME)); + auto delete_context = VExprContext::create_shared(std::move(delete_predicate)); + ASSERT_TRUE(delete_context->prepare(&state, RowDescriptor()).ok()); + ASSERT_TRUE(delete_context->open(&state).ok()); + count_star_request->delete_conjuncts.push_back(std::move(delete_context)); ASSERT_TRUE(count_star_reader->open(count_star_request).ok()); - format::FileAggregateRequest aggregate_request; - aggregate_request.agg_type = TPushAggOp::COUNT; - format::FileAggregateResult aggregate_result; - ASSERT_TRUE(count_star_reader->get_aggregate_result(aggregate_request, &aggregate_result).ok()); - EXPECT_EQ(aggregate_result.count, 2); + std::vector file_schema; + ASSERT_TRUE(count_star_reader->get_schema(&file_schema).ok()); + file_schema.push_back(format::row_position_column_definition()); + auto block = build_file_block(file_schema); + size_t rows = 0; + bool eof = false; + ASSERT_TRUE(count_star_reader->get_block(&block, &rows, &eof).ok()); + EXPECT_EQ(rows, 1); + EXPECT_EQ(block.get_by_position(0).column->size(), 1); + const auto& positions = + assert_cast(*block.get_by_position(row_position.value()).column); + ASSERT_EQ(positions.size(), 1); + EXPECT_EQ(positions.get_element(0), 0); } TEST_F(ParquetScanTest, AggregateMinMaxRejectsInexactBinaryStatistics) { diff --git a/be/test/format_v2/table_reader_test.cpp b/be/test/format_v2/table_reader_test.cpp index 38679e40d93a52..7c50a11dd5dc88 100644 --- a/be/test/format_v2/table_reader_test.cpp +++ b/be/test/format_v2/table_reader_test.cpp @@ -1573,6 +1573,10 @@ TEST(TableReaderTest, PendingRuntimeFilterDisablesTableLevelCount) { ASSERT_TRUE(reader.get_block(&block, &eos).ok()); EXPECT_EQ(fake_state->open_count, 1); EXPECT_EQ(block.rows(), 2); + ASSERT_NE(fake_state->last_request, nullptr); + // Aggregate pushdown is disabled while a runtime filter is pending, but COUNT(*) semantics do + // not change. The retained output slot remains a value-less placeholder during row fallback. + EXPECT_TRUE(fake_state->last_request->is_count_star_placeholder(LocalColumnId(0))); ASSERT_TRUE(reader.close().ok()); } @@ -1762,7 +1766,7 @@ TEST(TableReaderTest, PushDownCountRecordsReaderRowsBeforeClosingReader) { EXPECT_EQ(file_reader_stats.read_rows, 3); EXPECT_EQ(fake_state->close_count, 1); ASSERT_TRUE(fake_state->last_request != nullptr); - EXPECT_FALSE(fake_state->last_request->non_predicate_columns_are_count_star_placeholders); + EXPECT_TRUE(fake_state->last_request->count_star_placeholder_columns.empty()); ASSERT_TRUE(fake_state->last_aggregate_request.has_value()); ASSERT_EQ(fake_state->last_aggregate_request->columns.size(), 1); // A primitive COUNT(col) projection must reach the file reader just like a complex one. @@ -1808,7 +1812,8 @@ TEST(TableReaderTest, PushDownCountStarIgnoresProjectedPlaceholderColumn) { EXPECT_FALSE(eos); EXPECT_EQ(block.rows(), 3); ASSERT_TRUE(fake_state->last_request != nullptr); - EXPECT_TRUE(fake_state->last_request->non_predicate_columns_are_count_star_placeholders); + ASSERT_EQ(fake_state->last_request->count_star_placeholder_columns.size(), 1); + EXPECT_TRUE(fake_state->last_request->is_count_star_placeholder(LocalColumnId(0))); ASSERT_TRUE(fake_state->last_aggregate_request.has_value()); // Passing nullable_id here would implement COUNT(nullable_id) and reproduce the external ORC // and Parquet failures where footer row counts were reduced by null values. From 7d508ddaf7532ea96643981d44887c26927b111d Mon Sep 17 00:00:00 2001 From: Gabriel Date: Thu, 16 Jul 2026 13:15:03 +0800 Subject: [PATCH 14/17] [fix](be) Address final FileScannerV2 review comments ### What problem does this PR solve?\n\nFile-local struct predicates could hide lossy materialization errors, and Hive/TVF treated COUNT(column) like metadata-only COUNT(*).\n\nIssue Number: None\n\n### Release note\n\nNone\n\n### Check List\n\n- [x] Regression tests\n- [x] Code format --- be/src/format_v2/column_mapper.cpp | 25 +++++++-- be/test/format_v2/column_mapper_test.cpp | 29 ++++++++++ .../datasource/hive/source/HiveScanNode.java | 5 +- .../datasource/tvf/source/TVFScanNode.java | 5 +- .../tvf/source/TVFScanNodeTest.java | 55 +++++++++++++++++++ 5 files changed, 109 insertions(+), 10 deletions(-) diff --git a/be/src/format_v2/column_mapper.cpp b/be/src/format_v2/column_mapper.cpp index 5008e0bdb20669..6713183c09d816 100644 --- a/be/src/format_v2/column_mapper.cpp +++ b/be/src/format_v2/column_mapper.cpp @@ -897,7 +897,8 @@ static VExprSPtr cast_file_expr_to_table_type(const VExprSPtr& file_expr, static bool rewrite_binary_struct_literal_predicate( const VExprSPtr& expr, const std::vector& filter_mappings, const std::map& global_to_file_slot, - RewriteContext* rewrite_context) { + RewriteContext* rewrite_context, bool* can_localize) { + DORIS_CHECK(can_localize != nullptr); if (!is_binary_comparison_predicate(expr)) { return false; } @@ -936,6 +937,13 @@ static bool rewrite_binary_struct_literal_predicate( if (file_literal != nullptr) { children[literal_child_idx] = std::move(file_literal); } else { + if (!is_lossless_file_to_table_numeric_cast(file_leaf_type, table_leaf_type)) { + // A narrowing or otherwise lossy cast can fail or produce NULL while TableReader + // materializes the table schema. Evaluating it here could filter the offending row + // before that validation, so keep the complete predicate above TableReader. + *can_localize = false; + return true; + } children[struct_child_idx] = cast_file_expr_to_table_type(children[struct_child_idx], table_leaf_type, rewrite_context); children[literal_child_idx] = original_table_literal(table_literal, rewrite_context); @@ -951,7 +959,8 @@ static bool rewrite_binary_struct_literal_predicate( static bool rewrite_in_struct_literal_predicate( const VExprSPtr& expr, const std::vector& filter_mappings, const std::map& global_to_file_slot, - RewriteContext* rewrite_context) { + RewriteContext* rewrite_context, bool* can_localize) { + DORIS_CHECK(can_localize != nullptr); if (expr->node_type() != TExprNodeType::IN_PRED || expr->get_num_children() < 2 || !is_struct_element_expr(expr->children()[0])) { return false; @@ -987,6 +996,10 @@ static bool rewrite_in_struct_literal_predicate( auto file_literal = rewrite_literal_to_file_type(table_literal, leaf_rewrite_info, rewrite_context); if (file_literal == nullptr) { + if (!is_lossless_file_to_table_numeric_cast(file_leaf_type, table_leaf_type)) { + *can_localize = false; + return true; + } children[0] = cast_file_expr_to_table_type(children[0], table_leaf_type, rewrite_context); for (size_t literal_idx = 0; literal_idx < table_literals.size(); ++literal_idx) { @@ -1027,6 +1040,10 @@ static VExprSPtr rewrite_struct_or_slot_expr_to_file_expr( DORIS_CHECK(table_leaf_type != nullptr); DORIS_CHECK(expr->data_type() != nullptr); if (!expr->data_type()->equals(*table_leaf_type)) { + if (!is_lossless_file_to_table_numeric_cast(expr->data_type(), table_leaf_type)) { + *can_localize = false; + return expr; + } // Path localization changes the leaf to the physical file type. For example, after an // Iceberg evolution from STRUCT to STRUCT, the localized old-file // predicate is initially `element_at(file_col, 'a')::INT = 10::BIGINT`. Cast only the @@ -1091,11 +1108,11 @@ static VExprSPtr rewrite_table_expr_to_file_expr( return expr; } if (rewrite_binary_struct_literal_predicate(expr, filter_mappings, global_to_file_slot, - rewrite_context)) { + rewrite_context, can_localize)) { return expr; } if (rewrite_in_struct_literal_predicate(expr, filter_mappings, global_to_file_slot, - rewrite_context)) { + rewrite_context, can_localize)) { return expr; } if (is_struct_element_expr(expr) || expr->is_slot_ref()) { diff --git a/be/test/format_v2/column_mapper_test.cpp b/be/test/format_v2/column_mapper_test.cpp index 82189ad8acb7cf..16bdaad982d515 100644 --- a/be/test/format_v2/column_mapper_test.cpp +++ b/be/test/format_v2/column_mapper_test.cpp @@ -3151,6 +3151,35 @@ TEST_F(ColumnMapperCastTest, EXPECT_TRUE(request.conjuncts.empty()); } +// Scenario: a narrowing file-to-table cast can produce NULL or an error for values that do not fit +// the table leaf. Evaluating that cast below TableReader can filter those rows before +// _align_column_nullability() validates the required table child. Keep the predicate at table level +// so schema materialization observes every source row first. +TEST_F(ColumnMapperCastTest, NestedElementAtConjunctStaysTableLevelForNonLosslessFileToTableCast) { + const auto file_bigint_type = i64(); + const auto table_int_type = i32(); + + auto table_a = field_id_col("a", 11, table_int_type); + auto table_struct = struct_col("s", 10, {table_a}); + auto file_a = field_id_col("a", 11, file_bigint_type, 0); + auto file_struct = struct_col("s", 10, {file_a}, 5); + + auto table_leaf = executable_struct_element( + table_slot(0, 0, table_struct.type, table_struct.name), table_int_type, "a"); + auto filter_expr = executable_binary_predicate( + TExprOpcode::EQ, table_leaf, literal(table_int_type, Field::create_field(1))); + TableFilter filter {.conjunct = VExprContext::create_shared(filter_expr), + .global_indices = {GlobalIndex(0)}}; + + TableColumnMapper mapper({.mode = TableColumnMappingMode::BY_FIELD_ID}); + ASSERT_TRUE(mapper.create_mapping({table_struct}, {}, {file_struct}).ok()); + + FileScanRequest request; + ASSERT_TRUE(mapper.create_scan_request({filter}, {table_struct}, &request, &state).ok()); + ASSERT_EQ(request.predicate_columns.size(), 1); + EXPECT_TRUE(request.conjuncts.empty()); +} + // Scenario: the table literal is outside the old file leaf's INT range. Rewriting // BIGINT 2147483648 to INT would change the predicate, so keep the literal as BIGINT and cast the // file leaf instead: `CAST(file_s.b::INT AS BIGINT) = 2147483648::BIGINT`. diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/hive/source/HiveScanNode.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/hive/source/HiveScanNode.java index 3ea39987d021f7..cae6728c5f467f 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/hive/source/HiveScanNode.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/hive/source/HiveScanNode.java @@ -60,7 +60,6 @@ import org.apache.doris.thrift.TFileRangeDesc; import org.apache.doris.thrift.TFileScanRangeParams; import org.apache.doris.thrift.TFileTextScanRangeParams; -import org.apache.doris.thrift.TPushAggOp; import org.apache.doris.thrift.TTableFormatFileDesc; import org.apache.doris.thrift.TTransactionalHiveDeleteDeltaDesc; import org.apache.doris.thrift.TTransactionalHiveDesc; @@ -339,7 +338,7 @@ private void getFileSplitByPartitions(HiveExternalMetaCache cache, List getSplits(int numBackends) throws UserException { List fileStatuses = tableValuedFunction.getFileStatuses(); - // Push down count optimization. + // Avoid splitting only for table-level COUNT(*). COUNT(column) still reads column data. boolean needSplit = true; - if (getPushDownAggNoGroupingOp() == TPushAggOp.COUNT) { + if (isTableLevelCountStarPushdown()) { int parallelNum = sessionVariable.getParallelExecInstanceNum(scanContext.getClusterName()); int totalFileNum = fileStatuses.size(); needSplit = FileSplitter.needSplitForCountPushdown(parallelNum, numBackends, totalFileNum); diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/tvf/source/TVFScanNodeTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/tvf/source/TVFScanNodeTest.java index 8cf98daea94c59..8ea9041480dadd 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/tvf/source/TVFScanNodeTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/tvf/source/TVFScanNodeTest.java @@ -17,14 +17,20 @@ package org.apache.doris.datasource.tvf.source; +import org.apache.doris.analysis.SlotId; import org.apache.doris.analysis.TupleDescriptor; import org.apache.doris.analysis.TupleId; import org.apache.doris.catalog.FunctionGenTable; +import org.apache.doris.datasource.FileQueryScanNode; +import org.apache.doris.datasource.FileSplitter; import org.apache.doris.planner.PlanNodeId; import org.apache.doris.planner.ScanContext; import org.apache.doris.qe.SessionVariable; +import org.apache.doris.spi.Split; import org.apache.doris.tablefunction.ExternalFileTableValuedFunction; import org.apache.doris.thrift.TBrokerFileStatus; +import org.apache.doris.thrift.TFileType; +import org.apache.doris.thrift.TPushAggOp; import org.junit.Assert; import org.junit.Test; @@ -37,6 +43,40 @@ public class TVFScanNodeTest { private static final long MB = 1024L * 1024L; + @Test + public void testCountColumnKeepsNormalFileSplitting() throws Exception { + SessionVariable sv = new SessionVariable(); + sv.parallelExecInstanceNum = 1; + sv.setFileSplitSize(32 * MB); + TupleDescriptor desc = new TupleDescriptor(new TupleId(0)); + FunctionGenTable table = Mockito.mock(FunctionGenTable.class); + ExternalFileTableValuedFunction tvf = Mockito.mock(ExternalFileTableValuedFunction.class); + Mockito.when(table.getTvf()).thenReturn(tvf); + Mockito.when(tvf.getTFileType()).thenReturn(TFileType.FILE_LOCAL); + Mockito.when(tvf.getFileStatuses()).thenReturn(List.of( + splittableFile("file:///tmp/count_col_1.parquet", 128 * MB), + splittableFile("file:///tmp/count_col_2.parquet", 128 * MB))); + desc.setTable(table); + + TVFScanNode countColumnNode = new TVFScanNode( + new PlanNodeId(0), desc, false, sv, ScanContext.EMPTY); + setFileSplitter(countColumnNode, new FileSplitter(32 * MB, 32 * MB, 0)); + countColumnNode.setPushDownAggNoGrouping(TPushAggOp.COUNT); + countColumnNode.setPushDownCountSlotIds(Collections.singletonList(new SlotId(7))); + + List countColumnSplits = countColumnNode.getSplits(1); + Assert.assertEquals(8, countColumnSplits.size()); + + TVFScanNode countStarNode = new TVFScanNode( + new PlanNodeId(1), desc, false, sv, ScanContext.EMPTY); + setFileSplitter(countStarNode, new FileSplitter(32 * MB, 32 * MB, 0)); + countStarNode.setPushDownAggNoGrouping(TPushAggOp.COUNT); + countStarNode.setPushDownCountSlotIds(Collections.emptyList()); + + List countStarSplits = countStarNode.getSplits(1); + Assert.assertEquals(2, countStarSplits.size()); + } + @Test public void testDetermineTargetFileSplitSizeHonorsMaxFileSplitNum() throws Exception { SessionVariable sv = new SessionVariable(); @@ -57,4 +97,19 @@ public void testDetermineTargetFileSplitSizeHonorsMaxFileSplitNum() throws Excep long target = (long) method.invoke(node, statuses); Assert.assertEquals(100 * MB, target); } + + private static TBrokerFileStatus splittableFile(String path, long size) { + TBrokerFileStatus status = new TBrokerFileStatus(); + status.setPath(path); + status.setSize(size); + status.setModificationTime(0); + status.setIsSplitable(true); + return status; + } + + private static void setFileSplitter(TVFScanNode node, FileSplitter splitter) throws Exception { + java.lang.reflect.Field field = FileQueryScanNode.class.getDeclaredField("fileSplitter"); + field.setAccessible(true); + field.set(node, splitter); + } } From 6ea36107141a4fb0e4ec51126018b21d253d1d90 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Thu, 16 Jul 2026 15:04:20 +0800 Subject: [PATCH 15/17] [fix](be) Keep adaptive batching for COUNT fallbacks ### What problem does this PR solve? Issue Number: None Related PR: #65548 Problem Summary: FileScannerV2 disabled adaptive batch sizing whenever the FE sent COUNT, even when TableReader rejected metadata COUNT because of multiple arguments, unsafe mappings, filters, or deletes. Those fallback scans materialized normal rows using the fixed runtime batch size, inflating block memory for wide columns. Track whether the active split actually emits metadata-derived COUNT rows and suppress adaptive sizing only in that case; fallback scans retain the first-batch probe and predictor updates. ### Release note None ### Check List (For Author) - Test: Unit Test - FileScannerV2Test.AdaptiveBatchSizeRunsForCountFallbackOnly - TableReaderTest COUNT coverage - Behavior changed: No. Query results are unchanged; normal COUNT fallback scans retain adaptive batch sizing. - Does this need documentation: No --- be/src/exec/scan/file_scanner_v2.cpp | 24 +++++++---- be/src/exec/scan/file_scanner_v2.h | 7 ++++ be/src/format_v2/table_reader.cpp | 2 + be/src/format_v2/table_reader.h | 10 +++++ be/test/exec/scan/file_scanner_v2_test.cpp | 6 +++ be/test/format_v2/table_reader_test.cpp | 46 ++++++++++++++++++++++ 6 files changed, 88 insertions(+), 7 deletions(-) diff --git a/be/src/exec/scan/file_scanner_v2.cpp b/be/src/exec/scan/file_scanner_v2.cpp index 91da5a48c510bf..f06e9fb9533774 100644 --- a/be/src/exec/scan/file_scanner_v2.cpp +++ b/be/src/exec/scan/file_scanner_v2.cpp @@ -443,9 +443,12 @@ Status FileScannerV2::_prepare_next_split(bool* eos) { const auto format_type = get_range_format_type(*_params, _current_range); _init_adaptive_batch_size_state(format_type); - if (_should_run_adaptive_batch_size()) { - // JNI readers open eagerly in prepare_split(). Seed the probe size first so readers - // such as Paimon also use it for their first physical read batch. + if (_block_size_predictor != nullptr) { + // JNI readers open eagerly in prepare_split(). Always seed the probe before preparing + // the next split: its metadata-COUNT decision is not available yet, and the state + // exposed by TableReader can still describe the preceding split. Metadata shortcuts + // ignore this batch size, while row-scan fallbacks need it for their first physical + // read batch. _table_reader->set_batch_size(_predict_reader_batch_rows()); } std::map partition_values; @@ -863,10 +866,17 @@ bool FileScannerV2::_should_enable_adaptive_batch_size(TFileFormatType::type for } 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; + DORIS_CHECK(_table_reader != nullptr); + return _should_run_adaptive_batch_size(_block_size_predictor != nullptr, + _table_reader->current_split_uses_metadata_count()); +} + +bool FileScannerV2::_should_run_adaptive_batch_size(bool predictor_initialized, + bool current_split_uses_metadata_count) { + // Metadata COUNT emits synthetic rows and has no physical row width to learn from. A raw COUNT + // opcode is not sufficient here: unsupported argument counts, mappings, filters, or deletes + // make TableReader fall back to materializing normal rows, which still need adaptive batching. + return predictor_initialized && !current_split_uses_metadata_count; } size_t FileScannerV2::_predict_reader_batch_rows() { diff --git a/be/src/exec/scan/file_scanner_v2.h b/be/src/exec/scan/file_scanner_v2.h index 452b1652bee461..0cd03030b56851 100644 --- a/be/src/exec/scan/file_scanner_v2.h +++ b/be/src/exec/scan/file_scanner_v2.h @@ -88,6 +88,11 @@ class FileScannerV2 final : public Scanner { RuntimeProfile* profile, const io::FileCacheStatistics& file_cache_statistics); static bool TEST_should_skip_not_found(const Status& status, bool ignore_not_found); static bool TEST_should_skip_empty(const Status& status, bool stopped); + static bool TEST_should_run_adaptive_batch_size(bool predictor_initialized, + bool current_split_uses_metadata_count) { + return _should_run_adaptive_batch_size(predictor_initialized, + current_split_uses_metadata_count); + } #endif FileScannerV2(RuntimeState* state, FileScanLocalState* parent, int64_t limit, @@ -140,6 +145,8 @@ class FileScannerV2 final : public Scanner { void _init_adaptive_batch_size_state(TFileFormatType::type format_type); bool _should_enable_adaptive_batch_size(TFileFormatType::type format_type) const; bool _should_run_adaptive_batch_size() const; + static bool _should_run_adaptive_batch_size(bool predictor_initialized, + bool current_split_uses_metadata_count); size_t _predict_reader_batch_rows(); void _update_adaptive_batch_size(const Block& block); static RealtimeCounterDeltas _collect_realtime_counter_deltas( diff --git a/be/src/format_v2/table_reader.cpp b/be/src/format_v2/table_reader.cpp index 4897e871cec3bb..e8725ed65fa49b 100644 --- a/be/src/format_v2/table_reader.cpp +++ b/be/src/format_v2/table_reader.cpp @@ -862,6 +862,7 @@ Status TableReader::prepare_split(const SplitReadOptions& options) { _deletion_vector = nullptr; _aggregate_pushdown_tried = false; _remaining_table_level_count = -1; + _current_split_uses_metadata_count = false; _current_reader_reached_eof = false; RETURN_IF_ERROR(_evaluate_partition_prune_conjuncts(options.partition_prune_conjuncts, &_current_split_pruned)); @@ -887,6 +888,7 @@ Status TableReader::prepare_split(const SplitReadOptions& options) { DORIS_CHECK(options.current_range.table_format_params.table_level_row_count >= -1); _remaining_table_level_count = options.current_range.table_format_params.table_level_row_count; + _current_split_uses_metadata_count = _is_table_level_count_active(); } if (_is_table_level_count_active()) { return Status::OK(); diff --git a/be/src/format_v2/table_reader.h b/be/src/format_v2/table_reader.h index e3c3ca9a07dc90..b37fc3d0604a95 100644 --- a/be/src/format_v2/table_reader.h +++ b/be/src/format_v2/table_reader.h @@ -211,6 +211,7 @@ class TableReader { virtual Status prepare_split(const SplitReadOptions& options); virtual bool current_split_pruned() const { return _current_split_pruned; } + bool current_split_uses_metadata_count() const { return _current_split_uses_metadata_count; } // Discard the active split after the caller decides an error is ignorable, for example a // stale external-table file listing that returns NOT_FOUND. The next prepare_split() must start @@ -224,6 +225,7 @@ class TableReader { } _delete_rows = nullptr; _remaining_table_level_count = -1; + _current_split_uses_metadata_count = false; _current_split_pruned = false; return Status::OK(); } @@ -319,6 +321,7 @@ class TableReader { _current_task.reset(); _current_file_description.reset(); _remaining_table_level_count = -1; + _current_split_uses_metadata_count = false; return Status::OK(); } @@ -962,6 +965,9 @@ class TableReader { RETURN_IF_ERROR(status); RETURN_IF_ERROR( _materialize_aggregate_pushdown_rows(_push_down_agg_type, file_result, block)); + if (_push_down_agg_type == TPushAggOp::type::COUNT) { + _current_split_uses_metadata_count = true; + } *pushed_down = true; RETURN_IF_ERROR(close_current_reader()); return Status::OK(); @@ -1663,6 +1669,10 @@ class TableReader { int64_t _condition_cache_hit_count = 0; bool _current_reader_reached_eof = false; int64_t _remaining_table_level_count = -1; + // True only after the active split selects a table-level row-count shortcut or successfully + // materializes COUNT rows from file metadata. FileScannerV2 uses this result, rather than the + // raw aggregate opcode, to keep adaptive batching enabled for normal row-scan fallbacks. + bool _current_split_uses_metadata_count = false; // Snapshot supplied by FileScannerV2 for the active split. It gates every shortcut that emits // irreversible aggregate rows, not only the table-level row-count shortcut in prepare_split(). bool _all_runtime_filters_applied_for_split = true; diff --git a/be/test/exec/scan/file_scanner_v2_test.cpp b/be/test/exec/scan/file_scanner_v2_test.cpp index 5a21cfc7081177..762598456e09e1 100644 --- a/be/test/exec/scan/file_scanner_v2_test.cpp +++ b/be/test/exec/scan/file_scanner_v2_test.cpp @@ -116,6 +116,12 @@ TEST(FileScannerTest, V1CountPushdownRequiresExplicitCountStarArguments) { TPushAggOp::type::MINMAX, std::nullopt)); } +TEST(FileScannerV2Test, AdaptiveBatchSizeRunsForCountFallbackOnly) { + EXPECT_TRUE(FileScannerV2::TEST_should_run_adaptive_batch_size(true, false)); + EXPECT_FALSE(FileScannerV2::TEST_should_run_adaptive_batch_size(true, true)); + EXPECT_FALSE(FileScannerV2::TEST_should_run_adaptive_batch_size(false, false)); +} + struct RetryableCloseState { int close_calls = 0; }; diff --git a/be/test/format_v2/table_reader_test.cpp b/be/test/format_v2/table_reader_test.cpp index 7c50a11dd5dc88..d0096990357935 100644 --- a/be/test/format_v2/table_reader_test.cpp +++ b/be/test/format_v2/table_reader_test.cpp @@ -1765,6 +1765,7 @@ TEST(TableReaderTest, PushDownCountRecordsReaderRowsBeforeClosingReader) { EXPECT_EQ(block.rows(), 3); EXPECT_EQ(file_reader_stats.read_rows, 3); EXPECT_EQ(fake_state->close_count, 1); + EXPECT_TRUE(reader.current_split_uses_metadata_count()); ASSERT_TRUE(fake_state->last_request != nullptr); EXPECT_TRUE(fake_state->last_request->count_star_placeholder_columns.empty()); ASSERT_TRUE(fake_state->last_aggregate_request.has_value()); @@ -1859,6 +1860,51 @@ TEST(TableReaderTest, PushDownCountFallsBackWhenSemanticArgumentsAreAbsent) { EXPECT_EQ(block.rows(), 2); // The explicit aggregate_count=3 would be returned if absence were confused with COUNT(*). EXPECT_FALSE(fake_state->last_aggregate_request.has_value()); + EXPECT_FALSE(reader.current_split_uses_metadata_count()); +} + +TEST(TableReaderTest, PushDownCountFallsBackForMultipleArguments) { + const auto nullable_int_type = make_nullable(std::make_shared()); + const auto nullable_string_type = make_nullable(std::make_shared()); + std::vector file_schema; + file_schema.push_back(make_file_column(0, "id", nullable_int_type)); + file_schema.push_back(make_file_column(1, "name", nullable_string_type)); + + std::vector projected_columns; + projected_columns.push_back(make_table_column(0, "id", nullable_int_type)); + projected_columns.push_back(make_table_column(1, "name", nullable_string_type)); + set_name_identifiers(&projected_columns); + + RuntimeState state {TQueryOptions(), TQueryGlobals()}; + auto fake_state = std::make_shared(); + fake_state->aggregate_count = 3; + FakeTableReader reader(file_schema, fake_state); + ASSERT_TRUE( + reader.init({ + .projected_columns = projected_columns, + .conjuncts = {}, + .format = FileFormat::PARQUET, + .scan_params = nullptr, + .io_ctx = nullptr, + .runtime_state = &state, + .scanner_profile = nullptr, + .push_down_agg_type = TPushAggOp::type::COUNT, + .push_down_count_columns = + std::vector {GlobalIndex(0), GlobalIndex(1)}, + }) + .ok()); + + SplitReadOptions split_options; + split_options.current_range.__set_path("fake-table-reader-input"); + ASSERT_TRUE(reader.prepare_split(split_options).ok()); + + Block block = build_table_block(projected_columns); + bool eos = false; + ASSERT_TRUE(reader.get_block(&block, &eos).ok()); + EXPECT_FALSE(eos); + EXPECT_EQ(block.rows(), 2); + EXPECT_FALSE(fake_state->last_aggregate_request.has_value()); + EXPECT_FALSE(reader.current_split_uses_metadata_count()); } TEST(TableReaderTest, PushDownCountFallsBackForNullableToRequiredMapping) { From 0b0925a288023f9bdfecd84ee30edd10706ab6ac Mon Sep 17 00:00:00 2001 From: Gabriel Date: Thu, 16 Jul 2026 16:52:56 +0800 Subject: [PATCH 16/17] [fix](be) Preserve COUNT pushdown through hybrid readers ### What problem does this PR solve? Issue Number: None Related PR: #65548 Problem Summary: HudiHybridReader and PaimonHybridReader forwarded the aggregate opcode to their active child readers but dropped the optional COUNT argument state. New BEs therefore treated explicit COUNT(*) and safe COUNT(column) plans as unknown old-FE semantics and fell back to full row scans. The hybrid wrappers also hid the child metadata-COUNT state from FileScannerV2, allowing synthetic aggregate rows to reach adaptive batch-size sampling. Forward COUNT arguments to both native and JNI children and delegate the active split metadata state through the hybrid reader. ### Release note None ### Check List (For Author) - Test: Unit Test - HudiHybridReaderTest.* - PaimonHybridReaderTest.* - FileScannerV2 and TableReader COUNT coverage - Behavior changed: No. Query results are unchanged; eligible Hudi and Paimon COUNT scans retain metadata aggregation and correct adaptive-batch accounting. - Does this need documentation: No --- be/src/format_v2/table/hudi_reader.cpp | 6 ++ be/src/format_v2/table/hudi_reader.h | 1 + be/src/format_v2/table/paimon_reader.cpp | 6 ++ be/src/format_v2/table/paimon_reader.h | 1 + be/src/format_v2/table_reader.h | 4 +- be/test/format_v2/table/hudi_reader_test.cpp | 99 +++++++++++++++++++ .../format_v2/table/paimon_reader_test.cpp | 53 ++++++++++ 7 files changed, 169 insertions(+), 1 deletion(-) diff --git a/be/src/format_v2/table/hudi_reader.cpp b/be/src/format_v2/table/hudi_reader.cpp index 4294d51d043d9e..6c8917d867074c 100644 --- a/be/src/format_v2/table/hudi_reader.cpp +++ b/be/src/format_v2/table/hudi_reader.cpp @@ -81,6 +81,11 @@ bool HudiHybridReader::current_split_pruned() const { return _current_split_reader->current_split_pruned(); } +bool HudiHybridReader::current_split_uses_metadata_count() const { + DORIS_CHECK(_current_split_reader != nullptr); + return _current_split_reader->current_split_uses_metadata_count(); +} + Status HudiHybridReader::abort_split() { DORIS_CHECK(_current_split_reader != nullptr); return _current_split_reader->abort_split(); @@ -145,6 +150,7 @@ Status HudiHybridReader::_init_child_reader(format::TableReader* reader, .runtime_state = _runtime_state, .scanner_profile = _scanner_profile, .push_down_agg_type = _push_down_agg_type, + .push_down_count_columns = _push_down_count_columns, .condition_cache_digest = _condition_cache_digest, })); // Zero means no adaptive prediction has been produced yet. Preserve the child's normal diff --git a/be/src/format_v2/table/hudi_reader.h b/be/src/format_v2/table/hudi_reader.h index e22c6bd866f061..77a71cd79ba166 100644 --- a/be/src/format_v2/table/hudi_reader.h +++ b/be/src/format_v2/table/hudi_reader.h @@ -60,6 +60,7 @@ class HudiHybridReader final : public format::TableReader { Status prepare_split(const format::SplitReadOptions& options) override; Status get_block(Block* block, bool* eos) override; bool current_split_pruned() const override; + bool current_split_uses_metadata_count() const override; Status abort_split() override; Status close() override; void set_batch_size(size_t batch_size) override; diff --git a/be/src/format_v2/table/paimon_reader.cpp b/be/src/format_v2/table/paimon_reader.cpp index 258bbb5c021dc0..74a38515622f81 100644 --- a/be/src/format_v2/table/paimon_reader.cpp +++ b/be/src/format_v2/table/paimon_reader.cpp @@ -106,6 +106,11 @@ bool PaimonHybridReader::current_split_pruned() const { return _current_split_reader->current_split_pruned(); } +bool PaimonHybridReader::current_split_uses_metadata_count() const { + DORIS_CHECK(_current_split_reader != nullptr); + return _current_split_reader->current_split_uses_metadata_count(); +} + Status PaimonHybridReader::abort_split() { DORIS_CHECK(_current_split_reader != nullptr); return _current_split_reader->abort_split(); @@ -173,6 +178,7 @@ Status PaimonHybridReader::_init_child_reader(format::TableReader* reader, .runtime_state = _runtime_state, .scanner_profile = _scanner_profile, .push_down_agg_type = _push_down_agg_type, + .push_down_count_columns = _push_down_count_columns, .condition_cache_digest = _condition_cache_digest, })); // Zero means no adaptive prediction has been produced yet. Preserve the child's normal diff --git a/be/src/format_v2/table/paimon_reader.h b/be/src/format_v2/table/paimon_reader.h index fc94777b6dafb3..634f2bd7a6400c 100644 --- a/be/src/format_v2/table/paimon_reader.h +++ b/be/src/format_v2/table/paimon_reader.h @@ -66,6 +66,7 @@ class PaimonHybridReader final : public format::TableReader { Status prepare_split(const format::SplitReadOptions& options) override; Status get_block(Block* block, bool* eos) override; bool current_split_pruned() const override; + bool current_split_uses_metadata_count() const override; Status abort_split() override; Status close() override; void set_batch_size(size_t batch_size) override; diff --git a/be/src/format_v2/table_reader.h b/be/src/format_v2/table_reader.h index b37fc3d0604a95..cddddccd6bd3e1 100644 --- a/be/src/format_v2/table_reader.h +++ b/be/src/format_v2/table_reader.h @@ -211,7 +211,9 @@ class TableReader { virtual Status prepare_split(const SplitReadOptions& options); virtual bool current_split_pruned() const { return _current_split_pruned; } - bool current_split_uses_metadata_count() const { return _current_split_uses_metadata_count; } + virtual bool current_split_uses_metadata_count() const { + return _current_split_uses_metadata_count; + } // Discard the active split after the caller decides an error is ignorable, for example a // stale external-table file listing that returns NOT_FOUND. The next prepare_split() must start diff --git a/be/test/format_v2/table/hudi_reader_test.cpp b/be/test/format_v2/table/hudi_reader_test.cpp index 28c396371e4559..2dd001469d8a66 100644 --- a/be/test/format_v2/table/hudi_reader_test.cpp +++ b/be/test/format_v2/table/hudi_reader_test.cpp @@ -17,14 +17,20 @@ #include "format_v2/table/hudi_reader.h" +#include +#include #include +#include +#include +#include #include #include #include #include #include +#include "core/data_type/data_type_nullable.h" #include "core/data_type/data_type_number.h" #include "core/data_type/data_type_string.h" #include "core/data_type/data_type_struct.h" @@ -32,6 +38,9 @@ #include "format_v2/column_data.h" #include "gen_cpp/ExternalTableSchema_types.h" #include "gen_cpp/PlanNodes_types.h" +#include "io/io_common.h" +#include "runtime/runtime_profile.h" +#include "runtime/runtime_state.h" namespace doris::format { namespace { @@ -69,6 +78,39 @@ ColumnDefinition make_file_column(int32_t id, const std::string& name, const Dat return field; } +ColumnDefinition make_table_column(int32_t id, const std::string& name, const DataTypePtr& type) { + ColumnDefinition column; + column.identifier = Field::create_field(id); + column.name = name; + column.type = make_nullable(type); + return column; +} + +Block build_table_block(const std::vector& columns) { + Block block; + for (const auto& column : columns) { + block.insert({column.type->create_column(), column.type, column.name}); + } + return block; +} + +void write_int_parquet_file(const std::string& file_path, const std::vector& values) { + arrow::Int32Builder value_builder; + for (const auto value : values) { + ASSERT_TRUE(value_builder.Append(value).ok()); + } + std::shared_ptr value_array; + ASSERT_TRUE(value_builder.Finish(&value_array).ok()); + const auto table = arrow::Table::Make( + arrow::schema({arrow::field("id", arrow::int32(), false)}), {value_array}); + + auto file_result = arrow::io::FileOutputStream::Open(file_path); + ASSERT_TRUE(file_result.ok()) << file_result.status(); + std::shared_ptr output = *file_result; + PARQUET_THROW_NOT_OK(::parquet::arrow::WriteTable(*table, arrow::default_memory_pool(), output, + static_cast(values.size()))); +} + TTableFormatFileDesc hudi_table_format_desc(std::optional schema_id) { TTableFormatFileDesc table_format_params; table_format_params.__set_table_format_type("hudi"); @@ -201,5 +243,62 @@ TEST(HudiHybridReaderTest, AdaptiveBatchSizeReachesBothChildReaders) { EXPECT_EQ(child_batch_sizes.second, 123); } +TEST(HudiHybridReaderTest, NativeCountStarReportsMetadataRowsThroughHybridReader) { + const auto test_dir = + std::filesystem::temp_directory_path() / "doris_hudi_hybrid_count_star_test"; + std::filesystem::remove_all(test_dir); + std::filesystem::create_directories(test_dir); + const auto file_path = (test_dir / "base-file.parquet").string(); + write_int_parquet_file(file_path, {1, 2, 3}); + + const std::vector projected_columns { + make_table_column(0, "id", std::make_shared()), + }; + RuntimeProfile profile("test_profile"); + RuntimeState state {TQueryOptions(), TQueryGlobals()}; + TFileScanRangeParams scan_params; + scan_params.__set_file_type(TFileType::FILE_LOCAL); + scan_params.__set_format_type(TFileFormatType::FORMAT_PARQUET); + io::FileReaderStats file_reader_stats; + io::FileCacheStatistics file_cache_stats; + auto io_ctx = std::make_shared(); + io_ctx->file_reader_stats = &file_reader_stats; + io_ctx->file_cache_stats = &file_cache_stats; + ShardedKVCache cache(1); + + hudi::HudiHybridReader reader; + ASSERT_TRUE(reader.init({ + .projected_columns = projected_columns, + .conjuncts = {}, + .format = FileFormat::PARQUET, + .scan_params = &scan_params, + .io_ctx = io_ctx, + .runtime_state = &state, + .scanner_profile = &profile, + .push_down_agg_type = TPushAggOp::type::COUNT, + .push_down_count_columns = std::vector {}, + }) + .ok()); + + SplitReadOptions split_options; + split_options.cache = &cache; + split_options.current_split_format = FileFormat::PARQUET; + split_options.current_range.__set_path(file_path); + split_options.current_range.__set_file_size( + static_cast(std::filesystem::file_size(file_path))); + split_options.current_range.__set_format_type(TFileFormatType::FORMAT_PARQUET); + split_options.current_range.__set_table_format_params(hudi_table_format_desc(std::nullopt)); + ASSERT_TRUE(reader.prepare_split(split_options).ok()); + + Block block = build_table_block(projected_columns); + bool eos = false; + ASSERT_TRUE(reader.get_block(&block, &eos).ok()); + EXPECT_EQ(block.rows(), 3); + EXPECT_TRUE(reader.current_split_uses_metadata_count()); + + ASSERT_TRUE(reader.close().ok()); + std::filesystem::remove_all(test_dir); +} + } // namespace } // namespace doris::format diff --git a/be/test/format_v2/table/paimon_reader_test.cpp b/be/test/format_v2/table/paimon_reader_test.cpp index 6a3f20fac52726..ae1691a774f899 100644 --- a/be/test/format_v2/table/paimon_reader_test.cpp +++ b/be/test/format_v2/table/paimon_reader_test.cpp @@ -661,6 +661,59 @@ TEST(PaimonHybridReaderTest, AdaptiveBatchSizeReachesBothChildReaders) { EXPECT_EQ(child_batch_sizes.second, 321); } +TEST(PaimonHybridReaderTest, NativeCountColumnReportsMetadataRowsThroughHybridReader) { + const auto test_dir = + std::filesystem::temp_directory_path() / "doris_paimon_hybrid_count_column_test"; + std::filesystem::remove_all(test_dir); + std::filesystem::create_directories(test_dir); + const auto file_path = (test_dir / "data-file.parquet").string(); + write_int_pair_parquet_file(file_path, {1, 2, 3}, {10, 20, 30}, {"one", "two", "three"}); + + const std::vector projected_columns { + make_table_column(0, "id", std::make_shared()), + }; + RuntimeProfile profile("test_profile"); + RuntimeState state {TQueryOptions(), TQueryGlobals()}; + auto scan_params = make_local_parquet_scan_params(); + io::FileReaderStats file_reader_stats; + io::FileCacheStatistics file_cache_stats; + auto io_ctx = make_io_context(&file_reader_stats, &file_cache_stats); + ShardedKVCache cache(1); + + paimon::PaimonHybridReader reader; + ASSERT_TRUE(reader.init({ + .projected_columns = projected_columns, + .conjuncts = {}, + .format = FileFormat::PARQUET, + .scan_params = &scan_params, + .io_ctx = io_ctx, + .runtime_state = &state, + .scanner_profile = &profile, + .push_down_agg_type = TPushAggOp::type::COUNT, + .push_down_count_columns = + std::vector {GlobalIndex(0)}, + }) + .ok()); + + SplitReadOptions split_options; + split_options.cache = &cache; + split_options.current_split_format = FileFormat::PARQUET; + split_options.current_range = make_paimon_native_range(TFileFormatType::FORMAT_PARQUET); + split_options.current_range.__set_path(file_path); + split_options.current_range.__set_file_size( + static_cast(std::filesystem::file_size(file_path))); + ASSERT_TRUE(reader.prepare_split(split_options).ok()); + + Block block = build_table_block(projected_columns); + bool eos = false; + ASSERT_TRUE(reader.get_block(&block, &eos).ok()); + EXPECT_EQ(block.rows(), 3); + EXPECT_TRUE(reader.current_split_uses_metadata_count()); + + ASSERT_TRUE(reader.close().ok()); + std::filesystem::remove_all(test_dir); +} + TEST(PaimonHybridReaderTest, DispatchesNativeThenJniSplitToMatchingReader) { RuntimeProfile profile("test_profile"); RuntimeState state {TQueryOptions(), TQueryGlobals()}; From 5ecb49361259519cf8010046c1c0468a7335c7b4 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Thu, 16 Jul 2026 19:28:24 +0800 Subject: [PATCH 17/17] [test](be) Verify unsafe nested filter scan contract ### What problem does this PR solve? Issue Number: None Related PR: #65548 Problem Summary: Nested fallback tests required the source column to stay in predicate_columns even though unsafe predicates remain table-level and readers may classify the required source as non-predicate. Verify the stable scan contract instead: the source column is scanned exactly once with the expected file-local id and no file conjunct is produced. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter="ColumnMapperCastTest.NestedElementAtConjunctStaysTableLevelForNullableFileLeafMappedToRequiredTableLeaf:ColumnMapperCastTest.NestedElementAtConjunctStaysTableLevelForNonLosslessFileToTableCast" (2 tests passed) - ./run-be-ut.sh --run --filter="ColumnMapper*.*" (111 tests passed) - Behavior changed: No - Does this need documentation: No --- be/test/format_v2/column_mapper_test.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/be/test/format_v2/column_mapper_test.cpp b/be/test/format_v2/column_mapper_test.cpp index 16bdaad982d515..c0318d451187ed 100644 --- a/be/test/format_v2/column_mapper_test.cpp +++ b/be/test/format_v2/column_mapper_test.cpp @@ -3147,7 +3147,10 @@ TEST_F(ColumnMapperCastTest, FileScanRequest request; ASSERT_TRUE(mapper.create_scan_request({filter}, {table_struct}, &request, &state).ok()); - ASSERT_EQ(request.predicate_columns.size(), 1); + ASSERT_EQ(request.predicate_columns.size() + request.non_predicate_columns.size(), 1); + const auto& scan_column = request.predicate_columns.empty() ? request.non_predicate_columns[0] + : request.predicate_columns[0]; + EXPECT_EQ(scan_column.column_id(), LocalColumnId(5)); EXPECT_TRUE(request.conjuncts.empty()); } @@ -3176,7 +3179,10 @@ TEST_F(ColumnMapperCastTest, NestedElementAtConjunctStaysTableLevelForNonLossles FileScanRequest request; ASSERT_TRUE(mapper.create_scan_request({filter}, {table_struct}, &request, &state).ok()); - ASSERT_EQ(request.predicate_columns.size(), 1); + ASSERT_EQ(request.predicate_columns.size() + request.non_predicate_columns.size(), 1); + const auto& scan_column = request.predicate_columns.empty() ? request.non_predicate_columns[0] + : request.predicate_columns[0]; + EXPECT_EQ(scan_column.column_id(), LocalColumnId(5)); EXPECT_TRUE(request.conjuncts.empty()); }