Uh oh!
There was an error while loading. Please reload this page.
ARROW-11557: [Rust][Datafusion] Add deregister_table - #9445
Conversation
jorgecarleitao
left a comment
There was a problem hiding this comment.
Hi @marcprux , thank you very much for your contribution! It looks great!
I left two suggestions.
| pub fn deregister_table( | ||
| &mut self, | ||
| name: &str | ||
| ) -> bool { |
There was a problem hiding this comment.
I would consider raising an error if it cannot be un-registered instead of a boolean.
There was a problem hiding this comment.
I don't think it should be an error to attempt to deregister with an unknown table.
There was a problem hiding this comment.
my thinking was that as a user, if a function returns a bool, I am incentivized to write let _ = ctx.deregister_table(table) to ignore the value. If the function returns an error, I am incentivized to think about what to do with it (typically a unwrap or ?). So, imo, either a Result<()> or () seemed most reasonable.
Taking a step back, I can see that register_table currently just silently overwrites an existing table. So, for consistency, returning () makes sense. OTOH, I am not convinced that register_tables API is though through: a user can silently overwrite a table 🤯
Anyways, let's try to clean these later. I think that this PR is useful as is 👍
There was a problem hiding this comment.
There is precedent in Rust's stdlib for returning a boolean here (e.g. HashSet::remove), or an Option<> if it makes sense to reuse the removed TableProvider.
(also, unless the function is #[must_use], you don't have to let _ = .. to ignore a boolean return)
I guess the question is whether accidentally failing to deregister a table can trigger an issue later on. Perhaps not? The user would have to execute a query against a table they thought had been deregistered.
I agree that register_table should probably return a Some(TableProvider) when it replaces an existing table, though.
| assert_eq!(ctx.deregister_table("dual"), true); | ||
| assert_eq!(ctx.deregister_table("dual"), false); |
There was a problem hiding this comment.
What do you think about moving this to a separate test? This way, if something fails, it is more obvious what caused the failure.
There was a problem hiding this comment.
I'll do that. I hadn't done it originally because there is a fair amount of boilerplate to setting up a data source, but I agree that it makes sense to test is separately.
| .lock() | ||
| .unwrap() | ||
| .datasources | ||
| .remove(&name.to_string()) |
There was a problem hiding this comment.
I believe you should be able to use remove(name) here?
alamb
commented
Feb 11, 2021
Is this one ready to go? Any additional changes pending? |
alamb
commented
Feb 12, 2021
Thanks @marcprux ! |
https://issues.apache.org/jira/browse/ARROW-11557 Table de-registration, as discussed at https://lists.apache.org/thread.html/r0b3bc62a720c204c5bbe26d8157963276f7d61c05fcbad7eaf2ae9ff%40%3Cdev.arrow.apache.org%3EClosesapache#9445 from marcprux/patch-4 Authored-by: Marc Prud'hommeaux <marc@prux.org> Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>
…r> rather than Box and Arc 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 `Box`d and `Arc`d table providers (e.g. in [`LogicalPlan::TableScan`](https://github.com/apache/arrow/blob/437c9173c3e067712eb714c643ca839acc7ed7f6/rust/datafusion/src/logical_plan/plan.rs#L125)) * This change allows the same `TableProvider` instance to be reused easily for different `ExecutionContext`s # Changes * Change all uses of `TableProvider` to be wrapped in `Arc` rather than `Box` Closes#9487 from alamb/alamb/arcd_table_provider Authored-by: Andrew Lamb <andrew@nerdnetworks.org> Signed-off-by: Jorge C. Leitao <jorgecarleitao@gmail.com>
…r> rather than Box and Arc NOTE: This is a backwards incompatible change in DataFusion Inspired by a conversation with @seddonm1apache/arrow#9448 (comment) and apache/arrow#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 `Box`d and `Arc`d table providers (e.g. in [`LogicalPlan::TableScan`](https://github.com/apache/arrow/blob/f055d5e8ee8c6065f38d8351e3f668a43358cd98/rust/datafusion/src/logical_plan/plan.rs#L125)) * This change allows the same `TableProvider` instance to be reused easily for different `ExecutionContext`s # Changes * Change all uses of `TableProvider` to be wrapped in `Arc` rather than `Box` Closes#9487 from alamb/alamb/arcd_table_provider Authored-by: Andrew Lamb <andrew@nerdnetworks.org> Signed-off-by: Jorge C. Leitao <jorgecarleitao@gmail.com>
https://issues.apache.org/jira/browse/ARROW-11557
Table de-registration, as discussed at https://lists.apache.org/thread.html/r0b3bc62a720c204c5bbe26d8157963276f7d61c05fcbad7eaf2ae9ff%40%3Cdev.arrow.apache.org%3E