Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: fix index bug and add test to check it #7124

Merged
merged 3 commits into from
Jul 28, 2023
Merged

fix: fix index bug and add test to check it #7124

merged 3 commits into from
Jul 28, 2023

Conversation

mustafasrepo
Copy link
Contributor

@mustafasrepo mustafasrepo commented Jul 28, 2023 •
edited
Loading

Which issue does this PR close?

Closes #7118.

Rationale for this change

Thanks @alamb and @crepererum for filing the issue.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added physical-expr Changes to the physical-expr crates core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Jul 28, 2023
@mustafasrepo mustafasrepo changed the title Fix bug, add test [bug], [minor]: Fix bug, add test Jul 28, 2023
@jackwener jackwener changed the title [bug], [minor]: Fix bug, add test fix: fix index bug and add test to check it Jul 28, 2023
@jonahgao
Copy link
Member

Hi, @mustafasrepo
Where is the variable orderings used? I seem to only see updates to it, or maybe I missed something.
https://github.com/apache/arrow-datafusion/blob/350d4ca8a895a53f51935741ee35e5b79120829c/datafusion/physical-expr/src/aggregate/first_last.rs#L161

@mustafasrepo
Copy link
Contributor Author

Hi, @mustafasrepo Where is the variable orderings used? I seem to only see updates to it, or maybe I missed something.

https://github.com/apache/arrow-datafusion/blob/350d4ca8a895a53f51935741ee35e5b79120829c/datafusion/physical-expr/src/aggregate/first_last.rs#L161

Aggregate::Final or Aggregate::FinalPartitioned can require its ordering according to values inside orderings. Hence, these stages can get their values with appropriate ordering. However, I will change the implementation. Hopefully, in the new design, usage of it will be much more explicit. And merge_batch implementation will not rely on outside mechanism during merging.

@jonahgao
Copy link
Member

@mustafasrepo Thank you for explaining 👍

alamb
alamb approved these changes Jul 28, 2023
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @mustafasrepo -- lightning fast turnaround

@mustafasrepo
Add new test
62b4faa
@mustafasrepo
Merge branch 'apache_main' into bug_fix/first_last_multi_partition
e4b4ede 
# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@alamb alamb merged commit 55930fb into apache:main Jul 28, 2023
@alamb
Copy link
Contributor

alamb commented Jul 28, 2023

Thanks again @mustafasrepo

@mustafasrepo mustafasrepo deleted the bug_fix/first_last_multi_partition branch July 31, 2023 07:41
waynexia pushed a commit to waynexia/arrow-datafusion that referenced this pull request Oct 8, 2023
@mustafasrepo @waynexia
fix: fix index bug and add test to check it (apache#7124)
b6f3b28 
* Fix bug, add test

* Add new test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

@jackwener jackwener jackwener approved these changes

Assignees
No one assigned
Labels
core Core DataFusion crate physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Milestone
No milestone
4 participants
@mustafasrepo @jonahgao @alamb @jackwener