Repository navigation
[branch-4.2] Backport external data and FileScannerV2 updates - #69722
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: workspace routing discovery unauthorized (401) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
|
Codex automated review failed and did not complete. Error: workspace routing discovery unauthorized (401) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Reader option resolution now merges catalog defaults, but four existing PaimonExternalTable tests construct real-method mocks without initializing their catalog. Construct table spies with an empty catalog and database so the tests exercise the existing snapshot and projection assertions. Related PR: apache#69722 Validation: - Reproduced all four catalog-null errors locally before the fix. - 87 tests passed across PaimonExternalTableTest, PaimonScanParamsTest, PaimonReaderOptionsTest and PaimonHistoricalSchemaOptionsTest. - FE Checkstyle and git diff --check passed.
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: workspace routing discovery unauthorized (401) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: This content was flagged for possible cybersecurity risk. If this seems wrong, try rephrasing your request. If you’re doing authorized security work that requires more cyber permissive safeguards, apply for Daybreak access via https://platform.openai.com/settings/organization/status-and-access before retrying. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Apply the outstanding changes in master merge order: apache#69545 7d16729 apache#69551 7224a2d apache#69612 61d1aef apache#69615 8d2bd1e apache#69650 3dc7341 apache#69651 c00ecb1 apache#69646 e493c61 apache#69668 7865646 apache#69614 already exists on branch-4.2 through apache#69644. Adapt datasource APIs and tests to the release branch, retaining its existing scanner dispatch and statement-bound metadata contracts.
Reader option resolution now merges catalog defaults, but four existing PaimonExternalTable tests construct real-method mocks without initializing their catalog. Construct table spies with an empty catalog and database so the tests exercise the existing snapshot and projection assertions. Related PR: apache#69722 Validation: - Reproduced all four catalog-null errors locally before the fix. - 87 tests passed across PaimonExternalTableTest, PaimonScanParamsTest, PaimonReaderOptionsTest and PaimonHistoricalSchemaOptionsTest. - FE Checkstyle and git diff --check passed.
Keep the selected snapshot and schema together through relation projection, including readers with SELECT permission only. Read all buckets for legacy append schemas without explicit bucket keys instead of rejecting valid historical metadata or pruning files using incomplete hash information. Default FileScannerV2 to enabled in Thrift, matching the session default, and retain explicit disabling for legacy query scans.
bc1836e to
acfd04b
Compare
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Static review of PR #69722 at acfd04bc8cbf1c97ee0b87f810de46413700602c found two P2 defects: a valid NULL outer STRUCT containing native Variant can abort Arrow ingestion, and historical Paimon restoration can redirect a privilege-wrapped fallback branch to main and omit its rows. Both are reported inline. There were no existing inline threads or additional user-provided focus points.
All 81 changed paths were reviewed from the authoritative aggregate diff, with BE and FE complete reviews and an independent review of the suspicious mechanisms. Critical checkpoint conclusions:
- Goal and evidence: the backports implement the intended scanner/external-data changes, with the two inline exceptions; unit and regression assertions were inspected, not executed.
- Scope: the bundle is broad, but each mechanism and branch adaptation is related to its stated backport.
- Concurrency: Arrow state is scanner-local, IPC decompression stays on that thread, and TopN bounds use locked, monotonic snapshots. No new lock-order or shared-mutation defect was found.
- Lifecycle: IPC objects release before their memory pool; schema profiles outlive readers; Iceberg footer collection follows successful close. No new static-initialization dependency was found.
- Configuration: no new configuration item; the existing scanner option defaults to enabled and preserves explicit overrides for ordinary query scans. Load dispatch remains format based.
- Compatibility: optional split offsets tolerate old/empty fragments and are ignored for ORC; native/JNI and legacy scanner boundaries were traced.
- Shared definitions: complete touched Thrift definitions were compared on HEAD, master and branches 4.2/4.1/4.0/3.1. No new reused ID, conflicting meaning or required field was found; unrelated older collisions predate this PR.
- Parallel paths: load/query Arrow, Paimon native/JNI/system/fallback, ES v1/v2 and Parquet/ORC writer paths were checked. The privilege-wrapped fallback exception is inline.
- Conditions: row-ID and safe-conjunct ordering fences, unsupported statistics/type fallbacks, NULL-first handling and equality ties preserve TopN semantics. CSV/JSON schema probing preserves the legacy header, root/path and sampled-pipe behavior.
- Tests: positive and negative additions cover many conversion, ownership, protocol, snapshot and residual cases. The two reported paths need targeted regressions. The new multi-row-group/dictionary split-offset helper has no callers, so that intended coverage is inactive.
- Results: golden rows agree with explicit suite fixtures by static inspection; their generation and runtime correctness were not verified. Disabled known-bug tests remain skipped evidence.
- Observability: Arrow read/materialization timers and row accounting are present; TopN reuses pruning/read counters. No new excessive logging was found.
- Transactions and persistence: no EditLog, visible-version, MoW bitmap or failover protocol change. Iceberg manifest split metadata follows the existing commit path.
- Writes and atomicity: native Variant validates into owned temporary data before publishing values; destination defaults/filtering/error accounting remain in LoadTableReader. The ancestor-NULL defect is inline.
- FE/BE propagation: the split-offset producer, optional wire field, both FE builder paths and manifest consumer were traced; scanner defaults and explicit stream-load selection agree.
- Performance: scheduling adds per-file sorting to obtain earlier TopN bounds; IPC normalization is once per batch with tracked Arrow allocations. No runtime performance claim is made.
- Remaining concerns: candidate and risk dispositions are recorded in the shared ledger; no unresolved source-review question or unread changed path remains after the final review round.
Validation limits: this was static-only. No builds, tests, benchmarks, external-service runs, source edits or submodule initialization were performed. ES9 live behavior, the external writer/backend matrix, concurrency stress, golden generation and the inactive split-offset test matrix remain runtime/coverage limits. The design document's older statement that Arrow loads stay on V1 is stale relative to this change; actual code governed the review.
Review converged after 3 round(s).
| _file_slots[id.value()]->type())); | ||
| // Bind differing numeric widths before concrete SerDe array casts. Timestamp units | ||
| // stay on the wire even inside containers, so Doris retains its own rounding semantics. | ||
| if (!array->type()->Equals(binding)) { |
There was a problem hiding this comment.
[P2] Propagate ancestor NULLs even when the binding type is unchanged
For an IPC column matching STRUCT<payload: VARIANT>, Arrow StructBuilder::AppendNull() produces a NULL outer struct with a valid hidden child carrier containing empty metadata/value bytes. This branch skips masking because the types match, but DataTypeStructSerDe still reads every child and the new Variant SerDe rejects the hidden empty metadata, aborting a valid load instead of loading SQL NULL. Propagate ancestor validity before SerDe decoding independently of the cast decision, and cover a NULL outer STRUCT<VARIANT> with unchanged wire types.
| // Chain groups have their own option filtering and are not ordinary fallback readers. | ||
| return table.copy(dynamicOptions); | ||
| } | ||
| if (table instanceof FallbackReadFileStoreTable) { |
There was a problem hiding this comment.
[P2] Handle fallback pairs hidden by privilege wrappers
A privilege-enabled catalog can return PrivilegedFileStoreTable(FallbackReadFileStoreTable), so this check misses a real handle. For a SELECT+INSERT principal reading a main snapshot older than its current schema, restoreHistoricalSchema calls table.copy(TableSchema), broadcasting the main schema/options to both branches. Normal main options omit branch; the SDK only restores the other branch when that key is present, so the fallback table now defaults to main too. The later copy(Map) does not repair it: a partition present only in the backup branch disappears from the historical query. Traverse the privilege delegate while preserving authorization and restore each fallback branch independently.
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
What problem does this PR solve?
Backport the requested external-data and FileScannerV2 changes to
branch-4.2in their actual master merge order:2104b7509cda); not applied twiceThe eight outstanding changes are combined in one commit. This branch retains the 4.2 FE datasource APIs instead of importing master's connector-module refactor.
Backport adaptations
_file/_poscolumns. The inventory labels upstream-only evidence explicitly.Validation
A full BE rebuild and SQL/external-service regressions were not run locally. Live ES 9 tests require their opt-in endpoint. CI is requested for the combined backport.
Release note
Fix Paimon reads after field renames, external-catalog SHOW DATABASES authorization, and ES compound residual filtering. Support ES 9.x node discovery, publish Iceberg Parquet split offsets, improve Parquet V2 TopN row-group scheduling, and use FileScannerV2 for Arrow loads and text/JSON schema inference.
Check List (For Author)
Follow-up fixes
enable_file_scanner_v2default totrue, matching the session default. Preserve explicitfalsefor legacy query scans and extend the existing load-selection test.Follow-up validation: 169 targeted Paimon FE tests passed with zero failures/errors/skips. FE production compilation and Checkstyle passed. Both changed C++ test translation units passed compiler syntax checks and clang-format 16. Regenerated Java/C++ defaults are enabled; a Java 8-targeted protocol check verifies constructor/clear defaults, an omitted wire field, and an explicit-false round trip. Complete BE CI and live external SQL suites remain pending.
The two new reproductions first failed with the same missing-bucket-key and renamed-sequence-field errors as CI. A separate read-only-principal reproduction also failed before the permission correction. Complete external regression execution remains pending CI.
BE unit-test compatibility
branch-4.2: accept binary storage and the Parquet Variant extension, validate metadata and payloads before appending, and retain owned data after Arrow buffers are released. Allow that ingestion path through the load reader while preserving query-format restrictions and source-default validation.Validation: all 13 previously failing BE tests passed. The expanded run passed all 309 tests across nine suites, including Arrow/JSON loads, load validation, Iceberg, table mapping, and Variant input/output. The focused executable rebuilds the affected readers and SerDe against cached 4.2 build dependencies; this is not a complete BE rebuild. All seven changed C++ translation units passed compiler checks with current headers/generated Thrift, and all 33 affected C/C++ files passed clang-format 16. The complete staged diff, whitespace, and sensitive-information checks passed. Full CI is requested with
run buildalland/review.