Add comments to ClickBench queries about setting binary_as_string by alamb · Pull Request #16605 · apache/datafusion

alamb · 2025-06-28T14:56:32Z

Which issue does this PR close?

Rationale for this change

I forgot to set an important setting when running the clickbench queries locally with datafusion-cli resulting in strange performance results.

We can't just change the default value of this setting as it isn't correct in general (see #16599 (comment)) it is pretty specific to the ClickBench datafiles

@zhuqi-lucas proposed changing the default in datafusion-cli but I fear that may also cause confusion -- see #16599 for more details

What changes are included in this PR?

Add comments to the ClickBench queries so future readers will not be (as) likely to make the same mistake I did

Are these changes tested?

I tested locally to make sure the comments are ignored by the benchmark runner and they are

./benchmarks/bench.sh run clickbench_partitioned 41

And

./benchmarks/bench.sh run clickbench_partitioned

To verify that the queries still run correctly with the comments

Are there any user-facing changes?

Copilot

Pull Request Overview

This PR adds informative comments at the top of each ClickBench query SQL file to remind users to set the configuration datafusion.execution.parquet.binary_as_string for the hits_partitioned dataset. The goal is to help avoid inconsistent performance measurements when running the benchmarks locally.

Added comments explaining the need for setting binary_as_string in every ClickBench query.
Linked the comment to the relevant GitHub issue for additional context.

Reviewed Changes

Copilot reviewed 43 out of 43 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
benchmarks/queries/clickbench/queries/q9.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q8.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q7.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q6.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q5.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q42.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q41.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q40.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q4.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q39.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q38.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q37.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q36.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q35.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q34.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q33.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q32.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q31.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q30.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q3.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q29.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q28.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q27.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q26.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q25.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q24.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q23.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q22.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q21.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q20.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q2.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q19.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q18.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q17.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q16.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q15.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q14.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q13.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q12.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q11.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q10.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q1.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage
benchmarks/queries/clickbench/queries/q0.sql	Added comment to advise setting datafusion.execution.parquet.binary_as_string for ClickBench usage

zhuqi-lucas

LGTM, this is a clear PR, thank you @alamb !

alamb · 2025-07-03T21:20:45Z

@Dandandan can i trouble you for a review of this PR (it makes no code changes, just comments)

benchmarks/queries/clickbench/queries/q0.sql

benchmarks/queries/clickbench/queries/q1.sql

comphead

lgtm thanks @alamb

Co-authored-by: Oleks V <comphead@users.noreply.github.com>

…ents

alamb added 2 commits June 28, 2025 09:54

Add comments to ClickBench queries about setting binary_as_string

71b7937

Add to the rest of the files

977ce0d

alamb marked this pull request as ready for review June 28, 2025 14:58

alamb mentioned this pull request Jun 28, 2025

Fix: make datafusion-cli running inconsistent with clickbench benchma… #16599

Closed

alamb requested a review from Copilot June 28, 2025 15:00

Copilot AI reviewed Jun 28, 2025

View reviewed changes

zhuqi-lucas approved these changes Jun 28, 2025

View reviewed changes

Add to extended too

3b2007e

alamb added the documentation Improvements or additions to documentation label Jun 30, 2025

alamb requested a review from Dandandan July 3, 2025 21:20

comphead reviewed Jul 8, 2025

View reviewed changes

benchmarks/queries/clickbench/queries/q0.sql Show resolved Hide resolved

comphead reviewed Jul 8, 2025

View reviewed changes

benchmarks/queries/clickbench/queries/q1.sql Show resolved Hide resolved

comphead approved these changes Jul 8, 2025

View reviewed changes

alamb and others added 2 commits July 8, 2025 09:40

Apply suggestions from code review

27568d7

Co-authored-by: Oleks V <comphead@users.noreply.github.com>

Merge remote-tracking branch 'apache/main' into alamb/clickbench_comm…

3fb72b7

…ents

github-actions bot removed the documentation Improvements or additions to documentation label Jul 8, 2025

comphead merged commit 92d8632 into apache:main Jul 8, 2025
27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add comments to ClickBench queries about setting binary_as_string#16605

Add comments to ClickBench queries about setting binary_as_string#16605
comphead merged 5 commits intoapache:mainfrom
alamb:alamb/clickbench_comments

alamb commented Jun 28, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

zhuqi-lucas left a comment

Uh oh!

alamb commented Jul 3, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

comphead left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

alamb commented Jun 28, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

zhuqi-lucas left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alamb commented Jul 3, 2025 •

edited

Loading