Skip to content

fix: order by expr rewrite fix - #14486

Merged
comphead merged 4 commits into
apache:mainfrom
akoshchiy:14459-order-by-expr-rewrite-fix
Feb 6, 2025
Merged

fix: order by expr rewrite fix#14486
comphead merged 4 commits into
apache:mainfrom
akoshchiy:14459-order-by-expr-rewrite-fix

Conversation

@akoshchiy

@akoshchiy akoshchiy commented Feb 4, 2025

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #14459.

What changes are included in this PR?

Added the search for a columnized expressions in the children projection nodes.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

@github-actions github-actions Bot added sql SQL Planner logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) labels Feb 4, 2025
@akoshchiy
akoshchiy marked this pull request as ready for review February 5, 2025 18:48
@berkaysynnada

Copy link
Copy Markdown
Contributor

I checked the newly added tests and they were failing before 👍🏻

@berkaysynnada berkaysynnada left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @akoshchiy, the fix is LGTM.

@comphead comphead left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @akoshchiy

Double checked the similar query works in duckDB

D select a from (select 1 a, 2 b) group by a order by sum(b);
┌───────┐
│   a   │
│ int32 │
├───────┤
│     1 │
└───────┘

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logical-expr Logical plan and expressions sql SQL Planner sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ORDER BY with compound expr is not working

3 participants