[fix](topn) Resolve topn lazy materialization column indexes for aliases#65759
Open
hubgeter wants to merge 1 commit into
Open
[fix](topn) Resolve topn lazy materialization column indexes for aliases#65759hubgeter wants to merge 1 commit into
hubgeter wants to merge 1 commit into
Conversation
Issue Number: None Related PR: None Problem Summary: TopN lazy materialization resolved deferred column indexes with output slot names. Queries that renamed Hive columns therefore produced -1 indexes, and external row-id fetch could fill those columns with NULL when positional reading was used. Resolve the index from the already traced original base column so the descriptor and index share one column identity. Add a focused planner unit test and a Hive ORC Explain regression for the alias path. Fix incorrect NULL values from aliased external-table columns when TopN lazy materialization is used.
hubgeter
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
July 17, 2026 07:46
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
morrySnow
approved these changes
Jul 17, 2026
Contributor
|
add related pr please |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
Gabriel39
approved these changes
Jul 17, 2026
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review summary
Requesting changes for one P1 correctness issue. The single-alias physical-index fix is correct, but repeated aliases of the same lazy external column still create duplicate physical transport names and can crash the default positional Hive ORC phase-two reader. The inline comment contains the concrete trigger and fix constraints.
Key checkpoint conclusions
- Goal and proof: Resolving the ordinal from
originalColumnfixes the reported single-alias-1index, and both added assertions fail before that change. Repeated aliases remain unhandled, so the overall alias-correctness goal is incomplete. - Scope and design: The one-line change is focused and correct for one physical occurrence. The phase-two representation also needs either deduplication plus output fan-out or unique transport identities; any deduplication key must retain relation and access-path identity.
- Concurrency, lifecycle, and configuration: No new locking, lifecycle, static-initialization, or configuration behavior is introduced. The accepted issue is reachable under the existing default
hive_orc_use_column_names=false. - Compatibility and parallel paths: There is no Thrift/protobuf shape change or new rolling-upgrade incompatibility. CatalogRelation/TVF, joins, nested access paths, partition columns, OLAP fetch, Hive name/position modes, and legacy/v2 reader routing were checked; repeated external physical names are the remaining affected boundary.
- Tests: The JUnit assertion and Hive EXPLAIN check are valid pre-fix-failing structural coverage for one alias, but neither executes a duplicate-alias external fetch. An executed ORC/Parquet result regression is needed.
- Persistence, writes, observability, and performance: No persistence, transaction, data-write, or observability path changes. Planning complexity remains one schema lookup per lazy slot, with no distinct material CPU, memory, or I/O regression found.
- Final missed-case review: Alias chains, bare-plus-aliased output, identical alias text, repeated and distinct nested paths, case/schema identity, joins/self-joins, partition fields, external table formats, request/response fan-out, and OLAP row-store selection were rechecked. No distinct finding beyond the inline P1 remained.
- User focus: No additional review focus was provided.
Validation was static only, as required by the review runner; no local build or test was run. At submission time, CheckStyle and the available lightweight checks pass, FE UT and compile are pending, and the TeamCity performance job reports failure.
| outputBuilder.add(outputSlot); | ||
| lazyColumnForRel.add(originalColumn); | ||
| lazyBaseColumnIdxForRel.add(relationTable.getBaseColumnIdxByName(lazySlot.getName())); | ||
| lazyBaseColumnIdxForRel.add(relationTable.getBaseColumnIdxByName(originalColumn.getName())); |
Contributor
There was a problem hiding this comment.
[P1] Handle repeated aliases of one physical lazy column
Project(n1, n2)
TopN(order by id)
Project(name AS n1, name AS n2, id)
HiveScan(id, name)
The aliases have distinct ExprIds, so this loop appends the same originalColumn twice and sends column_descs_lists = [[name, name]] with indices [[1, 1]]. Thrift derives both slot names from the physical Column. In the phase-two positional ORC reader, slot_map consequently has one name entry; the first index erases it and the second dereferences the null inserted by slot_map["name"] (be/src/format/table/hive_reader.cpp:64-83). Parquet has the same sequence at lines 252-272. Thus this valid query can crash even after the single-alias fix. Please deduplicate fetches by relation/base column/access path and fan the value out to both output locations, or preserve unique transport identities, and add an executed duplicate-alias ORC/Parquet regression.
Contributor
|
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. To get authorized for security work, join the Trusted Access for Cyber program: https://chatgpt.com/cyber Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 29512 ms |
Contributor
TPC-DS: Total hot run time: 176582 ms |
Contributor
ClickBench: Total hot run time: 24.8 s |
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Review summary
No new non-duplicate inline findings were identified on head eb21afa2aaccd7e80efae72fdf3eb63ed9032f27. The existing unresolved P1 repeated-alias thread remains the sole blocking issue: the single-alias ordinal fix is correct, but repeated lazy outputs for one physical external column still create duplicate transport identities and can fail in the active legacy positional reader. This review is comment-only to avoid duplicating that landed inline issue.
Key checkpoint conclusions
- Goal and proof: Resolving the ordinal from
originalColumnfixes the reported single-alias-1index. The JUnit assertion and Hive EXPLAIN assertion both fail before the change and pass afterward while proving that lazy materialization is present. The broader alias-correctness goal remains incomplete only because of the existing repeated-alias P1. - Scope and design: The one-line production change is small, clear, and correct for a single physical occurrence. The remaining case needs deduplication plus output fan-out, or unique transport identities, keyed with relation/base-column/access-path identity as described in the existing thread.
- Concurrency, lifecycle, static initialization, and configuration: No new locking, shared mutable state, thread entry, object-lifecycle, static-initialization, or configuration behavior is introduced. Existing request metadata is immutable during concurrent RPC/scanner work.
- Compatibility and parallel paths: No Thrift/protobuf shape, persisted metadata, storage format, or rolling-upgrade contract changes. Catalog and TVF relations, alias chains, joins/self-joins, nested access paths, partition columns, OLAP column/row-store selection, Hive name/position modes, and legacy/v2 reader routing were checked. Duplicate original IDs disable OLAP row-store and safely fall back to distinct-slot column fetch; the remaining live failure is the already-reported legacy external duplicate-identity path.
- Conditions, errors, and FE-BE transport: The physical
Columntraced from the same relation is the correct schema key, andgetBaseColumnIdxByNameresolves it case-insensitively. Descriptor/index/location list cardinality and ordering remain aligned. No new silent error path or unchecked status was introduced. - Tests and results: The added tests are valid structural coverage for one alias, but the Hive addition is EXPLAIN-only and does not execute a duplicate-alias ORC/Parquet fetch; that missing runtime case is already requested by the P1. Current CI shows FE UT, External Regression, Compile, CheckStyle, P0, and performance passing;
check_coverage_feis failing. Local validation was static-only, as required by the review runner. - Persistence, transactions, writes, observability, and performance: No persistence, EditLog, transaction, data-write, or new observability path is involved. The lookup remains the pre-existing linear schema scan per lazy slot, with no distinct CPU, memory, or I/O regression found.
- Final convergence: Two normal full-review slices and one separate risk-focused scan all returned
NO_NEW_VALUABLE_FINDINGSagainst the same ledger, existing thread set, and unchanged head. The main final changed-file and unresolved-candidate sweep found no remaining non-duplicate candidate. - User focus: No additional user-provided review focus was specified.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Problem Summary:
Problem Summary: TopN lazy materialization resolved deferred column indexes with output slot names. Queries that renamed Hive columns therefore produced -1 indexes, and external row-id fetch could fill those columns with NULL when positional reading was used. Resolve the index from the already traced original base column so the descriptor and index share one column identity. Add a focused planner unit test and a Hive ORC Explain regression for the alias path.
Fix incorrect NULL values from aliased external-table columns when TopN lazy materialization is used.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)