ARROW-11561: [Rust][DataFusion] Add Send + Sync to MemTable::load #9448

seddonm1 · 2021-02-08T21:32:15Z

This PR adds Send + Sync to the MemTable::load method to allow implementation of a persist method like Spark's Dataframe in an async function.

github-actions · 2021-02-08T21:32:55Z

https://issues.apache.org/jira/browse/ARROW-11561

codecov-io · 2021-02-08T21:57:25Z

Codecov Report

Merging #9448 (40e3b42) into master (a321cde) will decrease coverage by 0.05%.
The diff coverage is 66.52%.

@@            Coverage Diff             @@
##           master    #9448      +/-   ##
==========================================
- Coverage   82.14%   82.09%   -0.06%     
==========================================
  Files         232      233       +1     
  Lines       54150    54371     +221     
==========================================
+ Hits        44484    44637     +153     
- Misses       9666     9734      +68

Impacted Files	Coverage Δ
rust/arrow/src/datatypes.rs	`78.33% <ø> (ø)`
rust/benchmarks/src/bin/tpch.rs	`12.15% <0.00%> (+0.06%)`	⬆️
rust/datafusion/src/datasource/memory.rs	`80.00% <ø> (ø)`
...tafusion/src/physical_plan/distinct_expressions.rs	`90.29% <0.00%> (-0.89%)`	⬇️
...atafusion/src/physical_plan/expressions/average.rs	`82.30% <0.00%> (-1.49%)`	⬇️
.../datafusion/src/physical_plan/expressions/count.rs	`85.86% <0.00%> (-1.91%)`	⬇️
...atafusion/src/physical_plan/expressions/min_max.rs	`91.54% <0.00%> (-1.76%)`	⬇️
...st/datafusion/src/physical_plan/expressions/sum.rs	`77.27% <0.00%> (-1.02%)`	⬇️
rust/datafusion/src/physical_plan/mod.rs	`86.00% <ø> (ø)`
rust/datafusion/src/physical_plan/udaf.rs	`75.00% <0.00%> (-3.95%)`	⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c219e3...40e3b42. Read the comment docs.

alamb

I think this is a good idea, though it might cause a small breaking API change.

alamb · 2021-02-09T22:23:00Z

rust/datafusion/src/datasource/memory.rs

@@ -107,7 +107,7 @@ impl MemTable {

    /// Create a mem table by reading from another data source
    pub async fn load(
-        t: &dyn TableProvider,
+        t: Box<dyn TableProvider + Send + Sync>,


Suggested change

t: Box<dyn TableProvider + Send + Sync>,

t: Arc<dyn TableProvider + Send + Sync>,

I suggest using Arc to follow the convention in ExecutionContext:
https://github.com/apache/arrow/blob/master/rust/datafusion/src/execution/context.rs#L565

I don't think there is any reason load needs exclusive ownership over the TableProvider

Thanks @alamb .

You make a good point and it is confusing to me why Box is used in some places and Arc in others (given the I thought ideal characteristics of Arc): e.g.

/// Execution context for registering data sources and executing queries #[derive(Clone)] pub struct ExecutionContextState { /// Data sources that are registered with the context pub datasources: HashMap<String, Arc<dyn TableProvider + Send + Sync>>,

vs

pub fn register_table( &mut self, name: &str, provider: Box<dyn TableProvider + Send + Sync>,

I can take a shot at cleaning up all the APIs to take an Arc over the next few days. I think that would be the ideal outcome.

Great. I did replace everything with Arc locally for testing and tests pass at least.

This PR adds Send + Sync to the MemTable::load method to allow implementation of a `persist` method like Spark's Dataframe in an async function. Closes apache#9448 from seddonm1/send-sync Authored-by: Mike Seddon <seddonm1@gmail.com> Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>

@seddonm1

…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>

@seddonm1

…r> rather than Box and Arc NOTE: This is a backwards incompatible change in DataFusion Inspired by a conversation with @seddonm1 apache/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>

This PR adds Send + Sync to the MemTable::load method to allow implementation of a `persist` method like Spark's Dataframe in an async function. Closes apache#9448 from seddonm1/send-sync Authored-by: Mike Seddon <seddonm1@gmail.com> Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>

@seddonm1

…r> rather than Box and Arc NOTE: This is a backwards incompatible change in DataFusion Inspired by a conversation with @seddonm1 apache#9448 (comment) and apache#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 apache#9487 from alamb/alamb/arcd_table_provider Authored-by: Andrew Lamb <andrew@nerdnetworks.org> Signed-off-by: Jorge C. Leitao <jorgecarleitao@gmail.com>

This PR adds Send + Sync to the MemTable::load method to allow implementation of a `persist` method like Spark's Dataframe in an async function. Closes apache#9448 from seddonm1/send-sync Authored-by: Mike Seddon <seddonm1@gmail.com> Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>

@seddonm1

…r> rather than Box and Arc NOTE: This is a backwards incompatible change in DataFusion Inspired by a conversation with @seddonm1 apache#9448 (comment) and apache#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 apache#9487 from alamb/alamb/arcd_table_provider Authored-by: Andrew Lamb <andrew@nerdnetworks.org> Signed-off-by: Jorge C. Leitao <jorgecarleitao@gmail.com>

Add Send Sync

40e3b42

github-actions bot added Component: Rust - DataFusion Component: Rust labels Feb 8, 2021

alamb reviewed Feb 9, 2021

View reviewed changes

alamb approved these changes Feb 10, 2021

View reviewed changes

alamb closed this in 7c5cf92 Feb 11, 2021

alamb mentioned this pull request Feb 13, 2021

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

Closed

seddonm1 deleted the send-sync branch February 18, 2021 07:12

asfimport mentioned this pull request Feb 11, 2021

[Rust][DataFusion] Add Send + Sync to MemTable::load #27432

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-11561: [Rust][DataFusion] Add Send + Sync to MemTable::load #9448

ARROW-11561: [Rust][DataFusion] Add Send + Sync to MemTable::load #9448

seddonm1 commented Feb 8, 2021

github-actions bot commented Feb 8, 2021

codecov-io commented Feb 8, 2021

alamb left a comment

alamb Feb 9, 2021

seddonm1 Feb 9, 2021

alamb Feb 10, 2021

seddonm1 Feb 10, 2021

	t: Box<dyn TableProvider + Send + Sync>,
	t: Arc<dyn TableProvider + Send + Sync>,

ARROW-11561: [Rust][DataFusion] Add Send + Sync to MemTable::load #9448

ARROW-11561: [Rust][DataFusion] Add Send + Sync to MemTable::load #9448

Conversation

seddonm1 commented Feb 8, 2021

github-actions bot commented Feb 8, 2021

codecov-io commented Feb 8, 2021

Codecov Report

alamb left a comment

Choose a reason for hiding this comment

alamb Feb 9, 2021

Choose a reason for hiding this comment

seddonm1 Feb 9, 2021

Choose a reason for hiding this comment

alamb Feb 10, 2021

Choose a reason for hiding this comment

seddonm1 Feb 10, 2021

Choose a reason for hiding this comment