Skip to content

ARROW-11620: [Rust][DataFusion] Consistently use Arc<dyn TableProvider> rather than Box and Arc - #9487

Closed
alamb wants to merge 2 commits into
apache:masterfrom
alamb:alamb/arcd_table_provider
Closed

ARROW-11620: [Rust][DataFusion] Consistently use Arc<dyn TableProvider> rather than Box and Arc#9487
alamb wants to merge 2 commits into
apache:masterfrom
alamb:alamb/arcd_table_provider

Conversation

@alamb

Copy link
Copy Markdown
Contributor

NOTE: This is a backwards incompatible change in DataFusion

Inspired by a conversation with @seddonm1#9448 (comment) and #9445 as well as some upcoming needs in IOx (a consumer of DataFusion)

Rationale:

  • No TableProvider APIs actually require ownership of the TableProvider (they all take &self)
  • Internally DataFusion was storing the TableProvider as an Arc already and inconsistently uses Boxd and Arcd table providers (e.g. in LogicalPlan::TableScan)
  • This change allows the same TableProvider instance to be reused easily for different ExecutionContexts

Changes

  • Change all uses of TableProvider to be wrapped in Arc rather than Box

@github-actions

Copy link
Copy Markdown

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This changes the signature of deregister_table to be consistent with other Rust ecosystem APIs and returned the previously registered item, if any

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The Box passed into to register_table was already immediately wrapped in an Arc -- this PR allows the user of this API to keep a reference to the provider themselves.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps this could be a good time to also make this method return an Option<Arc<..>> so users can tell when they register a table over an existing table (consistent with deregister_table).

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That is an excellent idea. I will make that change

@alamb
alambforce-pushed the alamb/arcd_table_provider branch from e326f94 to 28f1709CompareFebruary 13, 2021 15:23
@seddonm1

Copy link
Copy Markdown
Contributor

👍 from me.

@jorgecarleitaojorgecarleitao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Nice cleanup!!

Note to reviewers: due to my screwup on master, the diff is large, but only the last two commits matter.

@alamb

Copy link
Copy Markdown
ContributorAuthor

Thanks @jorgecarleitao -- I'll rebase this against master again just to keep the diff small

@alamb
alambforce-pushed the alamb/arcd_table_provider branch from 119cb12 to 378fe18CompareFebruary 15, 2021 19:48
@alamb

Copy link
Copy Markdown
ContributorAuthor

I plan to merge this tomorrow sometime unless anyone has any additionals comments or concerns

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@alamb@seddonm1@abreis@jorgecarleitao