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

Use Arc<str> inside Field (#3955) #3956

Closed
wants to merge 1 commit into from

Conversation

tustvold
Copy link
Contributor

@tustvold tustvold commented Mar 26, 2023

Draft as I am not sure about this and needs some shenanigans to work for serde

Which issue does this PR close?

Part of #3955

Rationale for this change

Allows for cheaper cloning of DataType and potentially faster comparisons, by comparing pointers instead of string contents

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added arrow Changes to the arrow crate parquet Changes to the parquet crate labels Mar 26, 2023
@alamb
Copy link
Contributor

alamb commented Mar 28, 2023

I think if we Arc the entire Field -- as in #3965 we may not need this

I know that @crepererum has mentioned other downstream places that copy the field name (like Statistics in DataFusion) but I think maybe a better design would be to update those downstream uses to hold a FieldRef rather than just the name 🤔

@tustvold
Copy link
Contributor Author

Agreed, my plan is to see what return we get from #3965 and then revisit this

@crepererum
Copy link
Contributor

hold a FieldRef rather than just the name

That's not always possible, since you may not have the datatype at hand yet. So at least during logical planning in DF this ain't an option.

@alamb
Copy link
Contributor

alamb commented Mar 28, 2023

That's not always possible, since you may not have the datatype at hand yet. So at least during logical planning in DF this ain't an option.

🤔 I think as soon as DataFusion creates a LogicalPlan / Expr it is possible to get the type using https://docs.rs/datafusion/latest/datafusion/prelude/enum.Expr.html#impl-ExprSchemable-for-Expr

Or more to the point, with a LogicalPlan you also have a Schema with Field

@tustvold
Copy link
Contributor Author

Going to close this for now to clear up the review backlog slightly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate parquet Changes to the parquet crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants