Use octet_length for ClickBench Q27/Q28 byte-length semantics#23475
Open
kosiew wants to merge 3 commits into
Open
Use octet_length for ClickBench Q27/Q28 byte-length semantics#23475kosiew wants to merge 3 commits into
octet_length for ClickBench Q27/Q28 byte-length semantics#23475kosiew wants to merge 3 commits into
Conversation
…stead of length() for q27 and q28
- Updated Q27/Q28 SQL to utilize octet_length(...).
- Adjusted expected EXPLAIN output to remove character_length(hits.URL/Referer).
- Modified plan labels to reflect octet_length(...).
- Confirmed existing non-ASCII coverage with expr.slt containing SELECT octet_length('josé').
…ics clarification - Updated comments in the following files: - benchmarks/queries/clickbench/queries/q27.sql - benchmarks/queries/clickbench/queries/q28.sql - benchmarks/sql_benchmarks/clickbench/benchmarks/q27.benchmark - benchmarks/sql_benchmarks/clickbench/benchmarks/q28.benchmark - Added note: "DataFusion length(...) counts characters; use octet_length(...) for ClickBench byte-length semantics."
Contributor
Author
|
run benchmark clickbench_1 |
Contributor
Author
|
run benchmark clickbench_partitioned |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing octet_length-01-23086 (08d730a) to 3e058f0 (merge-base) diff using: clickbench_1 File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing octet_length-01-23086 (08d730a) to 3e058f0 (merge-base) diff using: 27 File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing octet_length-01-23086 (08d730a) to 3e058f0 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_1 — base (merge-base)
clickbench_1 — branch
File an issue against this benchmark runner |
Contributor
Author
|
clickbench_partitioned: clickbench_1 |
Contributor
Author
|
run benchmark clickbench_1 clickbench_partitioned |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing octet_length-01-23086 (08d730a) to 3e058f0 (merge-base) diff using: clickbench_1 File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing octet_length-01-23086 (08d730a) to 3e058f0 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_1 — base (merge-base)
clickbench_1 — branch
File an issue against this benchmark runner |
2010YOUY01
approved these changes
Jul 12, 2026
2010YOUY01
left a comment
2010YOUY01
left a comment
Contributor
There was a problem hiding this comment.
LGTM, thank you!
I'm wondering is it possible to unify .sql and .benchmark for the same query? We could do it as follow-up perhaps.
alamb
approved these changes
Jul 12, 2026
alamb
left a comment
alamb
left a comment
Contributor
There was a problem hiding this comment.
Thank you @kosiew and @2010YOUY01
before we merge this PR, I think we should make a PR to update ClickBench itself to ensure our benchmarks in this repo match the queries in ClickBench itself.
|
|
||
| run | ||
| SELECT "CounterID", AVG(length("URL")) AS l, COUNT(*) AS c FROM hits WHERE "URL" <> '' GROUP BY "CounterID" HAVING COUNT(*) > 100000 ORDER BY l DESC LIMIT 25; | ||
| -- DataFusion length(...) counts characters; use octet_length(...) for ClickBench byte-length semantics. |
Contributor
There was a problem hiding this comment.
yeah this duplication is unfortunate but I think @Omega359 is working to consolidate them all
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
ClickBench Q27 and Q28 are intended to aggregate the byte length of string values, while DataFusion intentionally defines
lengthas an alias forcharacter_length, which counts UTF-8 characters rather than bytes. This change updates the in-repo ClickBench SQL to useoctet_length(...)explicitly, aligning these benchmark queries with ClickBench's intended semantics without changing DataFusion's SQL behavior.What changes are included in this PR?
Updated ClickBench Q27 to use
AVG(octet_length("URL"))instead ofAVG(length("URL"))in:benchmarks/queries/clickbench/queries/q27.sqlbenchmarks/sql_benchmarks/clickbench/benchmarks/q27.benchmarkUpdated ClickBench Q28 to use
AVG(octet_length("Referer"))instead ofAVG(length("Referer"))in:benchmarks/queries/clickbench/queries/q28.sqlbenchmarks/sql_benchmarks/clickbench/benchmarks/q28.benchmarkUpdated the corresponding SQLLogicTest (
clickbench.slt) queries and expectedEXPLAINoutput so the logical and physical plans referenceoctet_length(...)rather thancharacter_length(...)orlength(...).Added brief comments in the benchmark SQL noting that
length(...)in DataFusion counts characters and thatoctet_length(...)is used to preserve ClickBench byte-length semantics.Are these changes tested?
The patch updates the expected SQLLogicTest (
datafusion/sqllogictest/test_files/clickbench.slt) output for Q27 and Q28 to reflect the newoctet_length(...)plans.No test execution or benchmark results are included in this patch. Performance was not measured as part of this change.
Are there any user-facing changes?
There are no user-facing API or SQL semantic changes. This change only updates the in-repo ClickBench benchmark queries and their corresponding SQLLogicTest expectations to use explicit byte-length semantics.
LLM-generated code disclosure
This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed.