ARROW-11387: [Rust] fix build for conditional compilation of features 'simd + avx512' #9337

ritchie46 · 2021-01-27T14:40:23Z

This fixes conditional compilation for simd and avx512. Currently compilation for both fails:

error[E0428]: the name `buffer_bin_and` is defined multiple times
   --> arrow/src/buffer.rs:472:1
    |
414 | / pub(super) fn buffer_bin_and(
415 | |     left: &Buffer,
416 | |     left_offset_in_bits: usize,
417 | |     right: &Buffer,
...   |
468 | |     }
469 | | }
    | |_- previous definition of the value `buffer_bin_and` here
...
472 | / pub(super) fn buffer_bin_and(
473 | |     left: &Buffer,
474 | |     left_offset_in_bits: usize,
475 | |     right: &Buffer,
...   |
501 | |     }
502 | | }
    | |_^ `buffer_bin_and` redefined here
    |
    = note: `buffer_bin_and` must be defined only once in the value namespace of this module

error[E0428]: the name `buffer_bin_or` is defined multiple times
   --> arrow/src/buffer.rs:583:1
    |
525 | / pub(super) fn buffer_bin_or(
526 | |     left: &Buffer,
527 | |     left_offset_in_bits: usize,
528 | |     right: &Buffer,
...   |
579 | |     }
580 | | }
    | |_- previous definition of the value `buffer_bin_or` here
...
583 | / pub(super) fn buffer_bin_or(
584 | |     left: &Buffer,
585 | |     left_offset_in_bits: usize,
586 | |     right: &Buffer,
...   |
612 | |     }
613 | | }
    | |_^ `buffer_bin_or` redefined here
    |
    = note: `buffer_bin_or` must be defined only once in the value namespace of this module

This PR is tested on:

$ cargo c --features "simd"

$ cargo c --features "simd avx512"

$ cargo c --features "avx512"

Maybe a check like used in cargo hack can be added to the CI pipeline? This checks compilation for multiple feature combinations.

github-actions · 2021-01-27T14:40:46Z

Thanks for opening a pull request!

Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

Codecov Report

Merging #9337 (68c60cc) into master (ab5fc97) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #9337   +/-   ##
=======================================
  Coverage   81.89%   81.89%           
=======================================
  Files         215      215           
  Lines       52988    52988           
=======================================
  Hits        43392    43392           
  Misses       9596     9596

Impacted Files	Coverage Δ
rust/arrow/src/buffer.rs	`96.21% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab5fc97...68c60cc. Read the comment docs.

ChristianBeilschmidt · 2021-01-28T06:14:44Z

Thanks for opening a pull request!

Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW

Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)

* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)

@ritchie46 I guess you need to prepend your Jira Issue to your PR-title:
https://issues.apache.org/jira/browse/ARROW-11387

So, I guess
ARROW-11387: [Rust] fix build for conditional compilation of features 'simd + avx512'

And thank you for the PR!

alamb

It looks good to me. Thanks @ritchie46

FYI @vertexclique

alamb · 2021-01-28T11:59:20Z

On my machine, specifically, I tested in this manner:

# works fine
cargo +nightly build  --features=simd

# works fine
cargo +nightly build  --features=avx512

# fails without this PR, works with this PR:
cargo +nightly build  --features=avx512,simd

vertexclique · 2021-01-28T12:12:05Z

I think we've talked about this mutual exclusivity of features here: #8665 (comment)
I think this pr settles down simd feature's dependencies when avx512 is used. When this PR is merged it would be nice to document which one got selected. Like:

cargo +nightly build --features=avx512,simd This selects avx512 with this pr in I presume. That's enough to document from my point of view.

github-actions · 2021-01-28T20:54:09Z

https://issues.apache.org/jira/browse/ARROW-11387

ChristianBeilschmidt · 2021-01-29T14:19:39Z

Will this be published as a patch version to 3.0.0?

alamb · 2021-01-29T21:04:05Z

Will this be published as a patch version to 3.0.0?

@ChristianBeilschmidt -- I do not know what the plans are for a 3.0.0 patch. @andygrove do you know of how/if we do such a thing?

andygrove · 2021-01-29T21:06:42Z

For any JIRA issues that we want to include in a patch release, we should update the release version to include both 3.0.0 and 3.0.1 so that we have the list of candidates to use if we create a patch release as the first step

jorgecarleitao · 2021-01-30T06:18:26Z

I assigned both 4.0.0 and 3.0.1 to it, then. I agree that is a fix for a patch.

… 'simd + avx512' This fixes conditional compilation for `simd` and `avx512`. Currently compilation for both fails: ``` error[E0428]: the name `buffer_bin_and` is defined multiple times --> arrow/src/buffer.rs:472:1 | 414 | / pub(super) fn buffer_bin_and( 415 | | left: &Buffer, 416 | | left_offset_in_bits: usize, 417 | | right: &Buffer, ... | 468 | | } 469 | | } | |_- previous definition of the value `buffer_bin_and` here ... 472 | / pub(super) fn buffer_bin_and( 473 | | left: &Buffer, 474 | | left_offset_in_bits: usize, 475 | | right: &Buffer, ... | 501 | | } 502 | | } | |_^ `buffer_bin_and` redefined here | = note: `buffer_bin_and` must be defined only once in the value namespace of this module error[E0428]: the name `buffer_bin_or` is defined multiple times --> arrow/src/buffer.rs:583:1 | 525 | / pub(super) fn buffer_bin_or( 526 | | left: &Buffer, 527 | | left_offset_in_bits: usize, 528 | | right: &Buffer, ... | 579 | | } 580 | | } | |_- previous definition of the value `buffer_bin_or` here ... 583 | / pub(super) fn buffer_bin_or( 584 | | left: &Buffer, 585 | | left_offset_in_bits: usize, 586 | | right: &Buffer, ... | 612 | | } 613 | | } | |_^ `buffer_bin_or` redefined here | = note: `buffer_bin_or` must be defined only once in the value namespace of this module ``` This PR is tested on: `$ cargo c --features "simd"` `$ cargo c --features "simd avx512"` `$ cargo c --features "avx512"` Maybe a check like used in [cargo hack](https://github.com/taiki-e/cargo-hack) can be added to the CI pipeline? This checks compilation for multiple feature combinations. Closes apache#9337 from ritchie46/fix-feature-compilation Authored-by: Ritchie Vink <ritchie46@gmail.com> Signed-off-by: Jorge C. Leitao <jorgecarleitao@gmail.com>

fix build for conditional compilation of features 'simd + avx512'

68c60cc

github-actions bot added the Component: Rust label Jan 27, 2021

ritchie46 changed the title ~~fix build for conditional compilation of features 'simd + avx512'~~ [Rust] fix build for conditional compilation of features 'simd + avx512' Jan 27, 2021

ChristianBeilschmidt mentioned this pull request Jan 27, 2021

Update arrow requirement from 2.0 to 3.0 geo-engine/geoengine#130

Merged

ritchie46 changed the title ~~[Rust] fix build for conditional compilation of features 'simd + avx512'~~ [Arrow]-11387: [Rust] fix build for conditional compilation of features 'simd + avx512' Jan 28, 2021

alamb approved these changes Jan 28, 2021

View reviewed changes

kou changed the title ~~[Arrow]-11387: [Rust] fix build for conditional compilation of features 'simd + avx512'~~ ARROW-11387: [Rust] fix build for conditional compilation of features 'simd + avx512' Jan 28, 2021

jorgecarleitao closed this in 44e7441 Jan 30, 2021

ChristianBeilschmidt mentioned this pull request May 5, 2021

cargo doc fails with cannot find function bitwise_bin_op_simd_helper when using features=["simd"] apache/arrow-rs#232

Closed

asfimport mentioned this pull request Mar 10, 2021

[Rust] Arrow 3.0.0 release with simd feature doesn't compile without feature=avx512. #27280

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-11387: [Rust] fix build for conditional compilation of features 'simd + avx512' #9337

ARROW-11387: [Rust] fix build for conditional compilation of features 'simd + avx512' #9337

ritchie46 commented Jan 27, 2021

github-actions bot commented Jan 27, 2021

codecov-io commented Jan 27, 2021

ChristianBeilschmidt commented Jan 28, 2021

alamb left a comment •

edited

Loading

alamb commented Jan 28, 2021

vertexclique commented Jan 28, 2021

github-actions bot commented Jan 28, 2021

ChristianBeilschmidt commented Jan 29, 2021

alamb commented Jan 29, 2021

andygrove commented Jan 29, 2021

jorgecarleitao commented Jan 30, 2021

ARROW-11387: [Rust] fix build for conditional compilation of features 'simd + avx512' #9337

ARROW-11387: [Rust] fix build for conditional compilation of features 'simd + avx512' #9337

Conversation

ritchie46 commented Jan 27, 2021

github-actions bot commented Jan 27, 2021

codecov-io commented Jan 27, 2021

Codecov Report

ChristianBeilschmidt commented Jan 28, 2021

alamb left a comment • edited Loading

Choose a reason for hiding this comment

alamb commented Jan 28, 2021

vertexclique commented Jan 28, 2021

github-actions bot commented Jan 28, 2021

ChristianBeilschmidt commented Jan 29, 2021

alamb commented Jan 29, 2021

andygrove commented Jan 29, 2021

jorgecarleitao commented Jan 30, 2021

alamb left a comment •

edited

Loading