Skip to content

Shift from Field to FieldRef for all user defined functions #16122

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
Merged
merged 8 commits into from
May 28, 2025
Merged

Shift from Field to FieldRef for all user defined functions #16122

merged 8 commits into from
May 28, 2025

Conversation

timsaucer
Copy link
Member

@timsaucer timsaucer commented May 20, 2025 •
edited by alamb
Loading

Which issue does this PR close?

Rationale for this change

With the switch from DataType to Field we may have some plans that create a lot of copy operations. This PR switches from Field to FieldRef for the user defined scalar, window, and aggregate functions.

What changes are included in this PR?

Changes to the API for the user defined functions and similar supporting operations throughout the code base.

Are these changes tested?

Tested via existing unit tests.

Are there any user-facing changes?

None (the APIs that are changed are not yet released)

TODO:

  • Update migration guide to describe changes necessary for end users

@timsaucer timsaucer self-assigned this May 21, 2025
@timsaucer timsaucer added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates core Core DataFusion crate common Related to common crate execution Related to the execution crate proto Related to proto crate ffi Changes to the ffi crate spark labels May 21, 2025
@github-actions github-actions bot added optimizer Optimizer rules functions Changes to functions implementation physical-plan Changes to the physical-plan crate and removed common Related to common crate execution Related to the execution crate labels May 21, 2025
@timsaucer timsaucer force-pushed the feat/reduce-field-copy branch from add60f1 to 3939a9e Compare May 21, 2025 12:04
@timsaucer timsaucer force-pushed the feat/reduce-field-copy branch from 8154e01 to c361436 Compare May 22, 2025 11:42
@timsaucer timsaucer marked this pull request as ready for review May 22, 2025 12:24
alamb
alamb approved these changes May 22, 2025
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you @timsaucer -- I had hoped this would be a bigger improvement, but I think it at least sets us up for being more efficient / less String cloning going forward

FieldRef will also allow metadata to be copied through hopefully without requiring a deep copy

cc @andygrove as I think Comet already had to update to a pre-release version. This might be disruptive again

datafusion/expr/src/expr_fn.rs
}

fn state_fields(&self, _args: StateFieldsArgs) -> Result<Vec<Field>> {
fn state_fields(&self, _args: StateFieldsArgs) -> Result<Vec<FieldRef>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

It is nice that this is now avoiding a deep copy of a bunch of Fields 👍

datafusion/functions/src/datetime/now.rs

fn return_field_from_args(&self, _args: ReturnFieldArgs) -> Result<Field> {
fn return_field_from_args(&self, _args: ReturnFieldArgs) -> Result<FieldRef> {
Ok(Field::new(
Copy link
Contributor

Choose a reason for hiding this comment

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

So in theory we could update this code to create the FieldRef once on creation, and then return an Arc::clone rather than re-creating the Field each time -- perhaps we can do that as some follow on PRs.

@alamb alamb added the api change Changes the API exposed to users of the crate label May 22, 2025
@timsaucer
Copy link
Member Author

I had hoped this would be a bigger improvement, but I think it at least sets us up for being more efficient / less String cloning going forward

Can you expand on this a little? Was there a specific metric you were watching to see performance improvements or is it looking at the code that we roughly have the same number of allocation operations? Or something else?

@github-actions github-actions bot added the documentation Improvements or additions to documentation label May 23, 2025
@alamb
Copy link
Contributor

alamb commented May 23, 2025

Can you expand on this a little? Was there a specific metric you were watching to see performance improvements or is it looking at the code that we roughly have the same number of allocation operations? Or something else?

It was the latter -- mostly I was thinking we'd be able to reuse FieldRef more than Field. There are some improvements for sure but for some reason I expected more. I don't think this is a big deal but wanted to mention it

@alamb alamb merged commit 00132da into apache:main May 28, 2025
30 checks passed
@alamb
Copy link
Contributor

alamb commented May 28, 2025

Thanks @timsaucer

@andygrove andygrove mentioned this pull request May 29, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

Assignees

@timsaucer timsaucer

Labels
api change Changes the API exposed to users of the crate core Core DataFusion crate documentation Improvements or additions to documentation ffi Changes to the ffi crate functions Changes to functions implementation logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Changes to the physical-expr crates physical-plan Changes to the physical-plan crate proto Related to proto crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reduce Field Copy operations before releasing 48.0.0
2 participants
@timsaucer @alamb