Skip to content

[Enhancement](load) Add LoadTableReader for FileScannerV2 ingestion - #69320

Merged
Gabriel39 merged 6 commits into
apache:branch-4.2from
Gabriel39:dev/file-scanner-v2-load-4.2
Oct 26, 2026
Merged

Gabriel39 merged 6 commits into
apache:branch-4.2from
Gabriel39:dev/file-scanner-v2-load-4.2

Conversation

@Gabriel39

@Gabriel39 Gabriel39 commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Traditional file loads with source/destination tuples always select FileScanner V1. Add LoadTableReader as a TableReader subclass so supported loads can use FileScannerV2 without FE or Thrift changes.

LoadTableReader::finalize_chunk() uses base source materialization, then applies per-record defaults, source pre-filters, destination expressions, strict-mode conversion checks, nullability checks, and flexible partial-update skip bitmaps and sequence-column marks. customize_file_scan_request() enables malformed-record rejection. The generic TableReader owns file and split lifecycle, with no load-specific flag, branches, or presence-mask storage. Scanner continues to evaluate destination conjuncts before ordinary output projections. Source readers receive no destination predicate, aggregate pushdown, or condition-cache snapshot. Missing physical source aliases fail before reader open unless the FE default map declares a fallback; an explicit NULL default remains valid, and path columns are exempt.

CSV/JSON readers reject malformed load records with row rollback and error accounting. JSON reports field-presence masks separately from NULL values so missing defaults and partial updates retain their meaning. Empty outer arrays and array elements with missing or non-object configured roots count as rejected load records so the existing filter-ratio quality gate sees them. Valid siblings remain intact. Fully rejected batches do not end a split. Non-line JSON streams consume each pipe message until pipe EOF, including Kafka routine loads; chunked HTTP buffers remain one document and regular file ranges retain single-document behavior.

Supported load formats select V2 independently of the query session option: the Thrift default does not distinguish an omitted option from false in load plans. Query selection remains unchanged. Avro and Arrow IPC stream loads keep their existing path; the current V2 Arrow implementation only supports remote-Doris Flight scans. Load ranges keep their FE split boundaries.

This PR targets branch-4.2 and is independent of #69312.

Validation

  • Passed 28 targeted tests in a local ASAN/UBSAN harness, linking rebuilt changed translation units against existing project libraries. Coverage includes strict/non-strict conversion, pre-filter order, explicit NULL versus missing defaults, non-nullability, flexible updates and sequence marks, rejected-batch EOF, scanner selection, real CSV/JSON readers through the load reader, missing physical sources versus explicit NULL defaults and path columns, and ten JSON pipe cases: multiple Kafka messages, batch boundaries, malformed messages, fixed-length HTTP, chunked HTTP, empty pipe EOF, empty outer arrays, missing/null roots, valid siblings around invalid roots, and fully rejected root messages followed by valid messages. The harness also executes two existing scanner-selector scenarios against the production selector.
  • Recompiled the changed BE implementation units and syntax-checked the affected existing scanner, CSV, JSON, and TableReader test suites.
  • Passed clang-format 16 checks for all 17 affected C++ source/header files and git diff --check.
  • Added parser unit tests and a Stream Load regression covering target-column WHERE, rejected/unselected counts, JSON defaults, flexible partial updates, and rejection of empty arrays or invalid configured roots with max_filter_ratio=0 versus 1. Full BE build and cluster regression execution remain for CI; they were not run locally.

Release note

Route supported traditional file loads through FileScannerV2 with a table reader for load conversion and validation.

Check List (For Author)

  • Test
    • Regression test
    • Unit Test
  • Behavior changed:
    • Yes. Supported traditional loads select V2 independently of the query-only session switch.
  • Does this need documentation?
    • Yes. Updated the in-repository FileScannerV2 design document.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@Gabriel39
Gabriel39 requested a review from yiguolei as a code owner October 20, 2026 04:27
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes for one verified P1: enabling V2 for JSON routine loads exposes finite-document EOF handling on a multi-message Kafka pipe. Only the first message is read, while a successful task can commit offsets for unread messages; larger tasks can block when the unread pipe fills. The inline finding explains the production FE, reader and offset path.

Reviewed all 22 changed paths and relevant upstream/downstream code. The adapter is focused at the table-reader layer and reuses the existing source/destination protocol. Critical-checkpoint conclusions:

  • Data correctness, mapping and filtering: source defaults remain distinct from explicit NULL; direct-source strict/nullability checks match V1; source prefilters precede destination conversion and destination WHERE. Production key/auto-increment/UID metadata, flexible-update skip bitmaps and sequence marks were checked. Internal storage visibility/version and persisted delete-bitmap mechanisms are unchanged. The verified data-loss exception is the Kafka EOF issue above.
  • Concurrency, lifecycle and memory: no new threads, locks or cross-translation-unit statics. Wrapper/child ownership, expression cleanup, split abort/close, rejected-only batches and scanner-local counters were traced. The stream lifecycle defect is covered by the finding; no additional verified ownership or locking defect was found.
  • Configuration, compatibility and parallel paths: no new configuration, FE/BE variable, Thrift/protobuf definition or storage format. Query selection, V1 comparison, CSV/TEXT/PROTO, native Parquet/ORC source mapping and typed WAL query replay were checked; Avro/Arrow load exclusions remain explicit. Mandatory JSON routing requires the streaming fix.
  • Errors, writes and persistence: malformed-record rollback and fatal Status propagation were checked. Sink transaction, EditLog and failover protocols are unchanged, but the reader defect violates their source-consumption assumption before the existing offset commit.
  • Tests, results, performance and observability: new BE/regression cases and fixture expectations were reviewed statically, including strict conversion, WHERE counts, defaults/NULL and flexible updates. Rejected/unselected counters and error logging remain available. Dedicated adapter conversion/prefilter timing and performance measurements are absent; no unverified performance claim is posted.

No additional user focus was supplied. Existing inline threads were empty, so there are no existing blocking IDs or duplicate findings. All candidates and risk focuses have evidence-based dispositions, and no changed path or correctness question remains unreviewed. The static review converged after two rounds with all three reviewers returning NO_NEW_VALUABLE_FINDINGS on the unchanged final finding.

Validation limits: no builds, tests, benchmarks or cluster execution were run, as required by the review contract. New tests omit a multi-message Kafka pipe case; broker path columns and actual Parquet/ORC loads also lack new integration coverage. These execution/coverage limits remain despite static-review convergence.

Review converged after 2 round(s).

@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three P2 findings should be addressed: missing Parquet/ORC source aliases can become accepted NULLs, empty outer JSON arrays bypass rejected-row accounting and max_filter_ratio, and an active ORC-load selector test retains the old expectation. All three are inline.

The existing P1 discussion 4321532533 is fixed at this head: non-line JSON streams consume successive pipe messages until pipe EOF, with chunked HTTP remaining one document. It is not reposted or carried as an existing blocker. No additional user focus was supplied.

Reviewed all 22 changed paths at 439f82b using the supplied aggregate diff and required guides. The traced production path is FE source tuple -> physical FileReader -> mapper/TableReader source materialization -> LoadTableReader defaults or skip marks -> source prefilter -> destination conversion -> Scanner target predicates -> existing sink.

  1. Goal and proof: the table-layer ingestion adapter and supported-format routing are implemented; source validation and error-accounting gaps are the first two findings. Added unit/regression cases were inspected, not executed.
  2. Scope: changes are focused on the adapter, scanner hooks, physical rejection/presence metadata, tests and design documentation.
  3. Concurrency: parser/expression state and new counters are scanner-local; existing pipe queue locks coordinate producers/consumers. No new thread, lock order or shared counter race was established.
  4. Lifecycle and memory: the physical reader remains alive through finalization, per-batch masks track surviving source rows, const columns are materialized before row checks, and split/EOF/close paths were traced. No new ownership cycle or static-initialization dependency was found.
  5. Configuration: no configuration is added. Traditional-load selection intentionally ignores the query-only V2 option, as documented.
  6. Compatibility: existing FE source/default semantics were compared against V1; the missing-alias error-contract regression is reported. No persisted or wire-format change was found.
  7. Shared definitions: no Thrift/protobuf definition changed, so cross-release field-ID comparison is not applicable.
  8. Parallel paths: CSV/TEXT/PROTO, JSON line/file/Kafka/fixed/chunked HTTP, present Parquet/ORC fields, query routing and query-style WAL replay were traced. Avro/Arrow loads retain their existing selection.
  9. Special conditions: load rejection, source-predicate suppression, missing/default versus explicit NULL, flexible-update key/sequence marks and unsplit load ranges were checked. Empty-array rejection is the reported exception.
  10. Coverage limits: no builds, tests, submodule initialization or source edits were performed. Full scanner/cluster ingestion, compressed/PROTO/TEXT matrices, native flexible-update JSON with sequence, Parquet/ORC load interoperability and concurrent backpressure were not executed. No performance benchmark was run.
  11. Expected results: inspected fixture rows and explicit load counts are consistent for covered cases; the remaining stale selector assertion is reported separately.
  12. Observability: filtered and unselected rows remain distinct, existing profiling is retained, and physical errors use load error logs. The missing empty-array rejection count/log is reported.
  13. Transactions and persistence: no EditLog, commit, visible-version or persisted delete-bitmap protocol changes are present; the Kafka consumption/EOF boundary was specifically checked.
  14. Writes and failures: existing sink transactionality is unchanged; the source-side validation regressions can admit values or loads V1 rejected. No further crash/leak defect was substantiated by static tracing.
  15. FE/BE variables: the existing tuple, default, strict-mode, key/UID and sequence-map protocol is reused; no new wire variable is introduced.
  16. Performance: expression contexts persist, presence metadata is batch-scoped, and no additional proven hot-path regression was identified. This is not a runtime performance validation.
  17. Other risks: every candidate and initial risk has an accepted or evidence-based dismissed disposition; no unresolved static-review candidate or unreviewed changed path remains.

Review converged after 3 rounds. All three final reviewers returned NO_NEW_VALUABLE_FINDINGS on the unchanged three-comment set; all changed paths, risk focuses and candidate dispositions are complete. The runtime validation limits above remain explicit.

Review converged after 3 round(s).

Comment thread be/src/format_v2/table/load_reader.cpp Resolved
Comment thread be/src/format_v2/json/json_reader.cpp Resolved
@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One new P2 finding: configured JSON-root extraction silently drops invalid elements from a nonempty outer array without rejection accounting, allowing max_filter_ratio=0 to succeed. The inline comment gives the reachable input and fix direction.

Reviewed all 23 changed paths and their FE planning, mapper, scanner, parser, sink-quality-gate and WAL interactions at 2f46771. No additional user focus was supplied.

Checkpoint conclusions:

  • Goal and scope: LoadTableReader provides the intended source/destination adapter at the table-reader layer; the changes remain focused on ingestion integration, rejection/presence metadata, counters, tests and documentation. The new finding is an exception to the required load error semantics.
  • Concurrency: no new threads or locks; reader state and new counters remain scanner-local, with existing synchronized pipe producers.
  • Lifecycle: source/default/destination expressions and physical readers retain clear ownership; abort, close, EOF and all-rejected batches were traced. No new cross-translation-unit static initialization dependency.
  • Configuration: none added; supported loads intentionally select V2 independently of the query option.
  • Compatibility: existing FE tuple fields are reused; no Thrift/protobuf definitions, storage formats or serialized WAL layouts change. Actual WAL recovery remains on the specialized TVF query path.
  • Parallel paths: query selection, native per-range formats, CSV/PROTO/TEXT and Avro/Arrow fallback were compared. Loads retain FE split boundaries and suppress source predicate, aggregate and condition-cache pushdown.
  • Special checks: default-map membership, path NULLs, omitted keys, auto-increment and sequence skip marks were checked against real FE descriptors and V1 behavior.
  • Tests: added unit/regression cases cover strict/non-strict conversion, pre-filter order, defaults versus NULL, flexible updates, malformed records, pipe messages and empty/rejected batches. Configured-root rejection is a missing negative case.
  • Expected results: all four fixtures and regression assertions were reconciled statically; no .out results changed.
  • Observability: filtered and unselected counts use delta publication; the inline finding identifies the remaining quality-gate accounting gap.
  • Persistence/failover: no new metadata/EditLog state or replay protocol is introduced.
  • Writes/atomicity/crashes: validated output continues through existing sinks and transaction/visibility handling; no new commit mechanism or persistent ownership was added.
  • FE/BE propagation: source/default/strict/required-slot metadata uses existing transport fields and reaches the adapter.
  • Performance/memory: setup stays per scanner/split; presence/default work is batch-local. No additional substantiated leak, unbounded retained allocation or hot-path regression was found.

All four existing threads were independently revalidated: routine-message EOF, absent physical aliases, the original no-jsonpaths empty-array rejection and ORC selector expectation are fixed on this head. No prior P0/P1 remains applicable, and none was reposted. Empty arrays with jsonpaths match V1 behavior and were dismissed as a new regression.

Converged after 2 rounds: all three latest-round reviewers returned NO_NEW_VALUABLE_FINDINGS on the unchanged finding set, coverage is complete, and all risk/candidate dispositions are resolved. Validation limits: static analysis only; no builds or tests were run. Full BE/cluster execution, external-writer Parquet/ORC load parity and actual routine-load offset commits remain experimentally unverified.

Review converged after 2 round(s).

Comment thread be/src/format_v2/json/json_reader.cpp Resolved
@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all 24 changed paths at a73b750. No new actionable finding was verified. Two complete-review agents and a separate risk-focused agent returned NO_NEW_VALUABLE_FINDINGS; all candidates and initial risks have evidence-based dispositions.

The existing routine-load EOF finding (4321532533), absent-source-alias finding (4321638591), empty-array accounting finding (4321638593), remaining ORC selector expectation (4321638595), and configured-root accounting finding (4321731092) were independently revalidated as fixed at this head. None is reposted; no existing P0/P1 remains applicable.

Critical checkpoint conclusions:

  1. Goal and evidence: LoadTableReader adapts the existing source/destination tuple protocol to V2. Traced FE source construction through mapper/materialization, conversion, Scanner destination filtering, sink validation and load totals. Added unit, real-pipe and regression tests exercise this behavior; they were inspected, not executed.
  2. Scope: the changes remain focused on ingestion adaptation, format selection, physical record recovery, shared counters, tests and design documentation.
  3. Concurrency: no new worker, lock or lock order. Source blocks, expression contexts and load counters are scanner-owned; physical decode and finalization update counters sequentially. Existing pipe synchronization remains in place.
  4. Lifecycle: physical readers remain alive through finalization, presence metadata is consumed before close, output owns its columns, and abort/close release per-split state. Fully rejected batches do not manufacture EOF. No new static initialization dependency was introduced.
  5. Configuration: no new configuration. Supported loads select by format independently of the query-only V2 option, as documented and tested; query selection retains its existing rules.
  6. Compatibility: no persisted format or FE/BE wire-layout change. Source tuple/default/direct-mapping contracts remain in use; WAL replay retains its query/WalTableReader route and stable unique-id mapping.
  7. Serialized definitions: no Thrift/protobuf definitions changed, so a release-branch field-id comparison is not applicable.
  8. Parallel paths: examined CSV/compressed CSV, JSON, TEXT, PROTO, Parquet/ORC conversion, WAL routing and generic query-reader finalization. Avro/Arrow loads retain their existing path; loads retain FE split boundaries.
  9. Conditional semantics: verified strict direct-source conversion, NOT NULL rejection, absent default-map membership versus explicit NULL defaults, per-record missing versus NULL, path null normalization, flexible-update missing keys/value/sequence marks, and source-prefilter versus destination-filter ordering.
  10. Test coverage: inspected positive/negative conversion, defaults, nullability, prefilter, flexible updates, rejected batches, split progression, selector and stream-message cases. New physical ingestion integration tests concentrate on CSV/JSON; Parquet/ORC/WAL migration lacks a new end-to-end load matrix.
  11. Expected results: checked all five fixtures against loaded/rejected/unselected counts, ordered SQL results and max_filter_ratio success/failure assertions. No generated .out file changed. Build/test source globs include the new files.
  12. Observability: error-file logging and separate rejected/unselected counters propagate to Scanner, RuntimeState and the existing quality gate; no new distributed service requires instrumentation.
  13. Transactions and persistence: the adapter supplies destination blocks to existing sink/writer protocols; journal, recovery, visible-version and delete-bitmap persistence protocols are unchanged.
  14. Data writes and failure handling: conversion/rejection occurs before sink consumption. Nonrecoverable I/O, cancellation and logging errors propagate as failures. Flexible-update marks retain the existing sink contract; no new transaction or crash-recovery state was introduced.
  15. FE/BE variables: no new transmitted variable or sending point.
  16. Performance and memory: inspected allocation/copying, per-row validation/default-mask work, retained block ownership and pruning boundaries; no concrete new performance or lifetime defect was substantiated. No benchmark was run.
  17. Other invariants: table/global versus file/local identity translation remains mapper-owned. Load destination predicates, aggregates and condition-cache state are excluded from source readers. Base materialization retains shape/nullability checks before the load-specific hook changes schema or row counts.

User focus: no additional focus was supplied. Complete PR coverage and the mandatory ingestion/lifecycle checkpoints were applied.

Limits: static review only, with no builds, test execution, source edits or submodule initialization. Author-reported test runs were not independently reproduced. Runtime/cluster integration and performance remain unverified; no unresolved actionable candidate or unreviewed changed path remains.

Review converged after 1 round(s).

@@ -145,6 +145,29 @@ bool FileScanLocalState::TEST_should_use_file_scanner_v2(const TQueryOptions& qu
bool FileScanLocalState::should_use_file_scanner_v2(const TQueryOptions& query_options,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我们为什么还会有这个方法,如果scanner v2 已经能完全替代v1了,感觉不应该有这个方法了

Update CSV encoding, CSV field-count, and JSON no-match error assertions
for the reader selected by this PR. Preserve load status, rejected-row
counts and the exact JSON source record checks. Cover the following JSON
alias check that was hidden by the earlier assertion failure as well.

Validation: four Groovy assertion replays fail before the change and pass
afterward, using recorded CI responses for the three reported failures
and the reader-defined diagnostic for the subsequent alias check. Groovy
syntax checks, clang-format 16 for the PR's 17 C++ files, and diff checks
pass. Full cluster regression execution remains for CI.
@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@Gabriel39

Copy link
Copy Markdown
Contributor Author

Fixed the three new regression failures from builds 1074659 and 1074663:

  • test_csv_with_none_utf8_data: expect the V2 UTF-8 data-quality diagnostic; retain status and rejected-row checks.
  • test_stream_load_error_url: verify the V2 diagnostic reports the exact expected and actual field counts (3 and 9).
  • test_json_load: expect V2's distinct diagnostics for unmatched JSON paths and unmatched object aliases; retain exact source-record checks. The second check was previously hidden by the first failure.

Validation: the four affected Groovy assertions fail before the update and pass afterward in local assertion replays. The three reported cases use their recorded CI responses; the subsequent alias case uses the diagnostic from the reader's no-match branch. All three Groovy files pass syntax checks, and all 17 C++ files in this PR pass clang-format 16. Full cluster regressions have not been rerun locally; CI has been requested again.

Gabriel39 added a commit to Gabriel39/incubator-doris that referenced this pull request Oct 20, 2026
Apply the regression assertion fixes from apache#69320 to its master port.
Update UTF-8, physical field-count and JSON no-match diagnostics while
preserving status, rejected-row and exact JSON source-record checks.

Validation: all four affected Groovy assertion replays, all three suite
syntax checks, repository clang-format 16 check, and diff checks passed.
Full cluster regression execution remains for CI.

(cherry picked from commit 7e29433)

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static review of PR #69320 at 7e29433 found no new actionable defect. The main review and three independent reviewers covered all 27 paths in the supplied aggregate diff. The proposed inline set is empty.

Existing review context: independently verified that the prior JSON routine-load EOF P1 (4321532533), missing physical alias validation (4321638591), empty-array rejection count (4321638593), remaining ORC selector expectation (4321638595), and configured-root rejection count (4321731092) have been addressed in the inspected code. No existing P0/P1 remains confirmed applicable, so existing_blocking_comment_ids is empty. These findings are not reposted.

User focus: no additional focus was supplied. On the existing selector-design question (4322248373), the selection method still serves a purpose: Avro and Arrow IPC loads retain V1, and query selection still depends on its option and capability checks.

Critical checkpoints:

  1. Goal and behavior: LoadTableReader adapts existing FE source/destination tuples to V2 and preserves source materialization, defaults, prefilters, conversion/strict/null checks and destination WHERE order. Focused tests exercise these stages by inspection.
  2. Scope and layering: changes stay in scanner dispatch, a table-level load adapter, and neutral malformed-row/presence/counter contracts; mapper remains the source/file identity bridge.
  3. Concurrency and locks: readers, expressions, masks and counters are scanner-owned; no new thread, lock order, or shared-counter concurrency was introduced.
  4. Lifecycle and statics: physical reader and presence masks survive source finalization; output ColumnPtrs survive source clear; abort/EOF/close paths and expression ownership were traced. No new static initialization.
  5. Configuration: no new config; supported loads intentionally select by format when query options are omitted.
  6. Compatibility: FE load representation and wire/storage formats are unchanged; unsupported load formats keep their previous path.
  7. Thrift/protobuf: no definition changes, so cross-branch field-ID comparison is inapplicable.
  8. Parallel paths: traditional loads, query/TVF scans, CSV/JSON/TEXT/PROTO and WAL recovery were traced; load split refinement, aggregate and condition-cache pushdown are disabled where required.
  9. Special conditions: comments explain pipe-versus-file EOF, omitted query options, source default membership and unchanged load split boundaries; no unexplained correctness exception was confirmed.
  10. Test coverage: reviewed adapter/native-reader/pipe tests and stream-load regressions for strict conversion, NULL/default distinctions, partial updates, counts, multiple messages and fully rejected batches. End-to-end Parquet/ORC/PROTO/WAL execution coverage is less direct than the synthetic adapter coverage.
  11. Changed results: selector expectations and UTF-8/field-count/JSON-path diagnostics match the inspected production paths; no result file was hand-edited.
  12. Observability: malformed/conversion failures use the existing error-log channel; separate filtered/unselected counters publish scanner-local deltas to the quality gate.
  13. Transactions and persistence: no EditLog, rowset-version, commit/publish, or persisted WAL format change; existing transaction sink remains authoritative.
  14. Data writes and crashes: adapter emits destination blocks without owning commit side effects; cleanup, cancellation and EOF were checked, with no new leak/deadlock or atomicity defect established.
  15. FE-to-BE variables: no new transmitted field. Verified real FE required slots, typed defaults, flexible-update UID/key/auto-increment metadata and sequence propagation.
  16. Performance: expression setup remains outside row loops, and no avoidable I/O/asymptotic regression was confirmed. Runtime throughput or allocation parity is not established by static inspection.
  17. Additional checks: physical masks use LocalColumnId and accepted-row coordinates; explicit NULL remains distinct from absent fields, and source predicates cannot desynchronize masks before materialization.

Validation limits: this invocation was static only as required; no builds, tests, source edits or submodule initialization were performed. Test pass status and performance are not independently verified. These limits do not represent an unresolved code candidate.

Convergence: converged after 1 review round. All three assigned reviewers returned NO_NEW_VALUABLE_FINDINGS on the unchanged empty inline set; complete changed-path coverage and explicit R1/R2/R3 risk dispositions are recorded in the shared ledger. No unresolved code candidate remains.

Review converged after 1 round(s).

ADBC and Fluss do not support ordinary file loads. Reject their table
markers before checking the physical format, since the default Parquet
format otherwise admits them to LoadTableReader.

Add shared selector coverage for an omitted option and default format,
both option values and explicit physical formats. Query selection stays
on the existing connector-specific path.

Validation: the new regression fails before the fix; 31 targeted tests
pass in the branch-4.2 ASAN/UBSAN harness afterward, including the two CI
failure assertions. Master operator and relevant test translation units
pass syntax checks. clang-format 16, build hygiene and diff checks pass.
Full BE UT execution remains for CI.
@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@Gabriel39

Copy link
Copy Markdown
Contributor Author

Fixed the two BE UT failures reported by build 1074847. The load selector previously accepted the default Parquet format before considering the ADBC/Fluss table marker. These connectors support queries only, so the load gate now rejects their markers before the physical-format switch. Query routing is unchanged.

Added a shared regression covering both connectors with the default format/omitted option, both option values, and explicit physical formats. The same change is applied to #69320 and #69333.

Validation:

  • The new regression fails before the fix and passes afterward.
  • All 31 targeted tests pass in the branch-4.2 ASAN/UBSAN harness, linking rebuilt changed units against existing project libraries. This includes the assertions from both reported CI failures and the existing load conversion/JSON pipe tests.
  • Master's operator, shared regression, and the original ADBC/Fluss test translation units pass syntax checks using its own UT build configuration.
  • clang-format 16 checks pass on both branches; master's build-hygiene and diff checks pass.

The complete master BE UT suite was not run locally; CI has been requested again.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static review of d5061bd completed across all 27 changed paths. No new actionable finding was substantiated. Review converged in one round: both full-scope reviewers and the dedicated source-materialization risk reviewer returned NO_NEW_VALUABLE_FINDINGS, and the final sweep closed every candidate and risk focus.

Critical checkpoint conclusions:

  • Goal: supported traditional loads now use FileScannerV2 through LoadTableReader, preserving the FE source/destination tuple contract. Added tests cover conversion, rejection, defaults, partial updates and real pipe lifecycle; their runtime results were not independently executed.
  • Scope and design: the change is focused on load dispatch and conversion/validation. Scanner retains query/split integration, TableReader owns load semantics, and FileReaders own physical parsing and presence metadata.
  • Concurrency and locks: added masks and counters are scanner/reader-local and consumed serially; no new thread, lock, lock-order or heavy locked operation was introduced.
  • Lifecycle and static initialization: presence masks are consumed before reader close; EOF-with-rows, fully rejected batches, subsequent splits and idempotent cleanup were checked. No new cross-translation-unit initialization dependency was added.
  • Configuration: no new configuration item; supported loads intentionally select by format independently of the query-only V2 switch.
  • Compatibility: no Thrift/protobuf definition, persisted format or FE/BE field changed. Existing descriptor/default/direct-source metadata is reused; Arrow/Avro loads retain their existing path. Standard WAL replay uses the unchanged query/WalTableReader route.
  • Parallel paths and conditions: CSV variants, PROTO, TEXT, JSON line/file/message/chunked modes, native typed sources, partition/default columns and flexible updates were traced. Destination predicates remain after conversion; source readers receive no load destination predicates, aggregate pushdown or condition-cache snapshot.
  • Testing: new unit tests, fixtures and Groovy assertions were inspected, including strict/non-strict conversion, explicit NULL versus missing defaults, missing aliases, partial-update keys/sequence marks, rejected/unselected counts and max_filter_ratio. No generated .out result changed. Builds, tests, sanitizers and cluster regressions were not run under the static-only contract.
  • Observability and errors: checked Status propagation, error-log calls and separate filtered/unselected counters through the quality gate; no concrete new observability defect was found.
  • Transactions and persistence: no edit-log, transaction commit/publish, persistent metadata or delete-bitmap version change; existing sink/storage ownership remains in place. No crash/failover execution is claimed.
  • Performance and memory: no substantiated new asymptotic or lifetime defect was identified. Runtime throughput and allocation behavior remain unmeasured.

Existing context: independently confirmed that the routine-load EOF P1 (4321532533) is fixed on this head, so no existing blocking ID is carried forward. The previously reported missing-source alias, no-jsonpaths empty array, invalid configured roots and ORC test expectation are also fixed on their reported paths; no duplicate comments are posted. The selector remains necessary for formats/connectors outside this adapter's support. No additional user focus was supplied.

Coverage limits: this is a static review, with no independent runtime or differential external-writer/encoding/performance matrix. No unresolved actionable candidate or unreviewed changed path remains.

Review converged after 1 round(s).

@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 83.63% (419/501) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 64.30% (29897/46498)
Line Coverage 49.13% (315361/641896)
Region Coverage 44.96% (253929/564747)
Branch Coverage 46.62% (119866/257128)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 91.62% (459/501) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 75.98% (34312/45159)
Line Coverage 60.73% (386590/636539)
Region Coverage 57.62% (325937/565670)
Branch Coverage 58.46% (150145/256844)

@Gabriel39
Gabriel39 merged commit 9ace7cc into apache:branch-4.2 Oct 26, 2026
31 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

PR approved by anyone and no changes requested.

@github-actions github-actions Bot added the approved Indicates a PR has been approved by one committer. label Oct 26, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by one committer. reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants