Fix: make datafusion-cli running inconsistent with clickbench benchma… by zhuqi-lucas · Pull Request #16599 · apache/datafusion

zhuqi-lucas · 2025-06-28T06:18:45Z

…rk for parquet format

Which issue does this PR close?

The clickbench benchmark data hits.parquet, hits_partitioned dataset specifies string columns
as binary due to how it was written, it was an older parquet written data.

But datafusion-cli is inconsistent with it, this PR try to make the datafusion-cli and clickbench benchmark consistent.

Closes#16591

Rationale for this change

The clickbench benchmark data hits.parquet, hits_partitioned dataset specifies string columns
as binary due to how it was written, it was an older parquet written data.

But datafusion-cli is inconsistent with it, this PR try to make the datafusion-cli and clickbench benchmark consistent.

What changes are included in this PR?

The clickbench benchmark data hits.parquet, hits_partitioned dataset specifies string columns
as binary due to how it was written, it was an older parquet written data.

But datafusion-cli is inconsistent with it, this PR try to make the datafusion-cli and clickbench benchmark consistent.

Are these changes tested?

Testing now, it will not have cast.

./datafusion-cli -c "EXPLAIN SELECT \"SearchPhrase\", MIN(\"URL\"), COUNT(*) AS c FROM 'data/hits_partitioned' WHERE \"URL\" LIKE '%google%' AND \"SearchPhrase\" <> '' GROUP BY \"SearchPhrase\" ORDER BY c DESC LIMIT 10;"DataFusionCLI v48.0.0
+---------------+-------------------------------+
| plan_type | plan |
+---------------+-------------------------------+
| physical_plan | ┌───────────────────────────┐ |
| | │ SortPreservingMergeExec │ |
| | │ -------------------- │ |
| | │ c DESClimit:10 │ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ SortExec(TopK) │ |
| | │ -------------------- │ |
| | │ c@2DESC │ |
| | │ │ |
| | │ limit:10 │ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ ProjectionExec │ |
| | │ -------------------- │ |
| | │ SearchPhrase: │ |
| | │ SearchPhrase │ |
| | │ │ |
| | │ c:count(Int64(1)) │ |
| | │ │ |
| | │ min(data/hits_partitioned │ |
| | │ .URL): │ |
| | │ min(data/hits_partitioned │ |
| | │ .URL) │ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ AggregateExec │ |
| | │ -------------------- │ |
| | │ aggr: │ |
| | │ min(data/hits_partitioned │ |
| | │ .URL), count(1) │ |
| | │ │ |
| | │ group_by: │ |
| | │ SearchPhrase │ |
| | │ │ |
| | │ mode: │ |
| | │ FinalPartitioned │ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ CoalesceBatchesExec │ |
| | │ -------------------- │ |
| | │ target_batch_size: │ |
| | │ 8192 │ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ RepartitionExec │ |
| | │ -------------------- │ |
| | │ partition_count(in->out): │ |
| | │ 14 -> 14 │ |
| | │ │ |
| | │ partitioning_scheme: │ |
| | │ Hash([SearchPhrase@0],14)│ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ AggregateExec │ |
| | │ -------------------- │ |
| | │ aggr: │ |
| | │ min(data/hits_partitioned │ |
| | │ .URL), count(1) │ |
| | │ │ |
| | │ group_by: │ |
| | │ SearchPhrase │ |
| | │ │ |
| | │ mode:Partial │ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ CoalesceBatchesExec │ |
| | │ -------------------- │ |
| | │ target_batch_size: │ |
| | │ 8192 │ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ FilterExec │ |
| | │ -------------------- │ |
| | │ predicate: │ |
| | │ URLLIKE %google% AND │ |
| | │ SearchPhrase != │ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ DataSourceExec │ |
| | │ -------------------- │ |
| | │ files:113 │ |
| | │ format: parquet │ |
| | │ │ |
| | │ predicate: │ |
| | │ URLLIKE %google% AND │ |
| | │ SearchPhrase != │ |
| | └───────────────────────────┘ |
| | |
+---------------+-------------------------------+
1 row(s) fetched.Elapsed0.058seconds.

Are there any user-facing changes?

Now, the datafusion-cli will not take extra time to cast to Utf8View.

…rk for parquet format

zhuqi-lucas · 2025-06-28T06:30:15Z

Before this PR, there will be extra cast for datafusion-cli:

./datafusion-cli -c "EXPLAIN SELECT \"SearchPhrase\", MIN(\"URL\"), COUNT(*) AS c FROM 'data/hits_partitioned' WHERE \"URL\" LIKE '%google%' AND \"SearchPhrase\" <> '' GROUP BY \"SearchPhrase\" ORDER BY c DESC LIMIT 10;"DataFusionCLI v48.0.0
+---------------+-------------------------------+
| plan_type | plan |
+---------------+-------------------------------+
| physical_plan | ┌───────────────────────────┐ |
| | │ SortPreservingMergeExec │ |
| | │ -------------------- │ |
| | │ c DESClimit:10 │ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ SortExec(TopK) │ |
| | │ -------------------- │ |
| | │ c@2DESC │ |
| | │ │ |
| | │ limit:10 │ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ ProjectionExec │ |
| | │ -------------------- │ |
| | │ SearchPhrase: │ |
| | │ SearchPhrase │ |
| | │ │ |
| | │ c:count(Int64(1)) │ |
| | │ │ |
| | │ min(data/hits_partitioned │ |
| | │ .URL): │ |
| | │ min(data/hits_partitioned │ |
| | │ .URL) │ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ AggregateExec │ |
| | │ -------------------- │ |
| | │ aggr: │ |
| | │ min(data/hits_partitioned │ |
| | │ .URL), count(1) │ |
| | │ │ |
| | │ group_by: │ |
| | │ SearchPhrase │ |
| | │ │ |
| | │ mode: │ |
| | │ FinalPartitioned │ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ CoalesceBatchesExec │ |
| | │ -------------------- │ |
| | │ target_batch_size: │ |
| | │ 8192 │ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ RepartitionExec │ |
| | │ -------------------- │ |
| | │ partition_count(in->out): │ |
| | │ 14 -> 14 │ |
| | │ │ |
| | │ partitioning_scheme: │ |
| | │ Hash([SearchPhrase@0],14)│ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ AggregateExec │ |
| | │ -------------------- │ |
| | │ aggr: │ |
| | │ min(data/hits_partitioned │ |
| | │ .URL), count(1) │ |
| | │ │ |
| | │ group_by: │ |
| | │ SearchPhrase │ |
| | │ │ |
| | │ mode:Partial │ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ CoalesceBatchesExec │ |
| | │ -------------------- │ |
| | │ target_batch_size: │ |
| | │ 8192 │ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ FilterExec │ |
| | │ -------------------- │ |
| | │ predicate: │ |
| | │ CAST(URLASUtf8View)LIKE│ |
| | │ %google% AND │ |
| | │ SearchPhrase │ |
| | │ != │ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ DataSourceExec │ |
| | │ -------------------- │ |
| | │ files:113 │ |
| | │ format: parquet │ |
| | │ │ |
| | │ predicate: │ |
| | │ CAST(URLASUtf8View)LIKE│ |
| | │ %google% AND │ |
| | │ SearchPhrase │ |
| | │ != │ |
| | └───────────────────────────┘ |
| | |
+---------------+-------------------------------+
1row(s) fetched.Elapsed0.067seconds.

alamb

Thank you @zhuqi-lucas 🙏

alamb · 2025-06-28T11:24:59Z

+ // Consistent with the clickbench benchmark:
+ // The hits_partitioned dataset specifies string columns
+ // as binary due to how it was written. Force it to strings
+ parquet_options.binary_as_string = true;


Ah, this makes sense -- I found the clickbench equivalent option here: https://github.com/ClickHouse/ClickBench/blob/main/datafusion/create_partitioned.sql#L4

alamb · 2025-06-28T11:25:43Z

Given how much we/I use datafusion-cli to test benchmark performance (clickbench in particular) I think this is a good change to help

alamb · 2025-06-28T11:42:55Z

Actually I am now torn about this as it will further diverge datafusion-cli and the core library.

Maybe we can just solve the human error part with comments in the queries. I'll give that a try later today so we can compare results

Thank you so much @zhuqi-lucas for debugging this

zhuqi-lucas · 2025-06-28T12:28:45Z

Actually I am now torn about this as it will further diverge datafusion-cli and the core library.
Maybe we can just solve the human error part with comments in the queries. I'll give that a try later today so we can compare results
Thank you so much @zhuqi-lucas for debugging this

Thank you @alamb for review, i agree it will make diverge datafusion-cli and the core library, i am wandering if we can make the following config default to true, so it will not have diverge for all default cases:

/// (reading) If true, parquet reader will read columns of/// `Binary/LargeBinary` with `Utf8`, and `BinaryView` with `Utf8View`.////// Parquet files generated by some legacy writers do not correctly set/// the UTF8 flag for strings, causing string columns to be loaded as/// BLOB instead.pub binary_as_string: bool, default = false

Now it default to false, but i am not sure if it will make other things broken.

alamb · 2025-06-28T13:52:44Z

Now it default to false, but i am not sure if it will make other things broken.

Yeah I think it will break other things -- it isn't correct in general to treat binary columns as strings -- the reason it is needed for the clickbench_partitioned dataset is that the parquet files in that dataset were written with some ancient version of parquet-cpp and didn't set the type annotations correctly. Interestingly hits.parquet (the single file) doesn't have the same problem

zhuqi-lucas · 2025-06-28T14:02:05Z

Now it default to false, but i am not sure if it will make other things broken.
Yeah I think it will break other things -- it isn't correct in general to treat binary columns as strings -- the reason it is needed for the clickbench_partitioned dataset is that the parquet files in that dataset were written with some ancient version of parquet-cpp and didn't set the type annotations correctly. Interestingly hits.parquet (the single file) doesn't have the same problem

Good explain, thank you @alamb , i also found that hits.parquet doesn't have the same problem, that's very interesting.

Dandandan · 2025-06-28T14:48:42Z

I agree, we should default by doing the correct thing. the binary_as_string is a nice thing for fixing the benchmark, but by default we shouldn't do it.

As mentioned earlier, I worder though if most of the query performance might be solved by moving the cast out of the filter for string filters (converting the string literal to binary literal in the filter instead)?

alamb · 2025-06-28T15:00:28Z

Here is a proposed alternative:

Add comments to ClickBench queries about setting binary_as_string #16605

As mentioned earlier, I worder though if most of the query performance might be solved by moving the cast out of the filter for string filters (converting the string literal to binary literal in the filter instead)?

I vaguely remember we tried this but hit some issue -- like I think the like kernels only work on utf8 -- https://docs.rs/arrow/latest/arrow/compute/kernels/comparison/fn.like.html or something similar to that

So we would have to define what it means to apply a like on a binary column

Dandandan · 2025-06-28T16:39:50Z

Here is a proposed alternative:
Add comments to ClickBench queries about setting binary_as_string #16605
As mentioned earlier, I worder though if most of the query performance might be solved by moving the cast out of the filter for string filters (converting the string literal to binary literal in the filter instead)?
I vaguely remember we tried this but hit some issue -- like I think the like kernels only work on utf8 -- https://docs.rs/arrow/latest/arrow/compute/kernels/comparison/fn.like.html or something similar to that
So we would have to define what it means to apply a like on a binary column

hm yeah that makes sense, perhaps we could find out what we need for this.

I guess it would be relatively powerful with predicate pushdown as well: we don't have to decode / validate the data to create the filter.

alamb · 2025-06-29T09:33:53Z

hm yeah that makes sense, perhaps we could find out what we need for this.

I guess it would be relatively powerful with predicate pushdown as well: we don't have to decode / validate the data to create the filter.

The thing is that I am not sure how common it is for users to want to apply string predicates on binary columns

The clickbench single paruqet file is correctly annotated so that the relevant columns are strings . The clickbench_partitioned dataset is the only dataset I have ever seen that has columns marked as binary that are intended to be treated as strings (and I am pretty sure it was a bug, not on purpose)

Thus I am not sure how important optimizing this case is

Dandandan · 2025-06-29T17:25:14Z

hm yeah that makes sense, perhaps we could find out what we need for this.
I guess it would be relatively powerful with predicate pushdown as well: we don't have to decode / validate the data to create the filter.
The thing is that I am not sure how common it is for users to want to apply string predicates on binary columns
The clickbench single paruqet file is correctly annotated so that the relevant columns are strings . The clickbench_partitioned dataset is the only dataset I have ever seen that has columns marked as binary that are intended to be treated as strings (and I am pretty sure it was a bug, not on purpose)
Thus I am not sure how important optimizing this case is

I agree, not so sure either :) though I was also thinking about other areas where we might do casting in filter expression and therefore limit the pushdown usefulness. Needs some examples though to show this is happening.

alamb · 2025-06-30T12:08:04Z

I agree, not so sure either :) though I was also thinking about other areas where we might do casting in filter expression and therefore limit the pushdown usefulness. Needs some examples though to show this is happening.

I think we already have a bunch of tests for this (no casts on the column) for string view, for example

datafusion/datafusion/sqllogictest/test_files/string/string_view.slt

Lines 212 to 219 in ebf49b4

 # Filter should have a StringView literal and no column cast 
 query TT 
 explain SELECT column1_utf8 from test where column1_utf8view = 'Andrew'; 
 ---- 
 logical_plan 
 01)Projection: test.column1_utf8 
 02)--Filter: test.column1_utf8view = Utf8View("Andrew") 
 03)----TableScan: test projection=[column1_utf8, column1_utf8view] 

@adriangb is also thinking about / working on a more general predicate optimziation in

Add PhysicalExpr optimizer and cast unwrapping #16530

They are on my list of things to review this week (the code changes are coming fast and furious)

alamb · 2025-06-30T12:09:53Z

Instead of merging this PR, I would like to propose an alternative (just add comments):

Add comments to ClickBench queries about setting binary_as_string #16605

adriangb · 2025-06-30T13:27:35Z

@adriangb is also thinking about / working on a more general predicate optimziation in
Add PhysicalExpr optimizer and cast unwrapping #16530

Yep the whole idea of that PR is that for cases like ScalarValue::Utf8View(...) = column instead of casting column we cast ScalarValue::Utf8View(...) to ScalarValue::Utf8(...) or even ScalarValue::Binary(...) (I think for equality that is valid, we could even do it more generally to avoid any Utf8 validation / string conversion). So I think it should resolve issues like this with no additional settings or configuration, at least after a couple more optimizations are added 😄

Fix: make datafusion-cli running inconsistent with clickbench benchma…
c44aabe
…rk for parquet format

zhuqi-lucas mentioned this pull request Jun 28, 2025
Improve performance of ClickBench Q21 by removing the cast #16591
Closed

alamb approved these changes Jun 28, 2025
View reviewed changes

alamb self-requested a review June 28, 2025 11:41

alamb mentioned this pull request Jun 28, 2025
Add comments to ClickBench queries about setting binary_as_string #16605
Merged

comphead closed this in #16605 Jul 8, 2025

Uh oh!

Conversation

zhuqi-lucas 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!

zhuqi-lucas commented Jun 28, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alambJun 28, 2025

Choose a reason for hiding this comment

Uh oh!

alamb commented Jun 28, 2025

Uh oh!

alamb commented Jun 28, 2025

Uh oh!

zhuqi-lucas commented Jun 28, 2025

Uh oh!

alamb commented Jun 28, 2025

Uh oh!

zhuqi-lucas commented Jun 28, 2025

Uh oh!

Dandandan commented Jun 28, 2025

Uh oh!

alamb commented Jun 28, 2025

Uh oh!

Dandandan commented Jun 28, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb commented Jun 29, 2025

Uh oh!

Dandandan commented Jun 29, 2025

Uh oh!

alamb commented Jun 30, 2025

Uh oh!

alamb commented Jun 30, 2025

Uh oh!

adriangb commented Jun 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Dandandan commented Jun 28, 2025•
edited
Loading