Skip to content

Conversation

@vertexclique
Copy link
Contributor

@vertexclique vertexclique commented Jul 31, 2020

Currently, the parquet is installing arrow-flight and it's dependencies, which breaks the CI builds and it's unnecessary because it is not used. Parquet should work without default features of arrow by default. Simple PR will enable building it leaner.

@vertexclique vertexclique changed the title ARROW-9609 - Leaner feature gating for arrow in parquet ARROW-9609: [Rust] Leaner feature gating for arrow in parquet Jul 31, 2020
@github-actions
Copy link

Copy link
Contributor

@svenwb svenwb left a comment

Choose a reason for hiding this comment

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

FYI @andygrove @nevi-me @paddyhoran I've tested this change in our project using Rust Arrow

Copy link
Member

Choose a reason for hiding this comment

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

hmm, does this mean we're removing arrow dependency from tests as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, because it is already a dependency in arrow.rs, using super::*; export in test modules will make it use the actual dependency exports of the arrow (which has been done already seems like). Tests had passed locally with no change and it passed here. I see it as good improvement tbh :)

@nealrichardson nealrichardson changed the title ARROW-9609: [Rust] Leaner feature gating for arrow in parquet ARROW-9608: [Rust] Leaner feature gating for arrow in parquet Jul 31, 2020
@github-actions
Copy link

@vertexclique
Copy link
Contributor Author

I really want to merge this PR to get rid of flight and it's dependencies while compiling parquet. My local tests and usage of this version didn't cause any problems and tests were passing. I don't know what is failing at CI at Dev / Source Release and Merge Script. Can we merge this one as it is?

@andygrove
Copy link
Member

@vertexclique No, we'll need to fix the regression in the test caused by this change. I can help fix it. I'll look into it later today.

@andygrove
Copy link
Member

I had a general question ... it seems inverted that arrow depends on flight. I wonder if we have the dependency the wrong way around?

@vertexclique
Copy link
Contributor Author

vertexclique commented Aug 3, 2020

Parquet crate is using arrow's default features which are default = ["flight", "prettyprint"] but flight is not needed, nor prettyprint for parquet. these are not used at all. Moreover, since features are additive, we can't override arrow dependencies features in any project that uses parquet if we continue to use default features included.
One more nit; it seems like we've added the same dependency as a dev dependency. Which is also not needed because it is in the default dependency list.

... it seems inverted that arrow depends on flight. I wonder if we have the dependency the wrong way around?

Sorry, might be I am quite a bit tired, but I didn't understand the inversion part.

@andygrove
Copy link
Member

@vertexclique I will need to look at the code again but I don't see why arrrow would depend on a protocol. I think the protocl should depend on arrow.

The core arrow crate should have as few dependencies as possible, IMO. Same goes for Parquet.

Not everyone will be buiding distributed systems that require the protocol.

@vertexclique
Copy link
Contributor Author

vertexclique commented Aug 3, 2020

@andygrove Exactly! This is the problem we are having with the default feature gating scheme that arrow has, which scattered to arrow dependant crates.

@andygrove
Copy link
Member

@vertexclique
Copy link
Contributor Author

vertexclique commented Aug 3, 2020

@andygrove hold on I will open a pr for that too :)

@andygrove
Copy link
Member

@vertexclique I already did, but happy to take yours too.

#7892

@vertexclique vertexclique force-pushed the vcq/ARROW-9608-parquet-gating branch from 84ae452 to 702398b Compare August 10, 2020 15:43
@vertexclique
Copy link
Contributor Author

@andygrove Rebased. Looking out for the CI atm.

@andygrove
Copy link
Member

andygrove commented Sep 7, 2020

@vertexclique Just checking in on status. The PR has been idle for a while now. Should we close it?

@andygrove
Copy link
Member

@vertexclique I'm closing this for now. Feel free to re-open if this is something that you are still working on.

@andygrove andygrove closed this Sep 12, 2020
@vertexclique
Copy link
Contributor Author

@andygrove sorry for the radio silence I was on vacation. Iirc this is not needed anymore. Since we have pruned dependency tree to be leaner. Thanks!

Sign up for free to 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.

4 participants