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.

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

Add dyn_cmp_dict feature flag to gate dyn comparison of dictionary arrays #2596

Closed
tustvold opened this issue Aug 27, 2022 · 2 comments · Fixed by #2597
Closed

Add dyn_cmp_dict feature flag to gate dyn comparison of dictionary arrays #2596

tustvold opened this issue Aug 27, 2022 · 2 comments · Fixed by #2597
Assignees
Labels
arrow Changes to the arrow crate enhancement Any new improvement worthy of a entry in the changelog

Comments

@tustvold
Copy link
Contributor

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

Now that we provide scalar comparison kernels, it is relatively rare that a query needs to perform array-array comparison. In fact DataFusion has only one test that now calls into this code, as the join filter tests happen to have a predicate comparing across columns. Further the case of comparing two dictionary arrays, or a dictionary array to some other type of array, is a rare case of a rare case.

Unfortunately generating the comparison kernels for dictionary arrays is hugely expensive as it requires generating code for each unique combination of index and value type. For many use-cases this is a significant overhead for no benefit.

The scalar kernels do not have this issue as the predicate is evaluated against the underlying values first, and then this boolean array is unpacked based on the dictionary. As neither stage is parameterized on both the key and value types, the combinatorial explosion is avoided.

Describe the solution you'd like

I would like a feature flag that disables code generation for dictionary comparison kernels

On my local machine this yields for a release build

________________________________________________________
Executed in   21.08 secs    fish           external
   usr time  153.14 secs  549.00 micros  153.14 secs
   sys time    6.23 secs  106.00 micros    6.23 secs

Compared to master

________________________________________________________
Executed in   84.87 secs    fish           external
   usr time  224.12 secs  677.00 micros  224.11 secs
   sys time    6.44 secs    0.00 micros    6.44 secs

Or an ~50% speedup.

Describe alternatives you've considered

I thought about other feature flags, I think we could definitely consider some of these as follow ups, but dyn_cmp_dict is the big hitter

  • dyn_cmp - just feature flag the dyn comparison kernels as a whole
  • dyn_cmp_distinct - just feature flag dyn comparison against different types of array
  • dict_non_i32 - feature flag support for non-i32 dictionaries (potentially really fiddly to implement)

Additional context

Related to #2594

@alamb @psvri

@tustvold tustvold added the enhancement Any new improvement worthy of a entry in the changelog label Aug 27, 2022
@tustvold tustvold self-assigned this Aug 27, 2022
@alamb
Copy link
Contributor

alamb commented Aug 31, 2022

Thank you @tustvold

@alamb
Copy link
Contributor

alamb commented Aug 31, 2022

For what it is worth I think the case of comparing DictionaryArray<Int32> to DictionaryArray<Int8> or whatever is so limited that we could handle it via a cast first (aka cast both to DictionaryArray<Int32> rather than have specialized code for all combinations

@alamb alamb added the arrow Changes to the arrow crate label Sep 1, 2022
@alamb alamb changed the title Add dyn_cmp_dict flag to gate dyn comparison of dictionary arrays Add dyn_cmp_dict feature flag to gate dyn comparison of dictionary arrays Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate enhancement Any new improvement worthy of a entry in the changelog
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants