Skip to content

Conversation

@carols10cents
Copy link
Contributor

Which issue does this PR close?

Rationale for this change

I added a new benchmark in #14818. There wasn't documentation on how to add a new benchmark (and some of the existing docs were wrong) so I wrote up what I did as I figured it out. I guessed a lot, so please correct me!

What changes are included in this PR?

New and improved docs!

Are these changes tested?

No, but I'd love to have someone try following the new docs to test if they're clear and helpful!

Are there any user-facing changes?

Nope!

The command as written gives you `cargo` help, not `tpch` help as the
text above says. And the output shown was for the benchmark bin, not the
subcommand.

Also correct some inconsistencies and punctuation.
@alamb alamb added the documentation Improvements or additions to documentation label Feb 23, 2025
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 @carols10cents -- this looks good to me

I don't have any new benchmark to write to try these instructions out but they look good to me. Maybe @2010YOUY01 who recently added a benchmark could take a look at this one as well?

section on adding a benchmark subcommand and add code to create or download data as part of its
`run` function.

If you want to create or download the data with shell commands, in `benchmarks/bench.sh`, define a
Copy link
Contributor

Choose a reason for hiding this comment

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

thank you -- this content is great @carols10cents

I worry it will get out of sync being in a different file.

However, since it involves editing multiple files, I can't think of any place better to put it 🤔

@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Feb 24, 2025
@carols10cents
Copy link
Contributor Author

I also just added a commit removing the sentence about commenting on a PR with /benchmark-- @alamb says that's been disabled.

@alamb
Copy link
Contributor

alamb commented Feb 24, 2025

I also just added a commit removing the sentence about commenting on a PR with /benchmark-- @alamb says that's been disabled.

I think it was done in

@alamb alamb merged commit e799097 into apache:main Feb 24, 2025
4 checks passed
@alamb
Copy link
Contributor

alamb commented Feb 24, 2025

Thanks again @carols10cents

@alamb alamb added the documentation Improvements or additions to documentation label Feb 24, 2025
ozankabak pushed a commit to synnada-ai/datafusion-upstream that referenced this pull request Feb 25, 2025
* Correct docs on subcommand help

The command as written gives you `cargo` help, not `tpch` help as the
text above says. And the output shown was for the benchmark bin, not the
subcommand.

Also correct some inconsistencies and punctuation.

* Docs on how to add a new benchmark

* Improve wording and punctuation in benchmarks README

* Remove help text about /benchmark PR command that's disabled
@carols10cents carols10cents deleted the improve-benchmark-docs branch February 26, 2025 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Benchmark documentation could use some improvements

2 participants