[opt](lance) reduce redundant work in LanceTableReader - #68335
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Static review of PR #68335 at head c64d2d7 (base 70bf96e).
Two actionable regressions are called out inline:
- Nested registered-extension arrays: the new normalizer compacts a sliced parent before unwrapping extension children, so Arrow 24 cannot build list/struct parents containing ExtensionType children; the existing sliced nested JSON regression path fails.
- Row-ID/take_rows lifecycle: scanner profile counters are now lazy, so scanner-less phase-two fetches skip Lance data-cache byte statistics that were previously created and collected.
Review checkpoints:
- All ten authoritative changed paths were swept, including the converter extraction, nested normalization, scanner projection, generated search-result columns, global row IDs, runtime-filter/schema gating, close/reset paths, and test changes.
- Projection, global-rowid, runtime-filter, normal scanner reuse, build integration, and other candidate concerns were independently checked; no additional actionable issue was substantiated.
- Existing inline review comments were empty at the final duplicate-fence check, and the live base/head matched the authoritative bundle.
- No user-provided focus points were supplied beyond the complete-review requirement.
- This is a static review only: no builds or tests were run, so author/CI claims are not independent validation.
There was a problem hiding this comment.
Static review of PR #68335 at head c64d2d7 (base 70bf96e).
Two actionable regressions are called out inline:
- Nested registered-extension arrays: the new normalizer compacts a sliced parent before unwrapping extension children, so Arrow 24 cannot build list/struct parents containing ExtensionType children; the existing sliced nested JSON regression path fails.
- Row-ID/take_rows lifecycle: scanner profile counters are now lazy, so scanner-less phase-two fetches skip Lance data-cache byte statistics that were previously created and collected.
Review checkpoints:
- All ten authoritative changed paths were swept, including the converter extraction, nested normalization, scanner projection, generated search-result columns, global row IDs, runtime-filter/schema gating, close/reset paths, and test changes.
- Projection, global-rowid, runtime-filter, normal scanner reuse, build integration, and other candidate concerns were independently checked; no additional actionable issue was substantiated.
- Existing inline review comments were empty at the final duplicate-fence check, and the live base/head matched the authoritative bundle.
- No user-provided focus points were supplied beyond the complete-review requirement.
- This is a static review only: no builds or tests were run, so author/CI claims are not independent validation.
There was a problem hiding this comment.
Static review of PR #68335 at head c64d2d7 (base 70bf96e).
Two actionable regressions are called out inline:
- Nested registered-extension arrays: the new normalizer compacts a sliced parent before unwrapping extension children, so Arrow 24 cannot build list/struct parents containing ExtensionType children; the existing sliced nested JSON regression path fails.
- Row-ID/take_rows lifecycle: scanner profile counters are now lazy, so scanner-less phase-two fetches skip Lance data-cache byte statistics that were previously created and collected.
Review checkpoints:
- All ten authoritative changed paths were swept, including the converter extraction, nested normalization, scanner projection, generated search-result columns, global row IDs, runtime-filter/schema gating, close/reset paths, and test changes.
- Projection, global-rowid, runtime-filter, normal scanner reuse, build integration, and other candidate concerns were independently checked; no additional actionable issue was substantiated.
- Existing inline review comments were empty at the final duplicate-fence check, and the live base/head matched the authoritative bundle.
- No user-provided focus points were supplied beyond the complete-review requirement.
- This is a static review only: no builds or tests were run, so author/CI claims are not independent validation.
|
run buildall |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
PR approved by at least one committer and no changes requested. |
### What problem does this PR solve?
Problem Summary:
optimize LanceTableReader by lazily initializing schemas and scanner
metrics, reusing the record batch converter, caching Arrow normalization
plans, and avoiding unnecessary full-column reads. Also improve code
structure by extracting schema import and record batch conversion logic.
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [ ] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [x] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [ ] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [ ] No.
- [ ] Yes. <!-- Add document PR link here. eg:
apache/doris-website#1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR
should merge into -->
What problem does this PR solve?
Problem Summary:
optimize LanceTableReader by lazily initializing schemas and scanner metrics, reusing the record batch converter, caching Arrow normalization plans, and avoiding unnecessary full-column reads. Also improve code structure by extracting schema import and record batch conversion logic.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)