Skip to content

[planbuilder] sort expression aliases always referenced#2885

Merged
max-hoffman merged 2 commits intomainfrom
max/sort-alias-idx-bug
Mar 11, 2025
Merged

[planbuilder] sort expression aliases always referenced#2885
max-hoffman merged 2 commits intomainfrom
max/sort-alias-idx-bug

Conversation

@max-hoffman
Copy link
Copy Markdown
Contributor

@max-hoffman max-hoffman commented Mar 11, 2025

This fixes a bug where a sort expression alias computed in the lower projection fails to index the nested expression.

Below, the first plan's sort searches for c5:6 in the child, but only finds a:7. The second plan fixes the correctness issue. Obviously there are more desirable projection organizations that version two, but this is small enough of an edge case that I think rewriting projection management with proper expression interning would be overkill right now. The rest of the plan tests look OK/improvements.

select distinct abs(c5) as a from one_pk where c2 in (1,11,31) order by a

before: 

Sort(abs(one_pk.c5:6)->a:7 ASC nullsFirst)
 └─ Distinct
     └─ Project
         ├─ columns: [abs(one_pk.c5:1)->a:0]
         └─ Filter
             ├─ HashIn
             │   ├─ one_pk.c2:0
             │   └─ TUPLE(1 (tinyint), 11 (tinyint), 31 (tinyint))
             └─ ProcessTable
                 └─ Table
                     ├─ name: one_pk
                     └─ columns: [c2 c5]

after:

Distinct
 └─ Project
     ├─ columns: [abs(one_pk.c5:5)->a:0]
     └─ Sort(a:6 ASC nullsFirst)
         └─ Project
             ├─ columns: [one_pk.pk:0!null, one_pk.c1:1, one_pk.c2:2, one_pk.c3:3, one_pk.c4:4, one_pk.c5:5, abs(one_pk.c5:5)->a:0]
             └─ Filter
                 ├─ HashIn
                 │   ├─ one_pk.c2:2
                 │   └─ TUPLE(1 (tinyint), 11 (tinyint), 31 (tinyint))
                 └─ ProcessTable
                     └─ Table
                         ├─ name: one_pk
                         └─ columns: [pk c1 c2 c3 c4 c5]

Copy link
Copy Markdown
Contributor

@jycor jycor left a comment

Choose a reason for hiding this comment

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

LGTM

@max-hoffman max-hoffman merged commit be58f51 into main Mar 11, 2025
8 checks passed
@max-hoffman max-hoffman deleted the max/sort-alias-idx-bug branch March 11, 2025 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants