Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Rust] Remove specialisation from Rust parquet #26592

Closed
Tracked by #23060
asfimport opened this issue Nov 18, 2020 · 3 comments
Closed
Tracked by #23060

[Rust] Remove specialisation from Rust parquet #26592

asfimport opened this issue Nov 18, 2020 · 3 comments

Comments

@asfimport
Copy link
Collaborator

asfimport commented Nov 18, 2020

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?

Reporter: Greg Bowyer / @GregBowyer
Assignee: Greg Bowyer / @GregBowyer

Related issues:

PRs and other links:

Note: This issue was originally created as ARROW-10636. Please see the migration documentation for further details.

@asfimport
Copy link
Collaborator Author

Andrew Lamb / @alamb:
Related to (or perhaps a dupe) of ARROW-6717

@asfimport
Copy link
Collaborator Author

Andrew Lamb / @alamb:
Issue resolved by pull request 8698
#8698

@asfimport
Copy link
Collaborator Author

Greg Bowyer / @GregBowyer:
I am excited to see what you get, it does occur to me maybe we should run a bench on ARM, but I think at this point might be over-analyzing everything :S

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

No branches or pull requests

1 participant