Skip to content

Consolidate OwnedTableReference with TableReference#5508

Closed
alamb wants to merge 3 commits intoapache:mainfrom
alamb:alamb/remove_pwned
Closed

Consolidate OwnedTableReference with TableReference#5508
alamb wants to merge 3 commits intoapache:mainfrom
alamb:alamb/remove_pwned

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented Mar 7, 2023

Which issue does this PR close?

Related to #5343. I am trying to specifically demonstrate what I was talking about on https://github.com/apache/arrow-datafusion/pull/5343/files#r1123708924

Rationale for this change

TableReference vs OwnedTableReference and having to convert back and forth between them is cumbersome to work with. This PR proposes removing the OwnedTableReference as TableReference can actually hold an owned string (via Cow<'static>

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@alamb alamb marked this pull request as draft March 7, 2023 19:26
@github-actions github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions sql SQL Planner labels Mar 7, 2023
@alamb alamb changed the title Alamb/remove pwned Consolidate OwnedTableReference with TableReference Mar 7, 2023
@alamb alamb changed the title Consolidate OwnedTableReference with TableReference Consolidate OwnedTableReference with TableReference Mar 7, 2023
@Jefffrey
Copy link
Contributor

Jefffrey commented Mar 7, 2023

I see what you mean now, it didn't occur to me to use 'static as the lifetime for TableReference<'a> 🤯

Maybe change OwnedTableReference to be a type alias for TableReference<'static>? 🤔

@alamb
Copy link
Contributor Author

alamb commented Mar 8, 2023

Maybe change OwnedTableReference to be a type alias for TableReference<'static>? 🤔

That may help people understand how to use it better. I'll give it a try and see what it looks like

@Jefffrey
Copy link
Contributor

Jefffrey commented Mar 8, 2023

That may help people understand how to use it better. I'll give it a try and see what it looks like

FYI I've done that implementation on #5343 already @alamb

@alamb
Copy link
Contributor Author

alamb commented Mar 8, 2023

FYI I've done that implementation on #5343 already @alamb

Awesome -- thanks @Jefffrey -- I haven't made it back to that PR yet. I'll close this PR then

@alamb alamb closed this Mar 8, 2023
@alamb alamb deleted the alamb/remove_pwned branch March 8, 2023 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate logical-expr Logical plan and expressions sql SQL Planner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants