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

ARROW-12402: [Rust] [DataFusion] Implement SQL metrics example #10049

Closed
wants to merge 1 commit into from

Conversation

andygrove
Copy link
Member

This introduces a new method on ExecutionPlan to be able to access generic metrics from any physical operator.

One metric is implemented to demonstrate usage.

@andygrove
Copy link
Member Author

@alamb @jorgecarleitao @Dandandan @returnString Let me know what you think. It is really hard to debug distributed queries without any metrics 😉

@andygrove
Copy link
Member Author

@edrevo fyi

Copy link
Contributor

@Dandandan Dandandan left a comment

Choose a reason for hiding this comment

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

looks great! Indeed great to have for debugging / optimizing DataFusion and ballista!

@github-actions
Copy link

Copy link
Contributor

@returnString returnString left a comment

Choose a reason for hiding this comment

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

Looks good to me, nice simple API and no constraints imposed on consumers :)

One potential user-friendliness thing: would it be worth having SQLMetric use atomics internally to avoid the Mutex<SQLMetric> that'll probably be required for most usage? Almost certainly makes no difference from a perf perspective, the contention will be pretty much non-existent, just thinking about the API.

@kszucs kszucs closed this in ec6436e Apr 15, 2021
@alamb
Copy link
Contributor

alamb commented Apr 15, 2021

One potential user-friendliness thing: would it be worth having SQLMetric use atomics internally to avoid the Mutex that'll probably be required for most usage? Almost certainly makes no difference from a perf perspective, the contention will be pretty much non-existent, just thinking about the API.

I think this would be a good idea @returnString FYI. Perhaps part of a subsequent PR

GeorgeAp pushed a commit to sirensolutions/arrow that referenced this pull request Jun 7, 2021
This introduces a new method on `ExecutionPlan` to be able to access generic metrics from any physical operator.

One metric is implemented to demonstrate usage.

Closes apache#10049 from andygrove/ARROW-12402

Authored-by: Andy Grove <andygrove73@gmail.com>
Signed-off-by: Krisztián Szűcs <szucs.krisztian@gmail.com>
michalursa pushed a commit to michalursa/arrow that referenced this pull request Jun 10, 2021
This introduces a new method on `ExecutionPlan` to be able to access generic metrics from any physical operator.

One metric is implemented to demonstrate usage.

Closes apache#10049 from andygrove/ARROW-12402

Authored-by: Andy Grove <andygrove73@gmail.com>
Signed-off-by: Krisztián Szűcs <szucs.krisztian@gmail.com>
michalursa pushed a commit to michalursa/arrow that referenced this pull request Jun 13, 2021
This introduces a new method on `ExecutionPlan` to be able to access generic metrics from any physical operator.

One metric is implemented to demonstrate usage.

Closes apache#10049 from andygrove/ARROW-12402

Authored-by: Andy Grove <andygrove73@gmail.com>
Signed-off-by: Krisztián Szűcs <szucs.krisztian@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants