[fix](file) Resolve remaining FileScannerV2 audit issues#65931
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
TPC-H: Total hot run time: 29325 ms |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated code review: request changes.
I found seven patch-scoped issues in the FileScannerV2 audit changes: five P1 correctness/resource-safety problems and two P2 lifecycle/performance problems.
Checkpoint conclusions:
- Goal and scope: the patch remains focused on
be/src/format_v2; unchanged V1 paths were explicitly excluded by the PR and were not reported. - Correctness and compatibility: the new STRUCT collision check rejects already-resolved field-ID/partial projections. Legacy LIST/MAP parsing, row-lineage opt-in, JDBC casts, JSON buffer ownership, delete-expression ownership, dictionary identity, and hybrid counters otherwise traced cleanly.
- Concurrency and lifecycle: Flight stream destruction order is safe, but cancellation is armed after
DoGet, normal scanner stop cannot interrupt an in-flightNext, the watcher lacks query thread context, and its polling sleep delays every synchronous split close. - Memory and error handling: optional Data Page V2 and nonempty fixed-width dictionaries still allocate attacker-declared decompression extents before validating known sizes; retained decompression capacity cannot age out under the production read cadence.
- Performance and observability: counter propagation and JSON lookup changes are sound; the decompression retention and Flight close-delay findings remain.
- Tests: the added tests do not exercise blocked Flight setup/read, the production watcher close path, compressed optional V2 pages, compressed nonempty dictionaries, or real batch/page scratch cadence.
- Configuration, persistence, transactions, and data writes: no applicable changes found.
Validation was static only, as required by the review task; no build or tests were run. The checkout also lacks thirdparty/installed. The PR's reported test results were not independently rerun. No additional user focus was supplied.
| std::map<std::string, const ParquetColumnSchema*> file_children; | ||
| for (const auto& child : file_schema.children) { | ||
| file_children.emplace(to_lower(child->name), child.get()); | ||
| const auto [_, inserted] = file_children.emplace(to_lower(child->name), child.get()); |
There was a problem hiding this comment.
[P1] Preserve resolved field identity before rejecting case folds
This rejects every case-folded collision in the physical STRUCT before looking at the already-pruned projected_type. For an Iceberg schema s<Value(id=1), value(id=2), other(id=3)>, a field-ID projection of only s.other is fully resolved by TableColumnMapper/projected_type(), yet this loop still returns CORRUPTION because the unrelated siblings collide. It also prevents selecting either case-distinct child by its field ID. Please use the resolved local/field identity (or exact writer-name match) first and report ambiguity only when a requested child actually needs a case-insensitive fallback; add a partial-projection test.
| // cannot outlive the query execution timeout indefinitely. | ||
| options.timeout = std::chrono::seconds(_timeout_seconds); | ||
| options.stop_token = _stop_source.token(); | ||
| RETURN_DORIS_STATUS_IF_ERROR(_flight_client->DoGet(options, ticket).Value(&_stream)); |
There was a problem hiding this comment.
[P1] Arm cancellation for in-flight setup and normal scanner stop
The only code that signals _stop_source is created after synchronous DoGet() returns, so query cancellation while the server withholds initial stream data cannot interrupt setup and waits for the full Flight deadline. After open, the watcher polls only RuntimeState::is_cancelled(); reaching LIMIT instead calls FileScannerV2::try_stop() and sets only IOContext::should_stop, so a scanner already blocked in Next() is likewise not cancelled. The added fake test sets should_stop before entering Next() and misses both paths. Please arm a thread-safe cancellation bridge before DoGet() and have both query cancellation and scanner stop cancel the in-flight call; cover blocked setup and blocked read.
| options.timeout = std::chrono::seconds(_timeout_seconds); | ||
| options.stop_token = _stop_source.token(); | ||
| RETURN_DORIS_STATUS_IF_ERROR(_flight_client->DoGet(options, ticket).Value(&_stream)); | ||
| _cancellation_watcher = std::jthread([this](std::stop_token stop_token) { |
There was a problem hiding this comment.
[P2] Attach the cancellation thread to the query context
This query-owned std::jthread does not inherit the scanner worker's thread-local context, and the lambda enters without SCOPED_ATTACH_TASK (or even SCOPED_INIT_THREAD_CONTEXT). That violates the BE runtime thread-entry contract and leaves cancellation work outside the query's resource/memory accounting; any Doris thread-local use on this path can also fail for an unattached thread. Attach the watcher to the retained RuntimeState/ResourceContext at lambda entry, or run it on an existing query-attached executor.
| Status validate_fixed_width_page_size(const tparquet::PageHeader& header, int32_t type_length, | ||
| level_t max_rep_level, level_t max_def_level, | ||
| bool schema_is_required) { | ||
| if (type_length <= 0 || max_rep_level != 0 || max_def_level != 0 || !schema_is_required) { |
There was a problem hiding this comment.
[P1] Validate optional V2 fixed-width extents before allocation
This early return skips every optional fixed-width page (max_def_level != 0), but Data Page V2 already provides num_values, num_nulls, and exact level byte lengths. A compressed optional INT32 page with one value can therefore declare uncompressed_page_size = INT32_MAX; PageReader bounds only the tiny compressed payload, then load_page_data() calls _reserve_decompress_buf() with nearly 2 GiB before decompression can reject it. Extend the pre-allocation check for V2 PLAIN/BYTE_STREAM_SPLIT using the non-null count and level extents, and test the real compressed optional-V2 load path.
| // Only the current encoding is active. Old decoder instances retain reusable capacity but | ||
| // must not make the high-water policy treat their last batch as current working memory. | ||
| return (_page_decoder == nullptr ? 0 : _page_decoder->active_scratch_bytes()) + | ||
| const size_t active_decompress_bytes = _page_uses_decompress_buf ? _decompress_buf_size : 0; |
There was a problem hiding this comment.
[P1] Let retained decompression capacity age at a safe page boundary
This reports the full allocation as active whenever any compressed page uses it. After a 64 MiB outlier, every later 4 KiB compressed page therefore still looks 64 MiB active, so release_batch_scratch() resets the three-idle counter at each periodic probe. The production loop normally loads the next page before that probe, while the test calls release directly in the artificial gap after next_page(); read_with_plain_filter() never runs the probe at all. Track the current payload separately, carry a pending release to a safe page boundary, and test a large page followed by small compressed pages with boundaries misaligned to the 16-batch cadence plus the direct PLAIN-filter path.
| Status validate_dictionary_page_size(const tparquet::PageHeader& header) { | ||
| DORIS_CHECK(header.__isset.dictionary_page_header); | ||
| const int32_t num_values = header.dictionary_page_header.num_values; | ||
| if (UNLIKELY(num_values < 0 || (num_values == 0 && header.uncompressed_page_size != 0))) { |
There was a problem hiding this comment.
[P1] Validate nonempty fixed-width dictionaries before allocation
This accepts every positive num_values. With a compressed fixed-width dictionary, a one-entry INT32 page can advertise uncompressed_page_size = INT32_MAX while keeping its compressed payload tiny; PageReader bounds only the compressed bytes, and _decode_dict_page() allocates nearly 2 GiB before decompression and set_dict() finally enforce the known four-byte extent. Pass the physical width into this pre-allocation check and validate the checked count/width product for fixed-width PLAIN dictionaries, with a compressed ColumnChunkReader regression rather than only the empty helper test.
| _stream->Cancel(); | ||
| return; | ||
| } | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(25)); |
There was a problem hiding this comment.
[P2] Wake the watcher promptly during stream close
request_stop() does not interrupt sleep_for, so a normally completed Flight stream can block in join() for the remainder of this 25 ms polling interval before cleanup proceeds. TableReader::get_block() synchronously closes the current reader at every split EOF, so workloads with many small or fast Remote Doris tickets accumulate roughly 12.5 ms average (25 ms worst-case) avoidable latency per split. Use a stop-token-aware timed wait or notify a condition variable before joining, and add a prompt-close test for the production watcher.
TPC-DS: Total hot run time: 176524 ms |
ClickBench: Total hot run time: 25.02 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated follow-up code review: request changes.
I found two additional patch-scoped issues on the current head: one P1 compatibility regression in legacy LIST/MAP schema parsing and one P2 query task-context violation in the newly introduced DoGet worker. I did not duplicate residual problems already covered by the existing Flight-cancellation and Parquet-allocation threads.
Checkpoint conclusions:
- Goal and scope: this 37-file follow-up remains focused on FileScannerV2 and format-v2 audit fixes. The current implementations for exact-name field identity, Iceberg-only row-lineage opt-in, delete-result ownership, watcher synchronization, supported fixed-width extent checks, scratch release, JSON/JDBC behavior, dictionary identity, delta geometry, and hybrid counters otherwise traced coherently.
- Correctness and compatibility: the new SET-wrapper exception misinterprets the Parquet-compatible legacy
MAP_KEY_VALUErepresentation when it is a direct LIST element, changing an array-of-map schema into an incompatible wrapper shape or mapping failure. - Concurrency and lifecycle: watcher publication, cancellation, locking, and close ordering traced cleanly, but the new query-owned DoGet worker uses a generic non-task thread context and may remain detached beyond query teardown. The still-uncancelled in-flight DoGet RPC is already covered by the existing review thread and was not duplicated.
- Memory and malformed input: the repaired supported page layouts and scratch/cache lifetimes traced cleanly. Remaining repeated-page, invalid-width, BOOLEAN-dictionary, and cadence variants are at allocation sites already covered by existing review threads, so no duplicate comments were added.
- Performance and observability: JSON lookup, condition-cache aggregation, and scanner delta reporting traced cleanly; no additional issue found.
- Tests: the new tests do not cover the legacy uncontained
MAP_KEY_VALUELIST shape or enforce task attachment for the new worker. Other residual test gaps are already requested by existing threads. - Configuration, persistence, transactions, and data writes: no applicable changes found.
Validation was static only, as required by the review task; no build or tests were run. This checkout lacks .worktree_initialized and thirdparty/installed, and the PR's reported test results were not independently rerun. No additional user focus was supplied.
| const bool nested_collection_annotation = | ||
| is_list_node(second_level) || | ||
| (is_map_node(second_level) && | ||
| (!second_level.__isset.converted_type || |
There was a problem hiding this comment.
[P1] Preserve legacy MAP_KEY_VALUE elements nested in LIST
This exclusion treats every repeated MAP_KEY_VALUE node inside a LIST as the one-key SET-wrapper case. However, Parquet's compatibility contract says a MAP_KEY_VALUE group that is not contained by a MAP group must be interpreted as a map. For a valid legacy shape such as LIST -> repeated element (MAP_KEY_VALUE) -> repeated map -> key/value, the previous branch called parse_map_field(..., true) and produced ARRAY<MAP<...>>; this branch now falls through to parse_struct_field() and exposes a different nested shape (or fails table mapping). Please distinguish the direct one-key wrapper of an enclosing SET from a legacy MAP_KEY_VALUE list element, and add coverage for the latter.
| [this](std::stop_token stop_token) { _watch_cancellation(stop_token); }); | ||
|
|
||
| std::thread do_get_thread([pending, options, ticket] { | ||
| SCOPED_INIT_THREAD_CONTEXT(); |
There was a problem hiding this comment.
[P2] Attach the new DoGet worker to the query task
The watcher now follows the task-thread contract, but this newly introduced worker uses SCOPED_INIT_THREAD_CONTEXT() even though it performs the query's DoGet and owns its client/stream cleanup. On cancellation it is detached and can remain alive until the full Flight deadline, after the reader and query have gone away, so its work runs in the orphan context rather than the query's resource context. This is a distinct new thread from the watcher covered by the earlier review. Prefer removing the worker by using Flight's call-establishment stop token; if it remains, capture a shared ResourceContext, attach it at entry, and ensure its Flight objects are destroyed before that attachment ends.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated final follow-up code review: request changes.
I found six additional patch-scoped issues on the current head: three P1 resource/concurrency defects and three P2 performance/test defects.
Checkpoint conclusions:
- Goal and scope: this 35-file follow-up remains focused on FileScannerV2/format-v2 audit repairs. It fixes several earlier findings, but the remaining allocation, cancellation, and dictionary-retention paths mean the stated hardening goal is not yet complete.
- Correctness and compatibility: the revised LIST/MAP/SET compatibility, exact-first field identity, delete-expression ownership, Iceberg-only row-lineage synthesis, JDBC nullability, and JSON borrowed-buffer behavior trace coherently. No new storage-format or result-correctness regression was found beyond the reported resource/performance paths.
- Concurrency and lifecycle: the new Flight watcher reads a plain stop flag concurrently with the operator-thread write, so LIMIT cancellation has a C++ data race. Pending-open locking, task attachment, stream ownership, and close ordering otherwise traced soundly; previously reported detached-open and polling-close concerns were not duplicated.
- Memory and malformed input: repeated V2 plus variable-width/V1 data pages, and variable-width dictionaries, can still allocate an attacker-declared near-2-GiB decompression extent before Snappy validates the true output. Scratch release and ordinary dictionary generation lifetimes otherwise remain safe.
- Performance and observability: STRUCT child resolution is now quadratic per selected Row Group for wide nested schemas, and TIMESTAMPTZ pointer identity rematerializes the typed dictionary every filter batch. Hudi/Paimon condition-cache totals and scanner delta publication remain monotonic and exact.
- Tests: the new blocking Flight tests can stall for the 3600-second default deadline when a fatal assertion exits before server release. The tests also miss the reported repeated/variable-width allocation paths, wide repeated-Row-Group mapping, and multi-batch TIMESTAMPTZ production mapping.
- Configuration, persistence, transactions, data writes, and FE-BE protocol: no applicable changes found.
- User focus: no additional user-provided focus was supplied.
Validation was static only, as required by the review task; no build or tests were run. The checkout lacks .worktree_initialized, thirdparty/installed, and thirdparty/installed/bin/protoc, and the PR's reported test results were not independently rerun.
| Status validate_fixed_width_page_size(const tparquet::PageHeader& header, int32_t type_length, | ||
| level_t max_rep_level, level_t max_def_level, | ||
| bool schema_is_required) { | ||
| if (type_length <= 0 || max_rep_level != 0) { |
There was a problem hiding this comment.
[P1] Close the remaining compressed data-page preallocation paths
This still returns before validation for repeated V2 leaves, although V2 provides num_values - num_nulls and exact repetition/definition byte lengths. The type_length <= 0 and V1 optional fallbacks likewise reach _reserve_decompress_buf(uncompressed_size) before Snappy reads the frame's true decoded length. A tiny compressed ARRAY<INT32> V2 page or one-value BYTE_ARRAY/optional V1 page can therefore advertise INT32_MAX and make Doris reserve nearly 2 GiB before rejecting it. The existing thread covered flat optional V2; these are distinct remaining layouts. Apply the exact V2 formula regardless of repetition, preflight codec output before reserving for layouts without a statically known extent, and add compressed regressions for both paths.
| return Status::Corruption("Parquet dictionary has {} values and {} uncompressed bytes", | ||
| num_values, header.uncompressed_page_size); | ||
| } | ||
| if (type_length > 0) { |
There was a problem hiding this comment.
[P1] Validate variable-width dictionary output before allocating it
BYTE_ARRAY keeps type_length == -1, so this helper accepts any positive uncompressed_page_size; _decode_dict_page() then allocates that advertised size before reading either the cached or file payload. A one-entry Snappy dictionary that really expands to a short length-prefixed value can therefore advertise INT32_MAX and force a near-2-GiB allocation before GetUncompressedLength exposes the mismatch. This is distinct from the existing fixed-width dictionary thread. Preflight the compressed frame's decoded length before allocation (and require it to match the page header), with a compressed BYTE_ARRAY regression.
| private: | ||
| bool _is_cancelled() const { | ||
| return (_runtime_state != nullptr && _runtime_state->is_cancelled()) || | ||
| (_io_ctx != nullptr && _io_ctx->should_stop); |
There was a problem hiding this comment.
[P1] Synchronize the scanner-stop signal read by this watcher
ScannerContext::stop_scanners() can call FileScannerV2::try_stop() on the operator thread, which writes IOContext::should_stop, while this new std::jthread reads the same field here. should_stop is a plain bool and no common lock protects it, so the LIMIT cancellation path has a C++ data race; the watcher may miss the stop and leave Next() blocked until the Flight deadline. The new blocked-Next test performs the same unsynchronized write. This is distinct from the earlier missing-propagation thread: make the lifecycle signal atomic/acquire-release or use a dedicated synchronized stop token, and exercise that production mechanism in the test.
| const auto child_it = file_children.find(to_lower(table_name)); | ||
| if (child_it == file_children.end()) { | ||
| const ParquetColumnSchema* file_child = nullptr; | ||
| for (const auto& child : file_schema.children) { |
There was a problem hiding this comment.
[P2] Keep STRUCT child resolution indexed
This exact-name scan restarts from the first physical child for every projected field, so a full N-field STRUCT now performs roughly N(N+1)/2 comparisons; fallback scans again and recomputes lowercase strings inside the loop. NativeColumnReader::create() rebuilds this mapping for predicate/output readers in every selected Row Group, making wide nested schemas pay the quadratic cost repeatedly. This is distinct from the existing case-collision correctness thread. Build exact-name and normalized unique/ambiguous indices once per physical STRUCT, then preserve the current exact-first semantics through indexed lookups.
| std::async(std::launch::async, [&] { return reader->open(std::move(request)); }); | ||
| ASSERT_TRUE(server.wait_until_entered(std::chrono::seconds(2))); | ||
| state->cancel(Status::Cancelled("cancel blocked Flight DoGet")); | ||
| ASSERT_EQ(open_result.wait_for(std::chrono::milliseconds(750)), std::future_status::ready); |
There was a problem hiding this comment.
[P2] Release the server before a fatal async assertion can return
If this 750 ms assertion fails, GoogleTest returns from the test and destroys open_result before server (reverse declaration order). A std::future from std::async(std::launch::async, ...) waits for its unfinished task, while the server destructor that would release the blocked DoGet cannot run; the only remaining bound is the default 3600-second query/Flight deadline. The fatal assertions at lines 626 and 698 have the same cleanup ordering. Install a scope guard that releases the server before the future is destroyed, or explicitly release/join before reporting each failure.
|
|
||
| const DataTypePtr serde_type = is_dict_filter ? file_type : materialization_type; | ||
| if (_serde_type != serde_type.get() || _dictionary_index_only != is_dict_filter) { | ||
| const bool serde_type_changed = _serde_type != serde_type.get(); |
There was a problem hiding this comment.
[P2] Compare logical SerDe types before dropping the retained dictionary
With TIMESTAMPTZ mapping enabled, native schema parsing already creates a DataTypeTimeStampTz; build_parquet_column_schema() shares that pointer, but apply_timestamp_tz_mapping() replaces it with a newly allocated, semantically equal type. The pruning probe publishes the projected pointer, then every dictionary-ID batch selects the native field pointer here and resets _materialization_state; matched-value materialization switches back and rebuilds the full typed dictionary, repeating each batch. The direct test passes one shared type pointer and misses this production path. Compare semantic type/scale (or avoid remapping an already equal type) and add a multi-batch TIMESTAMPTZ dictionary-filter regression.
TPC-H: Total hot run time: 30086 ms |
TPC-DS: Total hot run time: 178453 ms |
ClickBench: Total hot run time: 25.13 s |
dcc08bf to
665cd8b
Compare
|
run buildall |
|
/review |
TPC-H: Total hot run time: 29677 ms |
TPC-DS: Total hot run time: 177539 ms |
There was a problem hiding this comment.
Automated follow-up review of 665cd8b1a7297ccc0826cd0dec4ac314b3151fc1.
Outcome: no new non-duplicate inline finding. This is a summary-only review, not an approval: the current head still leaves concerns already raised in existing discussions, and reposting those concerns would create duplicate threads.
Existing discussions that remain applicable:
- Compressed Parquet allocation hardening is still codec-incomplete:
validate_compressed_page_size()preflights only Snappy, while variable-width/optional V1 and dictionary paths for other codecs can still reserve the header-declared extent. This is already covered by r3635726309 and r3635726313. - Cancelled Flight
DoGetsetup is detached until its deadline instead of being cancelled at call establishment, even though the Arrow 17 Flight guidance documentsFlightCallOptions::stop_tokenfor that purpose. This remains within r3634830173. The scanner-stop flag read by the watcher is also still an unsynchronized plainbool, as reported in r3635726314. - The watcher still uses a non-stop-aware 25 ms sleep (r3634830183), the new STRUCT resolution remains quadratic for wide schemas (r3635726315), fatal assertions can still strand the async Remote Doris tests (r3635726322), and TIMESTAMPTZ dictionary materialization still keys retained state by pointer identity (r3635726324.
Critical checkpoint conclusions:
- Goal and proof: the patch implements most of the stated FileScannerV2 audit follow-ups and adds focused BE unit tests for the changed branches, but the goal is not fully complete while the existing issues above remain. I reviewed the tests statically; I did not execute them.
- Scope and focus: the 35 changed BE source/test files remain centered on mapper/reader correctness, native Parquet hardening, Remote Doris cancellation, and focused coverage. No unrelated product change was found.
- Concurrency and thread safety: only Remote Doris introduces new concurrency.
PendingOpenstate, Flight stream publication/cancellation, query resource retention, and watcher teardown are otherwise synchronized. The plain-boolscanner-stop race and uncancelled setup RPC remain in the existing threads above; no distinct lock-order or deadlock issue was found. - Lifecycle and memory safety: cached/cold Page V1/V2, dictionary/data pages, direct/ordinary reads, and retained decompression scratch were traced. The pending scratch release occurs only after the old page is exhausted. Detached Flight resource ownership is retained correctly, but the existing cancellation and async-test cleanup issues remain. No new use-after-free or ownership defect was found.
- Configuration: no configuration item or dynamic-reload contract is added.
- Compatibility: no storage, wire, FE-BE, function-symbol, or rolling-upgrade contract changes. The revised legacy LIST/MAP/MAP_KEY_VALUE handling matches the Parquet compatibility rules, and exact field identity now wins before unique case-insensitive fallback.
- Parallel paths: compact/selection delete filtering, ORC/materialized delete execution, cache hit/miss, Page V1/V2, dictionary/plain, direct/selected/ordinary reads, and Iceberg/Hudi/Paimon/JSON/JDBC paths were checked. Production delete predicates always append a fresh result column, so the unchanged parallel erases do not expose a separate reachable input-column deletion bug.
- Special conditions and nullable handling: BOM/empty/EOF line handling, Hive case folding, split reuse, invalid JDBC CAST-to-NULL, empty dictionaries, repeated/optional layouts, malformed level extents, and exact/case-colliding names were checked. No new issue was substantiated.
- Test coverage and results: the changed unit tests target the intended mapper, JSON, JDBC, hybrid-reader, Parquet, and Remote Doris cases. Known codec, production TIMESTAMPTZ, wide-STRUCT, and async cleanup gaps are already in the linked threads. Per the governing review contract, no local build or test was run;
.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protocare absent. - Observability: existing/new reader timers and cumulative condition-cache counters cover the changed paths; no additional metric or tracing gap was found.
- Persistence, transactions, and data writes: not applicable; this patch changes read paths and tests only.
- FE-BE variables: none added or changed.
- Performance: JSON removes a line-sized copy, hybrid batch-size propagation is correct, and Parquet checks are page-level. The remaining wide-STRUCT quadratic lookup and TIMESTAMPTZ rematerialization are already reported.
- Other issues: after the full changed-file sweep and the normal plus risk-focused review passes, no additional non-duplicate finding remained.
- User focus: no additional user-provided review focus was supplied.
ClickBench: Total hot run time: 25.11 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Signed-off-by: Gabriel <liwenqiang@selectdb.com>
Signed-off-by: Gabriel <liwenqiang@selectdb.com>
Signed-off-by: Gabriel <liwenqiang@selectdb.com>
665cd8b to
f9e7a47
Compare
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 28th, 2026 5:08 PM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
TPC-H: Total hot run time: 29619 ms |
TPC-DS: Total hot run time: 178264 ms |
ClickBench: Total hot run time: 25.25 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 28th, 2026 5:08 PM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? This follow-up audits the 10 unresolved review threads on apache#65674 and every child of DORIS-27038. - Three unresolved threads were already fixed on master. - This PR fixes the remaining seven review findings. ### What is changed? - Harden Parquet delta geometry, page allocation validation, schema ambiguity handling, dictionary reuse, decompression scratch lifetime, and one-child MAP_KEY_VALUE SET parsing. - Validate delete expression result ownership before erasing temporary columns. - Scope Iceberg row-lineage virtual columns to Iceberg readers. - Propagate Remote Doris Flight timeout and cancellation. - Reject NULL JDBC special-type casts for non-nullable targets. - Aggregate hybrid Paimon/Hudi condition-cache hit counters. - Remove redundant JSON-line copies and Hive key allocations. The scanner V1 path under `be/src/format` is intentionally untouched. ### Verification - BE ASAN unit tests: 172 tests from 12 related suites passed. - clang-format 16 dry-run passed for every changed C++ file. - `git diff --check` passed. - Confirmed no diff under `be/src/format`. --------- Signed-off-by: Gabriel <liwenqiang@selectdb.com>
## Summary Backport the requested Apache Doris PRs to `branch-4.1` in their master merge order: 1. #63648 2. #64033 3. #64263 4. #64315 5. #63825 6. #65332 7. #65354 8. #65094 9. #65401 10. #65502 11. #65135 12. #65742 13. #65709 14. #65548 15. #65676 16. #65770 17. #65759 18. #65782 19. #65784 20. #65674 21. #65921 22. #65934 23. #65925 24. #65931 #64263, #65354, #65094, #65502, and #65770 are already effectively present on current `branch-4.1`, so this branch does not duplicate those commits. The remaining requested changes are represented by ordered backport commits followed by branch-4.1 compatibility fixes. ## Compatibility notes - Rebased onto the latest `upstream/branch-4.1` and retained both sides of the Iceberg scan conflict resolution. - Keep `be/src/format_v2` and `be/test/format_v2` exactly aligned with current `upstream/master`, as requested. - Apply branch-4.1 compatibility adaptations outside the format_v2 paths. - Preserve the selected PR behavior while avoiding dependencies on unrelated master-only changes. - #65921 supplies the Parquet benchmark scenario header required by `parquet_benchmark_scenarios_test.cpp`, fixing the BE UT compile failure. - Adapt branch-4.1 `VRuntimeFilterWrapper` at the master TableReader boundary so FileScannerV2 residual predicates keep globally rewritten slot indexes. - Keep non-transactional JDBC V2 connections usable by avoiding the unsupported auto-commit transition before Hikari invalidates the proxy. ## FE UT fix - Widen the auto-profile test timing margins to prevent CI scheduling delays from selecting the wrong threshold branch. - Keep the table-filter correctness assertions and use broader performance guardrails suitable for shared FE UT workers. ## Verification - Full local FE+BE build: `Successfully build Doris`. - ASAN BE UT build completed successfully; `test/doris_be_test` linked with exit code 0. - Related BE tests: 382/382 passed across 32 suites, including FileScannerV2, runtime filters, Parquet, Iceberg readers, and format_v2 coverage. - JDBC scanner UT: 3/3 passed; Maven reactor `BUILD SUCCESS`; Checkstyle reports 0 violations. - ClickHouse JDBC V2 regression: 1 suite, 0 failed suites, 0 fatal scripts. - Iceberg branch/tag regression (`iceberg_branch_complex_queries`): 1 suite, 0 failed suites, 0 fatal scripts. - Repository clang-format 16 check: passed for 4,191 files. - `git diff --check`: passed. - `be/src/format_v2` and `be/test/format_v2` have zero diff from current `upstream/master`. --------- Signed-off-by: Gabriel <liwenqiang@selectdb.com> Co-authored-by: daidai <changyuwei@selectdb.com>
3 participants
What problem does this PR solve?
This follow-up audits the 10 unresolved review threads on #65674 and every child of DORIS-27038.
What is changed?
The scanner V1 path under
be/src/formatis intentionally untouched.Verification
git diff --checkpassed.be/src/format.