Skip to content

Fix incorrect Schema over aggregate function, Remove unnecessary exprlist_to_fields_aggregate - #10408

Merged
alamb merged 1 commit into
apache:mainfrom
jonahgao:fix_aggregate_schema
May 8, 2024
Merged

Fix incorrect Schema over aggregate function, Remove unnecessary exprlist_to_fields_aggregate#10408
alamb merged 1 commit into
apache:mainfrom
jonahgao:fix_aggregate_schema

Conversation

@jonahgao

@jonahgaojonahgao commented May 7, 2024

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes#10346.

Rationale for this change

exprlist_to_fields_aggregate was introduced by #2486.
It was necessary for the test aggregate_with_rollup_with_grouping.

asyncfnaggregate_with_rollup_with_grouping(){
let sql = "SELECT id, state, age, grouping(state), grouping(age), grouping(state) + grouping(age), COUNT(*) \
FROM person GROUP BY id, ROLLUP (state, age)";
let expected = "Projection: #person.id, #person.state, #person.age, #GROUPING(person.state), #GROUPING(person.age), #GROUPING(person.state) + #GROUPING(person.age), #COUNT(UInt8(1))\
\n Aggregate: groupBy=[[#person.id, ROLLUP (#person.state, #person.age)]], aggr=[[GROUPING(#person.state), GROUPING(#person.age), COUNT(UInt8(1))]]\
\n TableScan: person projection=None";
quick_test(sql, expected);
}

In this test, the grouped-by expression person.state was not included in the aggregate's output schema, so we need to further check the aggregate's input, as mentioned in the comments.

// when dealing with aggregate plans we cannot simply look in the aggregate output schema
// because it will contain columns representing complex expressions (such a column named
// #GROUPING(person.state) so in order to resolve person.state in this case we need to
// look at the input to the aggregate instead.

But on the current main branch, person.state is already included in the aggregate's output schema, which is achieved through the function grouping_set_to_exprlist. It adds all the grouped-by expressions.

let grouping_expr:Vec<Expr> = grouping_set_to_exprlist(group_expr.as_slice())?;

Therefore, exprlist_to_fields_aggregate should become unnecessary. Besides, it takes columns from the aggregate's input and might override a column with the same name in the output, leading to the bug in #10346..

What changes are included in this PR?

Remove the unnecessary exprlist_to_fields_aggregate and fix a bug.

Are these changes tested?

Yes

Are there any user-facing changes?

No

@github-actionsgithub-actionsBot added logical-expr Logical plan and expressions core Core DataFusion crate labels May 7, 2024

@alambalamb 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.

It is a very nice PR that fixes a bug by deleting code 👨‍🍳 👌

Thank you very much @jonahgao 🙏

@alambalamb changed the title Remove unnecessary exprlist_to_fields_aggregateFix incorrect Schema over aggregate function, Remove unnecessary exprlist_to_fields_aggregateMay 7, 2024
@alamb

alamb commented May 7, 2024

Copy link
Copy Markdown
Contributor

FYI @timsaucer

@alamb
alamb merged commit d58bae4 into apache:mainMay 8, 2024
@alamb

alamb commented May 8, 2024

Copy link
Copy Markdown
Contributor

Thanks again @jonahgao

@jonahgao
jonahgao deleted the fix_aggregate_schema branch May 8, 2024 22:36
findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

coreCore DataFusion cratelogical-exprLogical plan and expressions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Schema incorrect after select over aggregate function that returns a different type than the input

2 participants

@jonahgao@alamb