This is an automated email from the ASF dual-hosted git repository.
morningman pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new b42e1ab294b [refactor](be) Remove FileScannerV2's per-range table
reader rebuild (#66589)
b42e1ab294b is described below
commit b42e1ab294b2a1199c4a60d4c7c087c669d13d49
Author: Mingyu Chen (Rayner) <[email protected]>
AuthorDate: Sat Aug 8 20:17:45 2026 +0800
[refactor](be) Remove FileScannerV2's per-range table reader rebuild
(#66589)
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #66403, #66399
Problem Summary:
#66403 added a per-range table reader rebuild to `FileScannerV2`:
whenever a range carries a table format different from the one
`_table_reader` was built for, the reader is thrown away and rebuilt.
This removes it again, because nothing produces the situation it
handles.
It was added ahead of a connector that reads a table as a lake plus the
log written after it — fluss, proposed in #66399. That connector planned
its lake half through a sibling connector and its own half itself, so
one scan node was handed ranges of two table formats; a reader built
once from the first range is then given a foreign one and does not fail
cleanly, it fails as whatever that reader makes of it. Since which
ranges share a scanner is the engine's assignment, the same query would
succeed or fail by how the ranges happened to be dealt out.
That connector has since been changed — on review feedback — to carry
the one table format on every range of a scan and to make the per-range
choice inside its own table reader, which is the arrangement paimon and
hudi already use (`PaimonHybridReader`, `HudiHybridReader`). So the
producer this mechanism was built for no longer exists, in this tree or
in that branch.
**Nothing else produces a mixed-format scan node.** Checked, rather than
assumed:
- every `ConnectorScanRange` implementation in the tree returns a
**constant** table format string — `paimon`, `hudi`, `iceberg`, `jdbc`,
`max_compute`, `trino_connector`, `es`, `adbc`, `trino` — with one
exception;
- the exception is `HiveScanRange`, which returns `transactional_hive`
for ACID partitions and `hive` for the rest, so a hive ACID table
genuinely can plan two format strings into one node;
- but `transactional_hive` appears in none of
`FileScannerV2::is_supported`'s whitelists (`is_supported_table_format`,
`is_supported_arrow_table_format`, `is_supported_jni_table_format`), so
such a range never enters this scanner at all — it stays on the V1 path.
Removing the mechanism also removes a subtlety it carried. It rebuilt
the reader but deliberately **not** the expression contexts, because
`_init_expr_ctxes` is not idempotent. That is correct only while every
reader a scanner could switch between annotates its projected columns
identically — a property each newly added table format had to preserve
by accident, with nothing checking it. With one reader per scanner, that
property is structural instead.
Net effect: `-95` lines, no behaviour change.
### Release note
None
### Check List (For Author)
- Test
- [ ] Regression test
- [ ] Unit Test
- [x] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
The unit test removed here is the one that covered the removed method;
it drove `_rebuild_table_reader_if_format_changed` directly and has
nothing left to call. No other test in `file_scanner_v2_test.cpp`
references the method or the `_table_reader_format` member — the shared
`range_with_format` helper it used stays, with nine other callers.
Verified locally: `be/src/exec/scan/file_scanner_v2.cpp` and
`be/test/exec/scan/file_scanner_v2_test.cpp` compile clean under the
project's own flags (`-Wall -Wextra -Werror`) with the mechanism gone,
and `clang-format --dry-run -Werror` is clean on all three files. The
identical deletion has been running in the #66399 branch, where
`FileScannerV2*:FileScannerTest*:Fluss*` (73 tests) and the fluss
end-to-end suites (12 suites, 0 skipped) are green against a real fluss
+ flink + paimon cluster.
- Behavior changed:
- [x] No.
- Does this need documentation?
- [x] No.
Co-authored-by: Claude Fable 5 <[email protected]>
---
be/src/exec/scan/file_scanner_v2.cpp | 34 -------------------
be/src/exec/scan/file_scanner_v2.h | 7 ----
be/test/exec/scan/file_scanner_v2_test.cpp | 54 ------------------------------
3 files changed, 95 deletions(-)
diff --git a/be/src/exec/scan/file_scanner_v2.cpp
b/be/src/exec/scan/file_scanner_v2.cpp
index 052d465daa2..420bbd62a42 100644
--- a/be/src/exec/scan/file_scanner_v2.cpp
+++ b/be/src/exec/scan/file_scanner_v2.cpp
@@ -405,7 +405,6 @@ Status FileScannerV2::_open_impl(RuntimeState* state) {
if (_first_scan_range) {
RETURN_IF_ERROR(_create_table_reader_for_format(_current_range,
&_table_reader));
DORIS_CHECK(_table_reader != nullptr);
- _table_reader_format = table_format_name(_current_range);
RETURN_IF_ERROR(_init_expr_ctxes());
RETURN_IF_ERROR(_init_table_reader(_current_range));
}
@@ -511,14 +510,6 @@ Status FileScannerV2::_prepare_next_split(bool* eos) {
DORIS_CHECK(_table_reader != nullptr);
_current_range_path = _current_range.path;
- bool reader_rebuilt = false;
-
RETURN_IF_ERROR(_rebuild_table_reader_if_format_changed(_current_range,
&reader_rebuilt));
- if (reader_rebuilt) {
- // Same init the first reader got. The expression contexts are NOT
rebuilt: they are
- // per-scanner and format-independent, and _init_expr_ctxes is not
idempotent.
- RETURN_IF_ERROR(_init_table_reader(_current_range));
- }
-
const auto format_type = get_range_format_type(*_params,
_current_range);
_init_adaptive_batch_size_state(format_type);
if (_block_size_predictor != nullptr) {
@@ -601,31 +592,6 @@ Status FileScannerV2::_init_table_reader(const
TFileRangeDesc& range) {
return Status::OK();
}
-Status FileScannerV2::_rebuild_table_reader_if_format_changed(const
TFileRangeDesc& range,
- bool* rebuilt) {
- // The reader is chosen by the range's table format, not the node's,
because one node can be given
- // both: a connector that reads a table as a lake plus the log written
after it plans its lake half
- // through a sibling connector and its log half itself, and both land here
as ranges of the same
- // scan. Built once from the first range and never revisited, the reader
is then handed a range of
- // the other format -- which does not fail cleanly. It fails as whatever
that reader makes of a
- // foreign range, e.g. paimon's reporting an unsupported file format for a
range that carries no
- // paimon parameters at all. And which ranges share a scanner is up to the
engine's assignment, so
- // the same query succeeds or fails by how the ranges happened to be dealt
out.
- //
- // Split out from _prepare_next_split so the decision can be tested on its
own: re-initializing the
- // new reader needs scan-wide state that choosing it does not, so that
step stays with the caller.
- auto table_format = table_format_name(range);
- if (table_format == _table_reader_format) {
- *rebuilt = false;
- return Status::OK();
- }
- RETURN_IF_ERROR(_create_table_reader_for_format(range, &_table_reader));
- DORIS_CHECK(_table_reader != nullptr);
- _table_reader_format = std::move(table_format);
- *rebuilt = true;
- return Status::OK();
-}
-
Status FileScannerV2::_create_table_reader_for_format(
const TFileRangeDesc& range, std::unique_ptr<format::TableReader>*
reader) const {
DORIS_CHECK(reader != nullptr);
diff --git a/be/src/exec/scan/file_scanner_v2.h
b/be/src/exec/scan/file_scanner_v2.h
index 03e5f4d6bbc..f7141fa3919 100644
--- a/be/src/exec/scan/file_scanner_v2.h
+++ b/be/src/exec/scan/file_scanner_v2.h
@@ -129,9 +129,6 @@ private:
Status _init_table_reader(const TFileRangeDesc& range);
Status _create_table_reader_for_format(const TFileRangeDesc& range,
std::unique_ptr<format::TableReader>* reader) const;
- // Replaces _table_reader when {@code range} carries a different table
format than the one it was
- // built for, reporting whether it did. See the definition for why the
reader follows the range.
- Status _rebuild_table_reader_if_format_changed(const TFileRangeDesc&
range, bool* rebuilt);
Status _prepare_table_reader_split(const TFileRangeDesc& range,
std::map<std::string, Field>
partition_values);
static bool _should_skip_not_found(const Status& status, bool
ignore_not_found);
@@ -185,10 +182,6 @@ private:
std::string _current_range_path;
std::unique_ptr<format::TableReader> _table_reader;
- // The table format _table_reader was built for. A scan node may mix table
formats -- a fluss
- // union read gives one node its lake half as paimon ranges and its log
half as fluss ones -- and
- // the reader is format-specific, so it is rebuilt whenever this stops
matching the range.
- std::string _table_reader_format;
std::vector<format::ColumnDefinition> _projected_columns;
// File formats without embedded schema, such as CSV, still need the FE
slot descriptors in
// file-column order. This mirrors old FileScanner::_file_slot_descs and
is passed only to
diff --git a/be/test/exec/scan/file_scanner_v2_test.cpp
b/be/test/exec/scan/file_scanner_v2_test.cpp
index 3506e5db27a..353c08043ad 100644
--- a/be/test/exec/scan/file_scanner_v2_test.cpp
+++ b/be/test/exec/scan/file_scanner_v2_test.cpp
@@ -475,60 +475,6 @@ TEST(FileScannerV2Test,
JniCompatibilityShapesUseV2Scanner) {
EXPECT_TRUE(FileScannerV2::is_supported(params,
legacy_paimon_jni_range_without_reader_type()));
}
-// Scenario: one scan node is given ranges of two different table formats,
which is what a connector
-// reading a table as a lake plus the log written after it produces -- its
lake half planned by a
-// sibling connector, its own half by itself. The reader is format-specific,
so it has to follow the
-// RANGE. Built once from the first range, it is later handed a foreign one
and fails as whatever that
-// reader makes of it, not as a clean error; and since which ranges share a
scanner is the engine's
-// assignment, the same query then succeeds or fails by how the ranges
happened to be dealt out.
-TEST(FileScannerV2Test, TheTableReaderIsRebuiltWhenARangeChangesTableFormat) {
- RuntimeState state {TQueryOptions(), TQueryGlobals()};
- RuntimeProfile profile("file_scanner_v2_reader_per_range");
- TFileScanRangeParams params;
- params.__set_format_type(TFileFormatType::FORMAT_PARQUET);
-
- FileScannerV2 scanner(&state, &profile, nullptr);
- scanner._params = ¶ms;
-
- const auto paimon_range = range_with_format("paimon",
TFileFormatType::FORMAT_PARQUET);
- const auto hive_range = range_with_format("hive",
TFileFormatType::FORMAT_PARQUET);
-
- // Nothing has been built yet, so the first range always builds.
- bool rebuilt = false;
- ASSERT_TRUE(scanner._rebuild_table_reader_if_format_changed(paimon_range,
&rebuilt).ok());
- EXPECT_TRUE(rebuilt);
- EXPECT_EQ(scanner._table_reader_format, "paimon");
- const auto* first_reader = scanner._table_reader.get();
- ASSERT_NE(first_reader, nullptr);
-
- // A second range of the same format reuses it. Rebuilding here would be
wasteful rather than
- // wrong, but it would also throw away per-reader state the next split
expects to still be there.
- ASSERT_TRUE(scanner._rebuild_table_reader_if_format_changed(paimon_range,
&rebuilt).ok());
- EXPECT_FALSE(rebuilt);
- EXPECT_EQ(scanner._table_reader.get(), first_reader);
-
- // A range of another format must not be handed to the reader built for
the first one.
- ASSERT_TRUE(scanner._rebuild_table_reader_if_format_changed(hive_range,
&rebuilt).ok());
- EXPECT_TRUE(rebuilt);
- EXPECT_EQ(scanner._table_reader_format, "hive");
- EXPECT_NE(scanner._table_reader.get(), first_reader);
-
- // And back again, because the ranges of a mixed node arrive interleaved
rather than grouped.
- ASSERT_TRUE(scanner._rebuild_table_reader_if_format_changed(paimon_range,
&rebuilt).ok());
- EXPECT_TRUE(rebuilt);
- EXPECT_EQ(scanner._table_reader_format, "paimon");
-
- // The formats really do get different readers -- otherwise every
assertion above would hold
- // just as well for a scanner that never rebuilt anything.
- std::unique_ptr<format::TableReader> as_paimon;
- std::unique_ptr<format::TableReader> as_hive;
- ASSERT_TRUE(scanner._create_table_reader_for_format(paimon_range,
&as_paimon).ok());
- ASSERT_TRUE(scanner._create_table_reader_for_format(hive_range,
&as_hive).ok());
- const format::TableReader& paimon_reader = *as_paimon;
- const format::TableReader& hive_reader = *as_hive;
- EXPECT_STRNE(typeid(paimon_reader).name(), typeid(hive_reader).name());
-}
-
TEST(FileScannerV2Test, FailedTableReaderCloseCanBeRetriedThroughScanner) {
RuntimeState state {TQueryOptions(), TQueryGlobals()};
RuntimeProfile profile("file_scanner_v2_close_retry");
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
