Skip to content

Make clickbench query IDs 1-based #16455

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

Closed
wants to merge 1 commit into from

Conversation

pepijnve
Copy link
Contributor

Using 1 as the basis index makes it easier to correlate benchmark runs with query files in an editor

Which issue does this PR close?

None

Rationale for this change

Clickbench query IDs are currently zero-based, while most (if not all) text editors are 1-based when it comes to line numbering. Using 1-based ids makes it easier to correlate benchmark output with the query files.

Additionally TPCH IDs are already 1-based, so this aligns the two.

What changes are included in this PR?

Change clickbench runner to start counting at 1.

Are these changes tested?

Manual testing

Are there any user-facing changes?

Clickbench query IDs in benchmark output are all incremented by one

Using 1 as the basis index makes it easier to correlate benchmark runs with query files in an editor
@AdamGS
Copy link
Contributor

AdamGS commented Jun 19, 2025

But Clickbench treats/displays them as 0-based
Screenshot 2025-06-19 at 19 08 50

@pepijnve
Copy link
Contributor Author

Ok, that’s probably a stronger argument for keeping things as is

@pepijnve pepijnve closed this Jun 19, 2025
@pepijnve
Copy link
Contributor Author

@AdamGS would a PR that splits queries.sql into a file per query be acceptable? There's precedent for that in some of the other benchmarks, and that seems to be a reasonable compromise between maintaining consistent query IDs on the one hand and enabling trivial lookup on the other hand.

@AdamGS
Copy link
Contributor

AdamGS commented Jun 20, 2025

I'm not a maintainer but I feel your pain always to do that small mental jump when looking for a specific query. I think that's a fine solution.

@pepijnve
Copy link
Contributor Author

Ok, thanks for your opinion. Little developer quality of life improvements like this are worth it imo. I'll make a PR that contains the necessary code changes and a little bash script that pulls the query file from the upstream clickbench repo and splits it out.

@pepijnve
Copy link
Contributor Author

Take 2 in #16476

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants