Uh oh!
There was an error while loading. Please reload this page.
[fix](load) Fix Arrow stream load with uppercase columns - #65127
Conversation
sollhui
commented
Jul 1, 2026
run buildall |
hello-stephen
commented
Jul 1, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
sollhui
commented
Jul 1, 2026
/review |
sollhui
commented
Jul 1, 2026
/review |
sollhui
commented
Jul 1, 2026
run buildall |
There was a problem hiding this comment.
I found two issues that should be fixed before merge.
Critical checkpoints: the PR's goal is clear and mostly focused on preserving Arrow source-column case in the Nereids load planner, but the implementation does not cover accepted non-lowercase FORMAT AS spellings. No concurrency, lifecycle, persistence, FE/BE protocol, or config-change concerns were introduced by this small FE/load-planning change. I checked the related Nereids stream/broker load flow and the BE Arrow reader name check. Test coverage was added, but the new deterministic regression result should use generated qt_/order_qt_ output instead of direct assertions. I could not run the FE unit or regression tests in this checkout because thirdparty/installed and thirdparty/installed/bin/protoc are missing.
Subagent conclusions: optimizer-rewrite produced MAIN-001 and tests-session-config produced MAIN-002; both were independently verified and accepted for inline comments. Convergence round 1 ended with both subagents replying NO_NEW_VALUABLE_FINDINGS for this final comment set.
User focus: no additional user-provided review focus was present.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Additional changes requested after refreshing the review to head bfa4f6a.
I found two distinct correctness issues that are not covered by the existing inline comments: broker LOAD still re-lowercases Arrow source names through the legacy DataDescription conversion, and fixed partial-update metadata does not use the same case-insensitive column resolution that the new Arrow slot typing now uses.
Critical checkpoints: the PR goal is clear and the change is narrow, but parallel load paths and write-path metadata are incomplete. No new concurrency, lifecycle, config, persistence, or mixed-version protocol surface was introduced by the edited FE planner code. The main risk is data/write correctness for Arrow load planning, especially broker LOAD and MoW fixed partial update. Test coverage was added for stream load, but it does not cover these two paths. I could not run FE unit or regression tests in this checkout because thirdparty/installed and thirdparty/installed/bin/protoc are missing; static diff hygiene via git diff --check passed.
Duplicate-preflight: existing GitHub comments already cover the case-sensitive FORMAT AS ARROW spelling and the regression qt_/order_qt_ output issue, so I did not repeat those.
Subagent conclusions: OR-1 was merged into the broker legacy-conversion inline comment. TSC-1 was dismissed as stale/out of the live PR scope. After the PR head moved, convergence round final-2 ended with both optimizer-rewrite and tests-session-config replying NO_NEW_VALUABLE_FINDINGS for the current ledger/comment set.
User focus: no additional user-provided review focus was present.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Arrow Stream Load through the Nereids load planner lowercased source column names even though Arrow schema field names are case-sensitive. For tables with uppercase target columns, the Arrow scan-slot path could resolve the source name through a case-sensitive type map and fail planning with a null type or construct a scan slot whose name no longer matched the Arrow field. This change preserves source column names for Arrow load format, including non-lowercase Arrow format spellings, and resolves Arrow scan slot types through the target table's case-insensitive column lookup. ### Release note Fix Arrow Stream Load for tables with uppercase column names. ### Check List (For Author) - Test: Checkstyle; Regression test added; Unit test attempted - mvn -pl fe-core -DskipTests checkstyle:check - ./run-fe-ut.sh --run org.apache.doris.nereids.load.NereidsLoadScanProviderTest (not completed locally due existing fe-catalog compilation errors in Column.java/ScalarType before reaching fe-core tests) - ./run-regression-test.sh --run -d load_p0/stream_load -s test_arrow_stream_load_uppercase_column -genOut (not completed locally because no Doris FE was listening on 127.0.0.1:9030) - Behavior changed: Yes. Arrow load source column names are preserved in the Nereids load planner, and Arrow target column types are resolved case-insensitively. - Does this need documentation: No
sollhui
commented
Jul 2, 2026
/review |
sollhui
commented
Jul 2, 2026
run buildall |
There was a problem hiding this comment.
Automated review follow-up for current head e08f8130e5a6df1c2c1aaa932f174f0c56dca1f7.
I found no new valuable inline comments to add without duplicating existing review threads. The current head appears to have addressed the earlier non-lowercase Arrow format spelling issue and the regression-result-framework issue. The two remaining substantiated concerns are already filed as inline threads:
3507270606: broker LOAD still passes preserved Arrow names through legacyDataDescription, which lowercases them before converting back toNereidsBrokerFileGroup.3507270613: fixed partial-update metadata still collects input columns with exact-case matching while Arrow scan-slot type resolution now accepts case-insensitive source names.
Critical checkpoint conclusions:
- Goal and proof: the PR targets Arrow stream load with uppercase column names; direct stream-load paths are covered by the new regression and FE unit coverage, but broker LOAD and fixed partial-update variants remain covered by existing unresolved review threads.
- Scope: implementation is narrow in the Nereids load description/scan-provider path plus focused tests.
- Concurrency/lifecycle: no new concurrency, locking, or lifecycle-sensitive state is introduced.
- Configuration/session/compatibility: no new config items or serialized formats are added; the relevant compatibility risk is the existing legacy broker conversion path.
- Parallel paths: stream load was updated, but broker load and fixed partial-update metadata remain the important parallel paths, already called out inline.
- Tests/results: the current regression uses
order_qt_with an.outfile; I could not run FE or regression tests in this checkout becausethirdparty/installedandthirdparty/installed/bin/protocare absent.
Subagent conclusions: optimizer-rewrite and tests-session-config both completed the final convergence round with NO_NEW_VALUABLE_FINDINGS for the same final ledger/comment set. No subagent candidates became new inline comments; the only live findings were merged as duplicates of existing GitHub threads 3507270606 and 3507270613.
User focus: no additional user-provided review focus was present.
hello-stephen
commented
Jul 2, 2026
TPC-H: Total hot run time: 29684 ms |
hello-stephen
commented
Jul 2, 2026
TPC-DS: Total hot run time: 173665 ms |
hello-stephen
commented
Jul 2, 2026
ClickBench: Total hot run time: 25.3 s |
hello-stephen
commented
Jul 2, 2026
FE Regression Coverage ReportIncrement line coverage |
morrySnow
left a comment
There was a problem hiding this comment.
Code Review — #65127 Fix Arrow stream load with uppercase columns
The fix is functionally correct for the Arrow use case and well-tested. However, the same class of bug exists for the NATIVE format path but was left unfixed, and the approach adds yet another format-specific special case to an already-fragmented codebase. See inline comments for details.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
### What problem does this PR solve? Issue Number: None Related PR: apache#65127 Problem Summary: Native load scan slot planning reused a case-sensitive column-to-type map after source column names had been lowercased. When the target table preserved a different column case, this could create a slot with a null type. The fix resolves Arrow and Native slot types through the matched table column when available, centralizes case-preserving format checks in Util, and applies that behavior to both Nereids and legacy DataDescription conversion paths. ### Release note Fix Native and Arrow load column-name case handling. ### Check List (For Author) - Test: FE validate / Unit Test attempted / Static check - FE validate: mvn -pl fe-core -am validate -DskipTests - Unit Test attempted: ./run-fe-ut.sh --run org.apache.doris.nereids.load.NereidsLoadScanProviderTest, blocked before tests by existing fe-catalog compile errors in Column.java/ScalarType generated-type mismatch - Static check: git diff --check - Behavior changed: Yes, Native load now resolves scan slot types case-insensitively through the matched table column, and legacy load conversion preserves case for case-preserving formats consistently - Does this need documentation: No
sollhui
commented
Jul 6, 2026
/review |
sollhui
commented
Jul 6, 2026
run buildall |
There was a problem hiding this comment.
Automated review for PR 65127 at d930216.
I found one remaining correctness issue and requested changes inline.
Critical checkpoint conclusions:
- Goal/test proof: the PR addresses Arrow source-column case preservation, legacy broker conversion, and Native scan-slot type lookup, with regression and FE unit coverage added. The broker fixed partial-update path still has a correctness gap.
- Scope: the change is mostly focused on FE load/Nereids case handling and the paired tests.
- Concurrency/lifecycle/config/persistence: no new locks, long-lived state, configs, or persisted formats are introduced.
- Parallel paths/compatibility: a parallel broker-load fixed partial-update collector still uses exact-case matching before the new case-insensitive scan-slot resolution.
- Tests: the regression suite now uses order_qt output and has a committed .out file; unit coverage exists for Arrow and Native scan-slot type resolution, but broker Arrow fixed partial update with mixed-case target columns is not covered.
- Validation limits: this checkout is missing .worktree_initialized and thirdparty/installed/bin/protoc, so I did static/GitHub-metadata review only and did not run FE or regression tests.
Subagent conclusions:
- optimizer-rewrite proposed OR-01; I accepted it as M-OR-01 and submitted the broker-load partial-update inline comment.
- tests-session-config proposed TS-1 as a duplicate of existing thread discussion_r3507270613; I suppressed it as already covered.
- Convergence round 1 ended with both live subagents reporting NO_NEW_VALUABLE_FINDINGS for the final ledger/comment set.
User focus: no additional user-provided review focus was present.
Uh oh!
There was an error while loading. Please reload this page.
sollhui
commented
Jul 10, 2026
/review |
sollhui
commented
Jul 10, 2026
run buildall |
PR approved by at least one committer and no changes requested. |
There was a problem hiding this comment.
I reviewed the current head db15a1d3052bdd1dc8aceecbb50aa96f0c991169 against the GitHub PR file list and existing review threads. I did not find a new distinct issue that should be added as another inline comment.
Critical checkpoint conclusions:
- Goal and proof: the current patch preserves Arrow/JSON file-field case where the reader needs exact source names, keeps
columnsFromPathcanonicalized, resolves Arrow/Native scan-slot types through the table column lookup, and applies case-insensitive matching for fixed partial-update input columns. Unit and regression coverage was added for the main Arrow uppercase paths and the path-column canonicalization case. - Scope: the change is focused on load column-name case handling across Nereids, legacy conversion, stream load, broker load, partial update metadata, and the related regression/unit coverage.
- Concurrency/lifecycle: no new concurrency, lock ordering, async lifecycle, or static initialization behavior was introduced in the reviewed paths.
- Config/session compatibility: no new config item was added. Format-name behavior is centralized in
Util.isCasePreservingFormat, and the non-lowercase Arrow format spelling case is covered. - Parallel paths: I checked both Nereids and legacy
DataDescription, stream-load and broker-load partial-update collection, and the BE Arrow/Native reader interactions. The earlier review threads for Arrow spelling, legacy broker relowering, Native null type, partial update, columns-from-path, and result-framework output are addressed in the current patch or already covered by existing comments. - Tests: the new regression uses
order_qt_*with a committed.out, and the FE unit test covers explicit Arrow source case, non-lowercase Arrow format spelling, path-column lowercasing, Arrow/Native type lookup, and import-column case-insensitive matching. I did not rerun FE or regression tests in this review; the local checkout is grafted and lacks the prompt base object, so I used GitHub PR metadata/patch as the authoritative scope. A simple scan of added patch lines found no trailing whitespace. - Persistence/transaction/write behavior: no EditLog/storage-format change was introduced. The fixed partial-update paths now forward canonical table column names after case-insensitive matching, which preserves the downstream BE exact-name partial-update contract.
- Observability/performance: no new observability requirement or obvious hot-path performance issue stood out; the added checks are small planner-time lookups.
User focus: no additional user-provided review focus was supplied.
Subagent conclusions: the optimizer-rewrite and tests-session-config subagents found no new candidate findings in their initial passes. After the main ledger recorded the duplicate/dismissed points and the proposed comment-only final set, convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same ledger/comment set.
hello-stephen
commented
Jul 10, 2026
TPC-H: Total hot run time: 29754 ms |
hello-stephen
commented
Jul 10, 2026
TPC-DS: Total hot run time: 180727 ms |
hello-stephen
commented
Jul 10, 2026
ClickBench: Total hot run time: 25.04 s |
Uh oh!
There was an error while loading. Please reload this page.
…pache#65617) pick apache#65127 (cherry picked from commit d3bda5e)
…pache#65617) pick apache#65127 (cherry picked from commit d3bda5e)
…pache#65617) pick apache#65127 (cherry picked from commit d3bda5e)
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Arrow Stream Load through the Nereids load planner lowercased source column names even though Arrow schema field names are case-sensitive. For tables with uppercase target columns, the Arrow scan-slot path could resolve the source name through a case-sensitive type map and fail planning with a null type or construct a scan slot whose name no longer matched the Arrow field. This change preserves source column names for Arrow load format, including non-lowercase Arrow format spellings, and resolves Arrow scan slot types through the target table's case-insensitive column lookup.
Release note
Fix Arrow Stream Load for tables with uppercase column names.
Check List (For Author)