ARROW-10636: [Rust][Parquet] Switch to Rust Stable by removing specialization in parquet

[GregBowyer]

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

This is a very initial attempt at removing the specialization features from the Rust Parquet implementation.

The specialisation is too complex to be covered by min_specialization and requires a bit of reworking in the crate.

Right now the code dispatches in sub-traits and methods on the Parquet type, and uses a combination of trait abuse, macros and transmutes to eliminate the feature.

I have broken this up into several commits ranging from the simplest removals (which could probably be taken fairly easily) to the most ugly and complex.

I am not stoked on the transmute abuse, and I think another take (or follow up) should be taken to remove as many as possible in the code.

The general trait for DataType::T has been made a private sealed trait to make it impossible to implement external to the Parquet crate, this is intentional as I dont think many of the public interfaces are sensible for end users to be able to implement.

TODO:

• Purge the added std::mem::transmutes if possible
• Refine and rationalise the unimplemented! implementations
• Performance test?
• Rebase & Relabel commits with JIRA number

[github-actions]

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
• Contribution Guidelines - How to contribute patches

[GregBowyer]

Minor: Maybe as a followup this should print the number rather than its raw bytes?

[github-actions]

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

[GregBowyer]

I think I have a solution with std::any::Any for most of the transmutes

[sunchao]

Thanks @GregBowyer for the PR. It's will be awesome if we can get this done. I think before merging this we should get some benchmark results showing the diff before & after. We use to have some benchmark in the old repo but haven't port them to arrow yet.

[GregBowyer]

I think I pulled all the transmutes out so thats good

Thanks @GregBowyer for the PR. It's will be awesome if we can get this done. I think before merging this we should get some benchmark results showing the diff before & after. We use to have some benchmark in the old repo but haven't port them to arrow yet.

I will just pull them in tomorrow and get some baselines, I dont think it will be that hard.

I doubt there will be anything here for performance. I mean there are some more branches now, but I suspect the predict effortlessly, and on the flip side we are not chasing things around the I-Cache so I suspect its a wash in the end. However better checked than assumed.

[alamb]

I looked carefully at the first half of this PR and it looks quite good. By my reading this has also removed the use of transmute

Assuming this looks good from the benchmark perspective as mentioned by @sunchao I think this is a great step forward. Thank you @GregBowyer

[alamb]

Suggested change

	impl_from_raw!(f32,self => {Err(general_err!("Type cannot be converted to i64"))});
	impl_from_raw!(f64,self => {Err(general_err!("Type cannot be converted to i64"))});
	impl_from_raw!(f32,self => {Err(general_err!("Type f32 cannot be converted to i64"))});
	impl_from_raw!(f64,self => {Err(general_err!("Type f64 cannot be converted to i64"))});

[alamb]

Here is a potential contribution to this effort: #8708 (a PR with the bench marks ported -- fyi @GregBowyer ).

To run:

cd arrow/rust/parquet
cargo bench

[jorgecarleitao]

I do not have time to review this, but having tried this myself once (and failed miserably), I am just leaving a big thank you note to @GregBowyer for this 😍

[GregBowyer]

As per benchmarking this with the changes in alamb#2 this is slower than specialisation (I ran benchmarks a lot as its very noisy). I have solutions for the speed in encoding (boolean and int96 are to be solved but shouldn't be hard) I will work on decoding shortly.

[GregBowyer]

I have been working on this w.r.t performance, I think I have most parts performing better than the original. I am running off clean benchmarks right now to validate.

There are a few ancillary tweaks I have made (I will call out in the PR review) that gain some additional performance (I spotted some pipeline hazards in perf) but nothing crazy

[alamb]

I have been working on this w.r.t performance, I think I have most parts performing better than the original. I am running off clean benchmarks right now to validate.

I am very excited to see it. Thank you so much @GregBowyer

[nevi-me]

I know this is still in-flight, but the better strategy here is to run the Parquet tests with cargo +stable test as we also add the stable toolchain to compile arrow with. You might need to rebase to see that in the dev scripts, as this change was added last week.

[GregBowyer]

I am ok with that, I am fighting the CI anyhow and any assistance is welcome.

[nevi-me]

I'll submit a PR on your branch

[nevi-me]

I think the safest option is to revert the CI changes, then we can add parquet to stable tests separately in #8821

[GregBowyer]

changes reverted, I will probably need help getting the small CI parts fixed up

[GregBowyer]

Ok review comments been addressed.

I seperated out the benchmark code from this PR and rebased it to match comment style in git.

I think when CI can be made to function I am good with the PR, I am thinking of following up with a round of performance tuning in compression and delta_fixed_bin

[codecov-io]

Codecov Report

Merging #8698 (f8f9749) into master (edff65d) will increase coverage by 7.47%.
The diff coverage is 84.08%.

@@ Coverage Diff @@## master #8698 +/- ##
==========================================
+ Coverage 76.81% 84.28% +7.47% 
==========================================
Files 181 194 +13 Lines 40985 47951 +6966 ==========================================
+ Hits 31483 40417 +8934 + Misses 9502 7534 -1968 

Impacted Files	Coverage Δ
rust/parquet/src/record/triplet.rs	93.24% <0.00%> (ø)
rust/parquet/src/data_type.rs	80.29% <75.64%> (-4.32%)	⬇️
rust/parquet/src/encodings/decoding.rs	92.49% <86.91%> (+1.03%)	⬆️
rust/parquet/src/encodings/encoding.rs	95.59% <91.02%> (+2.41%)	⬆️
rust/parquet/src/encodings/rle.rs	93.13% <93.33%> (+0.10%)	⬆️
rust/parquet/src/arrow/arrow_reader.rs	90.58% <100.00%> (ø)
rust/parquet/src/arrow/converter.rs	62.96% <100.00%> (+7.40%)	⬆️
rust/parquet/src/column/writer.rs	94.02% <100.00%> (+0.12%)	⬆️
rust/parquet/src/file/statistics.rs	93.80% <100.00%> (-0.60%)	⬇️
rust/parquet/src/util/bit_util.rs	92.39% <100.00%> (+0.14%)	⬆️
... and 102 more

---

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 edff65d...f8f9749. Read the comment docs.

[jorgecarleitao]

Thanks a lot, @GregBowyer ! I think that this is almost done here.

Two comments:

1. It seems that there are some changes on the submodule testing committed to this PR that I am not sure are intended. Could you double check? Typically this happens when git add -u is called before git submodule update.

2. I have been updating the CI for this PR here, and in particular this patch.

[jorgecarleitao]

A small update: the CI runs successfully for everything except Clippy (issue unrelated to this PR) 🎉 🎉🎉

I fielded #8909 that addresses the Clippy warning.

I see two options:

1. incorporate the patch with the CI changes mentioned above and merge this PR with those changes
2. merge this as is now and PR the change to the CI separately

Let me know what you prefer, @GregBowyer 😃

[alamb]

I suggest merging this PR as is and then putting in the CI change as a follow on PR (this one is already large and long outstanding). Just let us know @GregBowyer

[GregBowyer]

I am ok merging as is and putting a fixup in later.

I pushed commits that should not alter the testing submodule

[alamb]

I gave this one more once over and it (still looks good to me). Merging this in

[alamb]

Filed https://issues.apache.org/jira/browse/ARROW-10929 to track migrating CI to stable rust

[alamb]

I did one last local test run after merging from master to make sure the tests passed. They did. So I'll also send a note over to the dev channel. Again thanks so much for this work @GregBowyer

cc @sunchao@andygrove@jorgecarleitao@vertexclique@jhorstmann@Dandandan

[alamb]

I filed https://issues.apache.org/jira/browse/ARROW-10931 to track follow on work to improve compressors

[Dandandan]

Wow, awesome work everyone 💯