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.

Sign up for GitHub

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

Jump to bottom

Reduce Cloning of Field #3955

Closed
tustvold opened this issue Mar 26, 2023 · 4 comments · Fixed by #3983
Closed

Reduce Cloning of Field #3955

tustvold opened this issue Mar 26, 2023 · 4 comments · Fixed by #3983
Assignees
@tustvold
Labels
arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate enhancement Any new improvement worthy of a entry in the changelog parquet Changes to the parquet crate

Comments

@tustvold
Copy link
Contributor

tustvold commented Mar 26, 2023 •
edited
Loading

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

Currently DataType and Schema use Box<Field> and Vec<Field> for nested types such as StructArray, etc... This has a couple of drawbacks:

  • Field is not cheap to clone, involving cloning String, DataType, and potentially HashMap
  • DataType comparison cannot make use of cheap pointer comparison
  • Converting between DataType::Struct and Schema is unnecessarily expensive

Describe the solution you'd like

DataType should be updated to use Arc<Field> instead of Box<Field> and Arc<[Field]> instead of Vec<Field>.

Similarly Schema::new should be updated to accept impl Into<Arc<[Field]>>

Describe alternatives you've considered

We could instead move the Arc inside of Field i.e. struct Field(Arc<FieldInner>). This would potentially help codepaths that project or otherwise rearrange fields within a Vec<Field> but at the cost of an additional indirection. It is also unclear how to support this with the builder APIs added in #2024

We could also introduce a FieldPtr type and use this where we currently use Field

Additional context
@tustvold tustvold added the enhancement Any new improvement worthy of a entry in the changelog label Mar 26, 2023
@tustvold tustvold changed the title Use Arc<Field> and Arc<[Field]> in DataType and Schema Reduce Cloning of Field Mar 26, 2023
@tustvold tustvold mentioned this issue Mar 26, 2023
Closed
@tustvold
Copy link
Contributor Author

This would be a potentially breaking change as #2239 made Schema's fields public...

tustvold added a commit to tustvold/arrow-rs that referenced this issue Mar 26, 2023
@tustvold
Use Arc<str> inside Field (apache#3955)
a6e8d20
@tustvold tustvold mentioned this issue Mar 26, 2023
Closed
tustvold added a commit to tustvold/arrow-rs that referenced this issue Mar 27, 2023
@tustvold
Add Fields abstraction (apache#3955)
99f19b6
tustvold added a commit to tustvold/arrow-rs that referenced this issue Mar 27, 2023
@tustvold
Add Fields abstraction (apache#3955)
e48e93f
@alamb
Copy link
Contributor

alamb commented Mar 28, 2023

I believe @tustvold is working on this, so assigning to him

tustvold added a commit to tustvold/arrow-rs that referenced this issue Mar 28, 2023
@tustvold
Add Fields abstraction (apache#3955)
fa9c067
This was referenced Mar 29, 2023
Merged
Merged
tustvold added a commit that referenced this issue Mar 30, 2023
@tustvold
Add Fields abstraction (#3955) (#3965)
a9ac325 
* Add Fields abstraction (#3955)

* Fix ffi

* Fix array_cast

* Fix doc

* Add Into<Fields> for StructBuilder

* Further SchemaBuilder utilities
tustvold added a commit to tustvold/arrow-rs that referenced this issue Mar 30, 2023
@tustvold
Add UnionFields (apache#3955)
426a42b
@tustvold tustvold mentioned this issue Mar 30, 2023
Merged
tustvold added a commit that referenced this issue Mar 30, 2023
@tustvold
Add UnionFields (#3955) (#3981)
e5a1676 
* Add UnionFields (#3955)

* Fix array_cast

* Review feedback

* Clippy
tustvold added a commit to tustvold/arrow-rs that referenced this issue Mar 30, 2023
@tustvold
Use FieldRef in DataType (apache#3955)
e7e7465
@tustvold tustvold mentioned this issue Mar 30, 2023
Merged
tustvold added a commit that referenced this issue Mar 30, 2023
@tustvold
Use FieldRef in DataType (#3955) (#3983)
8262b92
@tustvold tustvold added the arrow Changes to the arrow crate label Mar 30, 2023
@tustvold tustvold added the parquet Changes to the parquet crate label Apr 7, 2023
@tustvold
Copy link
Contributor Author

tustvold commented Apr 7, 2023

label_issue.py automatically added labels {'parquet'} from #3956

@tustvold tustvold added the arrow-flight Changes to the arrow-flight crate label Apr 7, 2023
@tustvold
Copy link
Contributor Author

tustvold commented Apr 7, 2023

label_issue.py automatically added labels {'arrow-flight'} from #3965

@bjchambers bjchambers mentioned this issue Oct 12, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tustvold tustvold

Labels
arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate enhancement Any new improvement worthy of a entry in the changelog parquet Changes to the parquet crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use FieldRef in DataType (#3955) tustvold/arrow-rs
2 participants
@alamb @tustvold