Skip to content

MINOR: enable dyn_cmp_dict feature on arrow for physical expr crate#4163

Merged
alamb merged 1 commit intoapache:masterfrom
isidentical:minor-test-failure
Nov 10, 2022
Merged

MINOR: enable dyn_cmp_dict feature on arrow for physical expr crate#4163
alamb merged 1 commit intoapache:masterfrom
isidentical:minor-test-failure

Conversation

@isidentical
Copy link
Contributor

Seems like when we are executing the test suite for the physical exprs in a standalone way (which I tend to do quite a lot 😄), it fails with a test requiring a specific feature from arrow-rs (dyn_cmp_dict). It is already turned on for datafusion-core, so ideally this should not change anything.

 $ cargo test --package datafusion-physical-expr                                                                                                     1,135s
[...]

failures:

---- expressions::binary::tests::test_dictionary_type_to_array_coersion stdout ----
thread 'expressions::binary::tests::test_dictionary_type_to_array_coersion' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`: the test returned a termination value with a non-zero status code (1) which indicates a failure', /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/test/src/lib.rs:184:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    expressions::binary::tests::test_dictionary_type_to_array_coersion

test result: FAILED. 345 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.25s

error: test failed, to rerun pass '-p datafusion-physical-expr --lib'
 $ cargo test --package datafusion-physical-expr --lib -- expressions::binary::tests::test_dictionary_type_to_array_coersion --exact --nocapture      129ms
    Finished test [unoptimized + debuginfo] target(s) in 0.07s
     Running unittests src/lib.rs (target/debug/deps/datafusion_physical_expr-e292b53aa0ae60cf)

running 1 test
Error: ArrowError(CastError("Comparing array of type Dictionary(Int32, Utf8) with array of type Dictionary(Int32, Utf8) requires \"dyn_cmp_dict\" feature"))

@github-actions github-actions bot added the physical-expr Changes to the physical-expr crates label Nov 9, 2022
@isidentical isidentical marked this pull request as ready for review November 9, 2022 23:39
@tustvold
Copy link
Contributor

tustvold commented Nov 10, 2022

Can we only enable this for dev, dyn_cmp_dict is an extremely heavy feature that is intentionally not enabled by default?

@isidentical
Copy link
Contributor Author

Can we only enable this for dev, dyn_cmp_dict is an extremely heavy feature that is intentionally not enabled by default?

Makes sense @tustvold, I've changed the PR to only enable it as part of the dev dependencies.

@alamb alamb merged commit 36890b8 into apache:master Nov 10, 2022
@ursabot
Copy link

ursabot commented Nov 10, 2022

Benchmark runs are scheduled for baseline = 456c254 and contender = 36890b8. 36890b8 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-expr Changes to the physical-expr crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants