Minor: Add comment on input_schema from AggregateExec by viirya · Pull Request #7727 · apache/datafusion

viirya · 2023-10-03T02:57:10Z

Which issue does this PR close?

None

Rationale for this change

As we integrate AggregateExec, one thing confusing me is why AggregateExec keeps a separate input_schema which is not actually schema of input operator, but is always the schema of input of aggregation. In other words, partial and final aggregation operators have the same input schema parameter. It doesn't look consistent on input_schema and input functions on the operator and other operators. And another look at this input_schema parameter, it doesn't look like there is any meaningful usage of it. It causes a bit strange when we integrate Spark with DataFusion's aggregation operator.

I've tried to remove it first in this patch. It is confirmed that input_schema is not used during query execution as all unit tests and end-to-end tests passed without it.

However, verify benchmark results pipeline has some failures. After looking into it, I found that the parameter is used when generating physical plan from protobuf nodes. When doing that, it needs input schema of partial aggregation to initiate aggregate expressions.

I changed this patch to add a few comment on it to help me and others to understand it in the future.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

viirya · 2023-10-03T06:30:23Z

Interesting. All tests are passed but verify benchmark results has failure.

viirya · 2023-10-03T08:12:34Z

Ah, I see. It is used when generating physical plan from proto nodes. When doing that, it needs input schema of partial aggregation to initiate aggregate expressions.

viirya · 2023-10-03T17:19:18Z

For the above reason, I'm not sure if it's possible to remove the parameter now. Let me think about it more.

This reverts commit 5096883.

viirya · 2023-10-04T16:04:59Z

cc @alamb

alamb · 2023-10-04T18:32:52Z

 /// Input schema before any aggregation is applied. For partial aggregate this will be the
 /// same as input.schema() but for the final aggregate it will be the same as the input
- /// to the partial aggregate
+ /// to the partial aggregate, i.e., partial and final aggregates have same `input_schema`.


🤔 I went to double check this (being inspired by the thoroughness of your own reviews @viirya ) and I was able to remove the entire field: #7741

alamb

I think this change is fine, but I think #7741 would be even better

viirya · 2023-10-04T19:20:08Z

I am far from a laptop but I tried to remove it at the first and the schema seems necessary when deserializing from protobuf to physical aggregate plan as aggregate expression initialization needs it.

…

On Wed, Oct 4, 2023, 11:33 Andrew Lamb ***@***.***> wrote: ***@***.**** approved this pull request. I think this change is fine, but I think #7741 <#7741> would be even better — Reply to this email directly, view it on GitHub <#7727 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAQZ56ZIYNPBHN4YQUSAP3X5WTXRAVCNFSM6AAAAAA5QJUG7CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMNJYGI2TIMBQGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

viirya · 2023-10-04T19:25:08Z

I left a comment above to explain it and where/why it is failed in the CI pipeline.

…

On Wed, Oct 4, 2023, 12:19 L. C. Hsieh ***@***.***> wrote: I am far from a laptop but I tried to remove it at the first and the schema seems necessary when deserializing from protobuf to physical aggregate plan as aggregate expression initialization needs it. On Wed, Oct 4, 2023, 11:33 Andrew Lamb ***@***.***> wrote: > ***@***.**** approved this pull request. > > I think this change is fine, but I think #7741 > <#7741> would be even > better > > — > Reply to this email directly, view it on GitHub > <#7727 (review)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAAQZ56ZIYNPBHN4YQUSAP3X5WTXRAVCNFSM6AAAAAA5QJUG7CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMNJYGI2TIMBQGQ> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

viirya · 2023-10-04T20:04:44Z

@alamb You can see my previous commits which removed the input_schema field. In CI, all unit tests and end-to-end tests can pass but verify benchmark results gets failures when deserializing protobuf to physical aggregate.

It is because during initializing aggregate expressions for final aggregate, their constructors all needs their input data type from input schema of partial aggregate (i.e., input schema before aggregate). That is why input_schema is there.

alamb · 2023-10-04T20:26:51Z

@alamb You can see my previous commits which removed the input_schema field. In CI, all unit tests and end-to-end tests can pass but verify benchmark results gets failures when deserializing protobuf to physical aggregate.
It is because during initializing aggregate expressions for final aggregate, their constructors all needs their input data type from input schema of partial aggregate (i.e., input schema before aggregate). That is why input_schema is there.

Sorry @viirya -- I should have looked at your comments more closely.

viirya · 2023-10-04T20:40:04Z

No worries @alamb . Thanks for approval. 😄

I guess it is still possible to remove it, if we embed the data type info in to individual aggregate expression protobuf node. But this will involve a lot of changes just for removing the input_schema. 😄 So I stopped there and turned to add a few comment to explain why the parameter is there.

alamb · 2023-10-04T21:19:24Z

I guess it is still possible to remove it, if we embed the data type info in to individual aggregate expression protobuf node. But this will involve a lot of changes just for removing the input_schema. 😄 So I stopped there and turned to add a few comment to explain why the parameter is there.

Makes sense. I was thinking we could effectively recover the intermediate schema somehow (though I not quite sure how)

viirya · 2023-10-06T20:12:32Z

Thanks @alamb. I merged this first. We may investigate if it is possible to remove it completely later.

* Remove input_schema * Revert "Remove input_schema" This reverts commit 5096883. * Add comment

viirya marked this pull request as draft October 3, 2023 02:57

github-actionsBot added the core Core DataFusion crate label Oct 3, 2023

viirya changed the title ~~Remove input_schema~~Remove input_schema from AggregateExecOct 3, 2023

viirya added 3 commits October 3, 2023 23:04

Remove input_schema
5096883

Revert "Remove input_schema"
c06693b
This reverts commit 5096883.

Add comment
070c5a2

viirya changed the title ~~Remove input_schema from AggregateExec~~Minor: Add comment on input_schema from AggregateExecOct 4, 2023

viirya marked this pull request as ready for review October 4, 2023 06:18

viirya force-pushed the remove_input_schema branch from 88eee3b to 070c5a2Compare October 4, 2023 06:19

github-actionsBot removed the core Core DataFusion crate label Oct 4, 2023

alamb mentioned this pull request Oct 4, 2023
Remove unused AggregateExec::input_schema#7741
Closed

alamb reviewed Oct 4, 2023
View reviewed changes

alamb approved these changes Oct 4, 2023
View reviewed changes

viirya merged commit a8c01de into apache:mainOct 6, 2023

Ted-Jiang pushed a commit to Ted-Jiang/arrow-datafusion that referenced this pull request Oct 7, 2023
Minor: Add comment on input_schema from AggregateExec (apache#7727)
1be2b74
* Remove input_schema * Revert "Remove input_schema" This reverts commit 5096883. * Add comment

matthewgapp mentioned this pull request Jan 11, 2024
matt/feat/recursive ctes/config flag matthewgapp/arrow-datafusion#3
Closed

Navigation Menu

Uh oh!

Minor: Add comment on input_schema from AggregateExec - #7727

Minor: Add comment on input_schema from AggregateExec#7727
viirya merged 3 commits into
apache:mainfrom
viirya:remove_input_schema

viirya commented Oct 3, 2023•
edited
Loading

Uh oh!

viirya commented Oct 3, 2023

Uh oh!

viirya commented Oct 3, 2023

Uh oh!

viirya commented Oct 3, 2023

Uh oh!

viirya commented Oct 4, 2023

Uh oh!

alambOct 4, 2023

Uh oh!

alamb left a comment

Uh oh!

viirya commented Oct 4, 2023 via email

Uh oh!

viirya commented Oct 4, 2023 via email

Uh oh!

viirya commented Oct 4, 2023

Uh oh!

alamb commented Oct 4, 2023

Uh oh!

viirya commented Oct 4, 2023

Uh oh!

alamb commented Oct 4, 2023

Uh oh!

viirya commented Oct 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

viirya commented Oct 3, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

viirya commented Oct 3, 2023

Uh oh!

viirya commented Oct 3, 2023

Uh oh!

viirya commented Oct 3, 2023

Uh oh!

viirya commented Oct 4, 2023

Uh oh!

alambOct 4, 2023

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

viirya commented Oct 4, 2023 via email

Uh oh!

viirya commented Oct 4, 2023 via email

Uh oh!

viirya commented Oct 4, 2023

Uh oh!

alamb commented Oct 4, 2023

Uh oh!

viirya commented Oct 4, 2023

Uh oh!

alamb commented Oct 4, 2023

Uh oh!

viirya commented Oct 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

viirya commented Oct 3, 2023•
edited
Loading