[Rust] Trait-specialization requires nightly

[asfimport]

Trait specialization is widely used in the Rust Arrow implementation. Uses can be identified by searching for instances of default fn in the codebase:

 

$> rg -c 'default fn' ../arrow/rust/
 ../arrow/rust/parquet/src/util/test_common/rand_gen.rs:1
 ../arrow/rust/parquet/src/column/writer.rs:2
 ../arrow/rust/parquet/src/encodings/encoding.rs:16
 ../arrow/rust/parquet/src/arrow/record_reader.rs:1
 ../arrow/rust/parquet/src/encodings/decoding.rs:13
 ../arrow/rust/parquet/src/file/statistics.rs:1
 ../arrow/rust/arrow/src/array/builder.rs:7
 ../arrow/rust/arrow/src/array/array.rs:3
 ../arrow/rust/arrow/src/array/equal.rs:3

 

This feature requires Nightly Rust. Additionally, there is no schedule for stabilization , primarily due to an unresolved soundness hole. (Note: there has been further discussion and ideas for resolving the soundness issue, but to my knowledge no definitive action.)

If we can remove specialization from the Rust codebase, we will not be blocked on the Rust team's stabilization of that feature in order to move to stable Rust.

Reporter: Kyle Strand
Assignee: Jorge Leitão / @jorgecarleitao

PRs and other links:

• GitHub Pull Request #8206
• GitHub Pull Request #8485

_{Note: This issue was originally created as ARROW-10002. Please see the migration documentation for further details.}

[asfimport]

Kyle Strand:
I have put together a repository with a minimal example of one way in which we're using specialization in the array module: https://github.com/BatmanAoD/arrow-rust-specialization-alternatives

The master branch shows how the code is written currently. This pull request shows how we could avoid specialization by introducing an "indexing" method associated with each primitive type: BatmanAoD/arrow-rust-specialization-alternatives#1

[asfimport]

Andy Grove / @andygrove:
Thanks [~batmanaod] this looks really interesting.

@paddyhoran [~nevime]  [~sunchao]  @alamb   @jorgecarleitao  @jhorstmann will likely be interested in this

[asfimport]

Neville Dipale / @nevi-me:
Hi [~batmanaod], I've looked at the code but haven't checked it out yet to do my own comparisons. I'd be interested in perf implications (I'm presuming there's no change for indexing), and how we would remove default fn on other trait methods, seeing as that it's mostly used to specialise between numeric primitives and booleans.

[asfimport]

Mahmut Bulut / @vertexclique:
Hi [~batmanaod]  , I have checked out the code and commented about ornamental changes. I don't see any visible perf implications. since dispatch over PrimitiveArrayOps replaced by the ArrowPrimitiveType, I don't expect that much perf impact.

[asfimport]

Kyle Strand:
[~nevi_me] and @vertexclique, thanks for taking a look!

I agree that there shouldn't be any perf impact; the relevant monomorphizations should be inlined in both the current code and in the proposed version. I'd be glad to have some benchmarking done once I've made the changes on a branch of the actual Arrow code, though.

I've been looking into several of the uses of default fn, not just this one, but I chose this one as a starting point because it seemed particularly tricky.

I'll go ahead and start implementing the suggested approach.

[asfimport]

Kyle Strand:
I have taken a first pass at changing the implementation of PrimitiveArrayOps here: #8206

[asfimport]

Andy Grove / @andygrove:
Issue resolved by pull request 8206
#8206

[asfimport]

Kyle Strand:
My first PR only removes default fn from one trait.

[asfimport]

Kyle Strand:
[~sunchao] [~nevi_me], @paddyhoran, @alamb, @jorgecarleitao, and @andygrove, the next step I would like to take is a fairly major refactoring. I don't know if there is a good reason for the current distinction between ArrowPrimitiveType and ArrowNativeType, and the ticket where they were introduced ([ARROW-3878]) doesn't provide many details, but it makes it somewhat more difficult to remove uses of default fn. I've written a Google Doc to explain the current situation and the change I would like to make: https://docs.google.com/document/d/14g80-O4lE5dDCv9TOOaS7xzshhGJ0mmrDDoLFvctEpk/edit?usp=sharing

[asfimport]

Jorge Leitão / @jorgecarleitao:
[~batmanaod], I agree with the proposal. I also struggled with the T::Native vs T for some time while working on the crate.

[asfimport]

Andy Grove / @andygrove:
[~batmanaod] Thank you for the detailed write-up of the issue. I am also supportive of this effort.

[asfimport]

Andrew Lamb / @alamb:
FWIW "I propose that we eliminate ArrowNativeType and simply merge its definition with ArrowPrimitiveType. " – also sounds good to me. Thanks for writing it up [~batmanaod]

[asfimport]

Andy Grove / @andygrove:
It seems like everyone so far agrees with the direction. The next question is when do we do this and who wants to do this. Given that the 2.0.0 release is targetted for next week, should we move this to 3.0.0?

[asfimport]

Chao Sun / @sunchao:
+1. It will be awesome if we can remove specialization with this approach! 

[asfimport]

Kyle Strand:
I think it makes sense to target this for 3.0.0.