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 2025/06/28 14:56:35 UTC
[PR] Add comments to ClickBench queries about setting binary_as_string [datafusion]
alamb opened a new pull request, #16605:
URL: https://github.com/apache/datafusion/pull/16605
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes https://github.com/apache/datafusion/issues/16591
- Closes https://github.com/apache/datafusion/pull/16599
## 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 https://github.com/apache/datafusion/pull/16599#issuecomment-3015291713) 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 https://github.com/apache/datafusion/pull/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
```shell
./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?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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 comments to ClickBench queries about setting binary_as_string [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #16605:
URL: https://github.com/apache/datafusion/pull/16605#issuecomment-3033684735
@Dandandan can i trouble you for a review of this PR (it makes no code changes, just comments0
--
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 comments to ClickBench queries about setting binary_as_string [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #16605:
URL: https://github.com/apache/datafusion/pull/16605#discussion_r2191249463
##########
benchmarks/queries/clickbench/queries/q0.sql:
##########
@@ -1 +1,3 @@
+-- Must set for ClickBench hits_partitioned dataset. See https://github.com/apache/datafusion/issues/16591
Review Comment:
```suggestion
-- Must set for ClickBench hits_partitioned dataset. See https://github.com/apache/datafusion/issues/16591
```
##########
benchmarks/queries/clickbench/queries/q1.sql:
##########
@@ -1 +1,3 @@
+-- Must set for ClickBench hits_partitioned dataset. See https://github.com/apache/datafusion/issues/16591
+-- set datafusion.execution.parquet.binary_as_string = true
Review Comment:
```suggestion
-- Must set for ClickBench hits_partitioned dataset. See https://github.com/apache/datafusion/issues/16591
-- set datafusion.execution.parquet.binary_as_string = true
```
--
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 comments to ClickBench queries about setting binary_as_string [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #16605:
URL: https://github.com/apache/datafusion/pull/16605#discussion_r2192567377
##########
benchmarks/queries/clickbench/queries/q0.sql:
##########
@@ -1 +1,3 @@
+-- Must set for ClickBench hits_partitioned dataset. See https://github.com/apache/datafusion/issues/16591
Review Comment:
no problem! Thank you for the review!
--
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 comments to ClickBench queries about setting binary_as_string [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #16605:
URL: https://github.com/apache/datafusion/pull/16605#discussion_r2191250802
##########
benchmarks/queries/clickbench/queries/q0.sql:
##########
@@ -1 +1,3 @@
+-- Must set for ClickBench hits_partitioned dataset. See https://github.com/apache/datafusion/issues/16591
Review Comment:
I know I'm annoying :)
--
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 comments to ClickBench queries about setting binary_as_string [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead merged PR #16605:
URL: https://github.com/apache/datafusion/pull/16605
--
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