-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixing Column aliases in outer join queries #15384
Conversation
…in aliasing Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only keep the SimpleProjection when the query has column with alias otherwise we should remove this if possible.
The new changes seem to [always/most of the time] keep it.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15384 +/- ##
==========================================
- Coverage 67.41% 65.44% -1.98%
==========================================
Files 1560 1562 +2
Lines 192752 193937 +1185
==========================================
- Hits 129952 126923 -3029
- Misses 62800 67014 +4214 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
var colName *sqlparser.ColName | ||
var alias sqlparser.IdentifierCI | ||
if withQualifier { | ||
if e.needsQualifier { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Signed-off-by: Florent Poinsard <[email protected]>
Signed-off-by: Manan Gupta <[email protected]> Signed-off-by: Florent Poinsard <[email protected]> Co-authored-by: Florent Poinsard <[email protected]>
I would like to ask if this fix can be reverse-merged into v19 version? |
v19 is not affected by this issue (EDIT: for the same reason). |
@GrahamCampbell Thanks for your reply! CREATE TABLE `item1` (
`i_id` int NOT NULL,
`i_im_id` int DEFAULT NULL,
`i_name` varchar(24) DEFAULT NULL,
`i_price` decimal(5,2) DEFAULT NULL,
`i_data` varchar(50) DEFAULT NULL,
PRIMARY KEY (`i_id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci;
select a.i_name as name1,b.i_name as name2,b.i_id as id2, c.i_name as name3
from item1 a inner join item2 b on a.i_id = b.i_im_id
LEFT OUTER JOIN item3 c on a.i_id = c.i_id
where a.i_id < 10 limit 1;
|
I would like to ask if this fix can be reverse-merged into v19 version? Please help confirm. |
v19 is affected by this issue, I also am encountering the same for v19.0.8-mysql80 🙇 @GrahamCampbell |
Signed-off-by: Manan Gupta <[email protected]> Signed-off-by: Florent Poinsard <[email protected]> Co-authored-by: Florent Poinsard <[email protected]> Signed-off-by: Harshit Gangal <[email protected]>
…17418) Signed-off-by: Manan Gupta <[email protected]> Signed-off-by: Florent Poinsard <[email protected]> Signed-off-by: Harshit Gangal <[email protected]> Co-authored-by: Manan Gupta <[email protected]> Co-authored-by: Florent Poinsard <[email protected]>
@GrahamCampbell, @chhan-coupang , and @nonbb this was backported to the v19 release branch here: #17418 Thanks! |
Description
This PR fixes the issue specified in #15383.
As the issue describes, the problem occurred when we had column aliasing on top of an outer join query. While investigating, it was noticed that we aren't pushing the alias down for outer join queries, and eventually were also getting rid of the
Projection
operator altogether, causing the aliases to be lost.The fix in this PR entails 2 things -
Related Issue(s)
Checklist
Deployment Notes