Skip to content

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

Closed
ritchie46 wants to merge 1 commit into
apache:masterfrom
ritchie46:fix-feature-compilation
Closed

ARROW-11387: [Rust] fix build for conditional compilation of features 'simd + avx512'#9337
ritchie46 wants to merge 1 commit into
apache:masterfrom
ritchie46:fix-feature-compilation

Conversation

@ritchie46

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown

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:

@ritchie46ritchie46 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
@codecov-io

Copy link
Copy Markdown

Codecov Report

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

Impacted file tree graph

@@ Coverage Diff @@## master #9337 +/- ##
=======================================
Coverage 81.89% 81.89% =======================================
Files 215 215 Lines 52988 52988 =======================================
Hits 43392 43392 Misses 9596 9596 
Impacted FilesCoverage Δ
rust/arrow/src/buffer.rs96.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

Copy link
Copy Markdown

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!

@ritchie46ritchie46 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

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks good to me. Thanks @ritchie46

FYI @vertexclique

@alamb

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

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.

@koukou 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
@github-actions

Copy link
Copy Markdown

@ChristianBeilschmidt

Copy link
Copy Markdown

Will this be published as a patch version to 3.0.0?

@alamb

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

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

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@ritchie46@codecov-io@ChristianBeilschmidt@alamb@vertexclique@andygrove@jorgecarleitao