Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add dictionary_expresions feature (#4386) #4999

Merged
merged 2 commits into from Jan 24, 2023
Merged

Add dictionary_expresions feature (#4386) #4999

merged 2 commits into from Jan 24, 2023

Conversation

tustvold
Copy link
Contributor

Which issue does this PR close?

Closes #4386

Rationale for this change

Release build of datafusion-cli on master

________________________________________________________
Executed in   65.44 secs    fish           external
   usr time   17.23 mins  521.00 micros   17.23 mins
   sys time    0.48 mins    0.00 micros    0.48 mins

With this feature

________________________________________________________
Executed in   48.18 secs    fish           external
   usr time   16.46 mins  567.00 micros   16.46 mins
   sys time    0.48 mins    0.00 micros    0.48 mins

What changes are included in this PR?

Adds a dictionary_expressions feature that gates arrow/dyn_cmp_dict and arrow/dyn_arith_dict. I vacillated a bit on the naming and whether this should enable arrow/dyn_arith_dict but figured as this is just plumbing features through, and does not alter the actual DataFusion code, downstreams could not use this feature and manually enabled the desired arrow features themselves if they so wish.

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added core Core datafusion crate physical-expr Physical Expressions labels Jan 20, 2023
@tustvold tustvold added the api change Changes the API exposed to users of the crate label Jan 20, 2023
tustvold
tustvold commented Jan 20, 2023
View reviewed changes
datafusion/core/tests/path_partition.rs
@@ -204,7 +204,7 @@ async fn csv_filter_with_file_col() -> Result<()> {
);

let result = ctx
.sql("SELECT c1, c2 FROM t WHERE date='2021-10-27' and date!=c1 LIMIT 5")
.sql("SELECT c1, c2 FROM t WHERE date='2021-10-27' and c1!='2021-10-27' LIMIT 5")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't see a compelling reason why this test needed to test comparison of dictionary columns

Copy link
Contributor

@alamb alamb Jan 20, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the partitioning columns in a ListingTable are dictionary encoded and this test is verifying the encoding. I think we should put the test back (and gate it with a #cfg directive)

@tustvold tustvold mentioned this pull request Jan 20, 2023
Open
6 tasks
alamb
alamb approved these changes Jan 20, 2023
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me -- it would be nice to have dictionary support enabled by default, but not required

datafusion/physical-expr/Cargo.toml
@@ -35,12 +35,15 @@ path = "src/lib.rs"
[features]
crypto_expressions = ["md-5", "sha2", "blake2", "blake3"]
default = ["crypto_expressions", "regex_expressions", "unicode_expressions"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep dictionary support as a default, if possible

Copy link
Contributor Author

@tustvold tustvold Jan 20, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fairly strongly disagree, it is pretty esoteric. As a data point, none of IOx's integration tests require this, and we use dictionaries a LOT 😄

It is important to highlight this isn't "dictionary support" but non-scalar, binary dictionary kernels which are pretty unusual in practice

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fairly strongly disagree, it is pretty esoteric.

I am not sure how to quantify how esoteric the feature is or how commonly it is used. Clearly IOx uses it. I was just thinking that this PR changes the default behavior

But maybe that is ok.

Perhaps some other committers have thoughts.

Copy link
Contributor Author

@tustvold tustvold Jan 20, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clearly IOx uses it

That is the point I'm trying to make, IOx doesn't use it, at least not within any of its tests. A user theoretically could construct a query that directly compares dictionary columns, in practice there are extremely limited use-cases that come to mind of this.

This feature was only enabled in #4168 prior to that it was disabled

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at least not within any of its tests.

🤔 when I didn't enable the dyn dictionary kernels in arrow iox tests failed in some past version

We have it enabled here: https://github.com/influxdata/influxdb_iox/blob/6f39ae342e64848bd6555bddbc1d3fa30050f75e/arrow_util/Cargo.toml#L12

@tustvold tustvold merged commit 9f498bb into apache:master Jan 24, 2023
@ursabot
Copy link

ursabot commented Jan 24, 2023

Benchmark runs are scheduled for baseline = 5d4038a and contender = 9f498bb. 9f498bb is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@tustvold
Copy link
Contributor Author

It would appear enabling the dyn_arith_dict kernels causes the CI runner to exceed the available disk space 😱

Not really sure how best to workaround this...

@tustvold tustvold mentioned this pull request Jan 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
api change Changes the API exposed to users of the crate core Core datafusion crate physical-expr Physical Expressions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make Binary Dictionary Operations Optional
3 participants
@tustvold @ursabot @alamb