You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/04/21 17:00:33 UTC
[GitHub] [arrow-datafusion] returnString opened a new pull request #25: Use atomics for SQLMetric implementation, remove unused name field
returnString opened a new pull request #25:
URL: https://github.com/apache/arrow-datafusion/pull/25
As mentioned in apache/arrow#10049 as a potential followup, this PR simplifies metrics management in physical ops by removing the need for a mutex around `SQLMetric` instances, instead using atomics to track metric values.
I've also removed the `name` field as this info is duplicated by `ExecutionPlan::metrics` returning a string-keyed map. Happy to revert that portion of the change if people feel strongly about it though!
It might be possible to futher simplify this by removing the `Arc` allocs and sharing `SQLMetric` instances as `Sync` references if the existing lifetime setup plays nicely with it, but I didn't want to spend _too_ much time digging into this; I think this PR as-is gives us the immediate win of not needing to deal with mutexes :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove commented on pull request #25: Use atomics for SQLMetric implementation, remove unused name field
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #25:
URL: https://github.com/apache/arrow-datafusion/pull/25#issuecomment-824483334
Closes https://github.com/apache/arrow-datafusion/issues/30
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] codecov-commenter commented on pull request #25: Use atomics for SQLMetric implementation, remove unused name field
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #25:
URL: https://github.com/apache/arrow-datafusion/pull/25#issuecomment-824223463
# [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/25?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#25](https://codecov.io/gh/apache/arrow-datafusion/pull/25?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (613caaf) into [master](https://codecov.io/gh/apache/arrow-datafusion/commit/c365a4f59d16d39cf27b19fd2bf34a27d590db4d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c365a4f) will **decrease** coverage by `0.01%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head 613caaf differs from pull request most recent head 0a4d560. Consider uploading reports for the commit 0a4d560 to get more accurate results
[](https://codecov.io/gh/apache/arrow-datafusion/pull/25?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #25 +/- ##
==========================================
- Coverage 70.41% 70.39% -0.02%
==========================================
Files 123 123
Lines 21261 21255 -6
==========================================
- Hits 14970 14963 -7
- Misses 6291 6292 +1
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-datafusion/pull/25?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [datafusion/src/physical\_plan/hash\_aggregate.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/25/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9oYXNoX2FnZ3JlZ2F0ZS5ycw==) | `84.54% <100.00%> (-0.09%)` | :arrow_down: |
| [datafusion/src/physical\_plan/mod.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/25/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9tb2QucnM=) | `87.69% <100.00%> (+0.59%)` | :arrow_up: |
| [datafusion/src/physical\_plan/sort.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/25/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9zb3J0LnJz) | `91.45% <100.00%> (-0.74%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/25?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/25?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [c365a4f...0a4d560](https://codecov.io/gh/apache/arrow-datafusion/pull/25?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove merged pull request #25: Use atomics for SQLMetric implementation, remove unused name field
Posted by GitBox <gi...@apache.org>.
andygrove merged pull request #25:
URL: https://github.com/apache/arrow-datafusion/pull/25
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] returnString commented on pull request #25: Use atomics for SQLMetric implementation, remove unused name field
Posted by GitBox <gi...@apache.org>.
returnString commented on pull request #25:
URL: https://github.com/apache/arrow-datafusion/pull/25#issuecomment-824217318
Probably one for @andygrove, if you have some time :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org