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

Remove OwnedTableReference and OwnedSchemaReference #9933

Merged
merged 4 commits into from
Apr 4, 2024
Merged

Remove OwnedTableReference and OwnedSchemaReference #9933

merged 4 commits into from
Apr 4, 2024

Conversation

comphead
Copy link
Contributor

@comphead comphead commented Apr 4, 2024

Which issue does this PR close?

Related to #9764
Followup on #9824

Rationale for this change

In #9824 we switched to use Arc smart pointers instead of CoW<&'a>. Having done that the auxiliary types supporting lifetimes became obsolete.

What changes are included in this PR?

The PR removes OwnedTableReference and OwnedSchemaReference types which are now obsolete.

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added sql SQL Planner logical-expr Logical plan and expressions optimizer Optimizer rules core Core DataFusion crate labels Apr 4, 2024
@comphead
Copy link
Contributor Author

comphead commented Apr 4, 2024

I have run benches, there is slight noise for most of benches, however there are sometimes regression 4%, sometimes performance improvement 4%.

My feeling once we unify TableReference types, we can also unify references to this types which will give more boost. Sometimes we use direct reference, sometime option, sometime lifetime, etc, unifying accessing to TableReference will make code cleaner and faster

phillipleblanc
phillipleblanc reviewed Apr 4, 2024
View reviewed changes
Copy link
Contributor

@phillipleblanc phillipleblanc left a comment

Choose a reason for hiding this comment

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

I like this change 👍. What is the process to mark this as a breaking change users should be aware of when updating?

@comphead comphead added the api change Changes the API exposed to users of the crate label Apr 4, 2024
@alamb alamb mentioned this pull request Apr 4, 2024
Closed
7 tasks
@alamb
Copy link
Contributor

alamb commented Apr 4, 2024

I like this change 👍. What is the process to mark this as a breaking change users should be aware of when updating?

We add a api-change label which then highlights it as a breaking change in the release notes https://github.com/apache/arrow-datafusion/blob/main/dev/changelog/36.0.0.md

@comphead comphead mentioned this pull request Apr 4, 2024
Merged
alamb
alamb approved these changes Apr 4, 2024
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

This looks really nice -- thank you @comphead 🙏

datafusion/expr/src/logical_plan/builder.rs
.collect::<Vec<_>>();
left_fields.into_iter().chain(right_fields).collect()
}
JoinType::Left => {
// left then right, right set to nullable in case of not matched scenario
let left_fields = left_fields
.map(|(q, f)| (q.map(|r| r.to_owned_reference()), f.clone()))
.map(|(q, f)| (q.cloned(), f.clone()))
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking to try removing all this clones in future... if possible

@comphead
Copy link
Contributor Author

comphead commented Apr 4, 2024

I'll wait for couple of more hours, if there is no more comments, I'll merge to avoid conflicts on upcoming analyzer changes

@alamb alamb mentioned this pull request Apr 4, 2024
Merged
@comphead comphead merged commit 3ae0299 into apache:main Apr 4, 2024
24 checks passed
@phillipleblanc phillipleblanc mentioned this pull request May 13, 2024
Merged
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phillipleblanc phillipleblanc phillipleblanc left review comments

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
api change Changes the API exposed to users of the crate core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules sql SQL Planner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@comphead @alamb @phillipleblanc