Fix select * is not supported for associated query#39131
Open
ClaireLytt wants to merge 1 commit into
Open
Conversation
Contributor
Author
terrymanu
requested changes
Jul 17, 2026
terrymanu
left a comment
Member
There was a problem hiding this comment.
Summary
Review Result: Not Mergeable
Feedback Mode: Change Request
Reason: The implementation direction aligns with the confirmed column-label mapping root cause, but the committed regression test does not exercise the reproduced failing path.
Issues
P1: Commit the actual #28841 reproduction path
- Problem: The added case at
e2e-dql-select-join.xml:136is a plain single-table query. It covers neither the original mixedA.column, B.*join from #28841 nor the later HintManager trigger. The committed runner simply executes this SQL normally atGeneralDQLE2EIT.java:121. Public issue evidence reports that the single-table pattern succeeds without HintManager and fails with it (discussion). The PR’s failing screenshot invokesassertIssue28841WithHintManagerandassertColumnLabelAccessibleAfterNext, but neither method nor equivalent setup exists at the latest head. - Impact: The committed test does not reproduce the demonstrated failure, so the new metadata-map merge behavior can regress while the added case remains green.
- Required Change: Please commit a deterministic regression test that fails at the merge-base and passes with this fix—either the original mixed-projection join and topology from #28841 or the actual HintManager path shown in the public reproduction. Preserve label-based
ResultSetaccess so the assertion verifies the missing-column-label behavior.
Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope:
RELEASE-NOTES.md, JDBC result-set mapping, and the SQL E2E case at latest head938b331dc40e7f21ca6612133cc81b8e9a2fa9ca; PR basemasterat1a7d3e67b3bb628db3f43f6b1f82c1274f5b4983; local merge-base7568693e8ee02637a47d3cca8acbf449797932e5. The local triple-dot file list matched all three GitHub/pulls/39131/filesentries. - Not Reviewed Scope: GitHub Actions and CI; no local database E2E or Maven execution was performed.
- Verification: Latest-head XML validation passed (
xmllint, exit 0). Latest-head source inspection confirmed that no committed HintManager reproduction harness exists. PR metadata, files, commits, issue comments, PR comments, and reviews were fetched through authenticated REST. The GitHub app connector was unavailable, but the fallback supplied every required endpoint, so there is no evidence-access gap. - Release Note / User Docs: The release-note entry is present. Additional user documentation is not required because this fix introduces no configuration, migration, or public API change.
Contributor
Author
|
@terrymanu
SELECT o.user_id, i.* FROM t_order o JOIN t_order_item i ON o.order_id = i.order_id WHERE o.order_id = ?
SELECT T.*, T.status status_new FROM t_order T WHERE T.order_id = 1000with
|
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
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.


Fixes #28841
Changes proposed in this pull request:
Before committing this PR, I'm sure that I have checked the following options:
./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.