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.

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

feat: ensure the output order of metric items aligns with their definition order #2222

Open
niebayes opened this issue Aug 22, 2023 · 5 comments
Labels
C-feature Category Features

Comments

@niebayes
Copy link
Contributor

niebayes commented Aug 22, 2023

What problem does the new feature solve?

Let's take the MergeScanMetric as an example. In the code, we have defined four metric items in the order of ready time, first consume time, finish time, and output rows. However, the display order of these items in the output of the explain analyze SQL query doesn't match the definition order. Ensuring their alignment can enhance the clarity of the explain analyze output in certain cases.

image

261644932-f85916d1-1efa-467d-a56c-ccc64f099fc7

What does the feature do?

Ensure the output order of metric items aligns with their definition order.

Implementation challenges

No response

@niebayes niebayes added the C-feature Category Features label Aug 22, 2023
@NiwakaDev
Copy link
Collaborator

I'm interested in this issue, but how do we add some tests for this issue?

I guess that explain analyze tests depends on time.

@niebayes
Copy link
Contributor Author

niebayes commented Sep 17, 2023 via email

@NiwakaDev NiwakaDev self-assigned this Sep 18, 2023
@NiwakaDev
Copy link
Collaborator

@waynexia

This issue happens due to HashMap in the aggregate_by_partition method (datafusion) since elements order in HashMap are non-deterministic:
https://github.com/waynexia/arrow-datafusion/blob/57a66d871fccb58f51588a4f1d30fae8964dc206/datafusion/core/src/physical_plan/metrics/mod.rs#L249-L280

Once I implement a patch, could I submit a PR for this issue to your repository?

@niebayes
Copy link
Contributor Author

@waynexia PTAL

@waynexia
Copy link
Member

Once I implement a patch, could I submit a PR for this issue to your repository?

Please submit to apache/arrow-datafusion. I'll replace my fork with upstream later

@NiwakaDev NiwakaDev removed their assignment Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature Category Features
Projects
None yet
Development

No branches or pull requests

3 participants