You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2024/09/07 11:21:00 UTC
[PR] Minor: Add documentation about performance PRs, add (TBD) section on criteroa [datafusion]
alamb opened a new pull request, #12372:
URL: https://github.com/apache/datafusion/pull/12372
## Which issue does this PR close?
Related to https://github.com/apache/datafusion/issues/12357
## Rationale for this change
I think there is some "common knowledge" that is not explicitly written down anywhere
Specifically, that performance improvements should be accompanied by benchmark results (most recently this came up in https://github.com/apache/datafusion/pull/12270)
## What changes are included in this PR?
1. Document need for benchmarks for performance PRs
2. Add a section (without content) for the outcome of https://github.com/apache/datafusion/issues/12357
3. Some other "drive by" documentation improvements
Note I am not trying to change anything, only document the current reality.
## Are these changes tested?
by doc CI
## Are there any user-facing changes?
Docs
--
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.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add documentation about performance PRs, add (TBD) section on feature criteria [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #12372:
URL: https://github.com/apache/datafusion/pull/12372#discussion_r1748609618
##########
docs/source/contributor-guide/index.md:
##########
@@ -88,35 +106,61 @@ committer who approved your PR to help remind them to merge it.
## Creating Pull Requests
-We recommend splitting your contributions into multiple smaller focused PRs rather than large PRs (500+ lines) because:
+When possible, we recommend splitting your contributions into multiple smaller focused PRs rather than large PRs (500+ lines) because:
1. The PR is more likely to be reviewed quickly -- our reviewers struggle to find the contiguous time needed to review large PRs.
2. The PR discussions tend to be more focused and less likely to get lost among several different threads.
3. It is often easier to accept and act on feedback when it comes early on in a small change, before a particular approach has been polished too much.
If you are concerned that a larger design will be lost in a string of small PRs, creating a large draft PR that shows how they all work together can help.
-Note all commits in a PR are squashed when merged to the `main` branch so there is one commit per PR.
+Note all commits in a PR are squashed when merged to the `main` branch so there is one commit per PR after merge.
# Reviewing Pull Requests
Some helpful links:
-- [PRs Waiting for Review]
-- [Approved PRs Waiting for Merge]
+- [PRs Waiting for Review] on GitHub
+- [Approved PRs Waiting for Merge] on GitHub
[prs waiting for review]: https://github.com/apache/datafusion/pulls?q=is%3Apr+is%3Aopen+-review%3Aapproved+-is%3Adraft+
[approved prs waiting for merge]: https://github.com/apache/datafusion/pulls?q=is%3Apr+is%3Aopen+review%3Aapproved+-is%3Adraft
-When reviewing PRs, please remember our primary goal is to improve DataFusion and its community together. PR feedback should be constructive with the aim to help improve the code as well as the understanding of the contributor.
+When reviewing PRs, our primary goal is to improve DataFusion and its community together. PR feedback should be constructive with the aim to help improve the code as well as the understanding of the contributor.
Please ensure any issues you raise contains a rationale and suggested alternative -- it is frustrating to be told "don't do it this way" without any clear reason or alternate provided.
Some things to specifically check:
-1. Is the feature or fix covered sufficiently with tests (see `Test Organization` below)?
+1. Is the feature or fix covered sufficiently with tests (see the [Testing](testing.md) section)?
2. Is the code clear, and fits the style of the existing codebase?
+## Performance Improvements
+
+Performance improvements are always welcome: performance is a key DataFusion
+feature.
+
+In general, the performance improvement from a change should be "enough" to
+justify any added code complexity. How much is "enough" is a judgement made by
+the committers, but generally means that the improvement should be noticeable in
+a real-world scenario and is greater than the noise of the benchmarking system.
+
+To help committers evaluate the potential improvement, performance PRs should
+in general be accompanied by benchmark results that demonstrate the improvement.
+
+The best way to demonstrate a performance improvement is with the existing
+benchmarks:
+
+- [System level SQL Benchmarks](https://github.com/apache/datafusion/tree/main/benchmarks)
+- Microbenchmarks such as those in [functions/benches](https://github.com/apache/datafusion/tree/main/datafusion/functions/benches)
+
+If there is no suitable existing benchmark, you can create a new one. It helps
Review Comment:
This is a good point, very often reviewing PRs we have to ask showing the actual performance benefit with existing benches or other performance tests
--
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.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add documentation about performance PRs, add (TBD) section on feature criteria [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead merged PR #12372:
URL: https://github.com/apache/datafusion/pull/12372
--
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.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Minor: Add documentation about performance PRs, add (TBD) section on feature criteria [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #12372:
URL: https://github.com/apache/datafusion/pull/12372#discussion_r1748053342
##########
docs/source/contributor-guide/index.md:
##########
@@ -88,35 +106,61 @@ committer who approved your PR to help remind them to merge it.
## Creating Pull Requests
-We recommend splitting your contributions into multiple smaller focused PRs rather than large PRs (500+ lines) because:
+When possible, we recommend splitting your contributions into multiple smaller focused PRs rather than large PRs (500+ lines) because:
1. The PR is more likely to be reviewed quickly -- our reviewers struggle to find the contiguous time needed to review large PRs.
2. The PR discussions tend to be more focused and less likely to get lost among several different threads.
3. It is often easier to accept and act on feedback when it comes early on in a small change, before a particular approach has been polished too much.
If you are concerned that a larger design will be lost in a string of small PRs, creating a large draft PR that shows how they all work together can help.
-Note all commits in a PR are squashed when merged to the `main` branch so there is one commit per PR.
+Note all commits in a PR are squashed when merged to the `main` branch so there is one commit per PR after merge.
# Reviewing Pull Requests
Some helpful links:
-- [PRs Waiting for Review]
-- [Approved PRs Waiting for Merge]
+- [PRs Waiting for Review] on GitHub
+- [Approved PRs Waiting for Merge] on GitHub
[prs waiting for review]: https://github.com/apache/datafusion/pulls?q=is%3Apr+is%3Aopen+-review%3Aapproved+-is%3Adraft+
[approved prs waiting for merge]: https://github.com/apache/datafusion/pulls?q=is%3Apr+is%3Aopen+review%3Aapproved+-is%3Adraft
-When reviewing PRs, please remember our primary goal is to improve DataFusion and its community together. PR feedback should be constructive with the aim to help improve the code as well as the understanding of the contributor.
+When reviewing PRs, our primary goal is to improve DataFusion and its community together. PR feedback should be constructive with the aim to help improve the code as well as the understanding of the contributor.
Please ensure any issues you raise contains a rationale and suggested alternative -- it is frustrating to be told "don't do it this way" without any clear reason or alternate provided.
Some things to specifically check:
-1. Is the feature or fix covered sufficiently with tests (see `Test Organization` below)?
+1. Is the feature or fix covered sufficiently with tests (see the [Testing](testing.md) section)?
Review Comment:
added link (there is actually no `Testing Organization` section anymore below)
##########
docs/source/contributor-guide/index.md:
##########
@@ -88,35 +106,61 @@ committer who approved your PR to help remind them to merge it.
## Creating Pull Requests
-We recommend splitting your contributions into multiple smaller focused PRs rather than large PRs (500+ lines) because:
+When possible, we recommend splitting your contributions into multiple smaller focused PRs rather than large PRs (500+ lines) because:
1. The PR is more likely to be reviewed quickly -- our reviewers struggle to find the contiguous time needed to review large PRs.
2. The PR discussions tend to be more focused and less likely to get lost among several different threads.
3. It is often easier to accept and act on feedback when it comes early on in a small change, before a particular approach has been polished too much.
If you are concerned that a larger design will be lost in a string of small PRs, creating a large draft PR that shows how they all work together can help.
-Note all commits in a PR are squashed when merged to the `main` branch so there is one commit per PR.
+Note all commits in a PR are squashed when merged to the `main` branch so there is one commit per PR after merge.
# Reviewing Pull Requests
Some helpful links:
-- [PRs Waiting for Review]
-- [Approved PRs Waiting for Merge]
+- [PRs Waiting for Review] on GitHub
+- [Approved PRs Waiting for Merge] on GitHub
[prs waiting for review]: https://github.com/apache/datafusion/pulls?q=is%3Apr+is%3Aopen+-review%3Aapproved+-is%3Adraft+
[approved prs waiting for merge]: https://github.com/apache/datafusion/pulls?q=is%3Apr+is%3Aopen+review%3Aapproved+-is%3Adraft
-When reviewing PRs, please remember our primary goal is to improve DataFusion and its community together. PR feedback should be constructive with the aim to help improve the code as well as the understanding of the contributor.
+When reviewing PRs, our primary goal is to improve DataFusion and its community together. PR feedback should be constructive with the aim to help improve the code as well as the understanding of the contributor.
Please ensure any issues you raise contains a rationale and suggested alternative -- it is frustrating to be told "don't do it this way" without any clear reason or alternate provided.
Some things to specifically check:
-1. Is the feature or fix covered sufficiently with tests (see `Test Organization` below)?
+1. Is the feature or fix covered sufficiently with tests (see the [Testing](testing.md) section)?
2. Is the code clear, and fits the style of the existing codebase?
+## Performance Improvements
Review Comment:
This is the new content
--
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.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org