refactor: move `DataSource` to `datafusion-datasource` #14671

logan-keede · 2025-02-14T18:27:01Z

Which issue does this PR close?

Part of [Epic] Split datasources out from datafusion crate (datafusion/core) #14444.

Rationale for this change

What changes are included in this PR?

refactor of DataSource

Are these changes tested?

not completely, only cargo check work for now

Are there any user-facing changes?

yes, but only if they are building from source.

alamb · 2025-02-15T11:19:37Z

So exciting...

logan-keede · 2025-02-15T12:25:48Z

@alamb any Idea on what to do with unit tests in physical-plan? I cant think of anything except pulling functions that depend on the component that I moved, to core_integration tests or maybe to datasource crate. is there a better method?

alamb · 2025-02-15T16:22:36Z

@alamb any Idea on what to do with unit tests in physical-plan? I cant think of anything except pulling functions that depend on the component that I moved, to core_integration tests or maybe to datasource crate. is there a better method?

I looked at the errors, and most of them appear to be related to the tests in physical_plan using MemoryExec (well now DataSource with a MemoryExec).

Some other ideas:

Add a dev-dependency between datafusion-physical-plananddatafusion-datasource`
(better) Add a mock ExecutionPlan only for testing in physical_plan -- this would end up with some amount of duplicate code with DataSourceExec but I think that would be ok for testing.
Move the tests, as you say, into tests in core_integration or something

alamb · 2025-02-15T16:23:39Z

BTW thank you for working on this -- I suggest we try the "make a mock ExecutionPlan" approach -- that would be the cleanest I think and would be a good way for you to learn more about how ExecutionPlans worked

I can try and find time to sketch it out if needed

logan-keede · 2025-02-15T17:53:39Z

BTW thank you for working on this -- I suggest we try the "make a mock ExecutionPlan" approach -- that would be the cleanest I think and would be a good way for you to learn more about how ExecutionPlans worked

Thanks for the suggestions. I will try to make MockExecutionPlan.

logan-keede · 2025-02-16T18:07:02Z

@alamb, I have implemented a MockMemorySourceConfig and MockDataSource which is essentially same as what was previously in physical_plan but now for tests only. You can find the implementation here. The tests are passing on my PC, although there are some warnings, so cleaning up that part is still pending.

I think this is different from what you were suggesting. It would be pretty helpful if you can point me in the right direction.

Thanks,
Logan

alamb · 2025-02-16T18:15:43Z

@alamb, I have implemented a MockMemorySourceConfig and MockDataSource which is essentially same as what was previously in physical_plan but now for tests only. You can find the implementation here. The tests are passing on my PC, although there are some warnings, so cleaning up that part is still pending.

I think this is different from what you were suggesting. It would be pretty helpful if you can point me in the right direction.

Thanks, Logan

I think this sounds reasonable -- I was just saying that the split between MockMemorySourceConfig and MockDataSource is likely not necessary. There is a split in the current DataSourceConfig and DataSourceExec to permit sharing between difefrent data sources

logan-keede · 2025-02-16T18:30:06Z

I think this sounds reasonable -- I was just saying that the split between MockMemorySourceConfig and MockDataSource is likely not necessary.

It does look that way, I will try to make it one Mock struct.

…_DataSource

logan-keede · 2025-02-18T19:25:05Z

@alamb What do we want to do with benches? (they are causing circular dependency)
except that I think this is ready for review.

alamb · 2025-02-19T10:56:58Z

@alamb What do we want to do with benches? (they are causing circular dependency) except that I think this is ready for review.

I think we should move the benches into datafusion/core/benches

alamb · 2025-02-19T12:26:33Z

Thank you very much for this PR @logan-keede -- I will try and review it later today (I am out of the office this week so I have only limited time to review PRs)

alamb

Thank you SO much @logan-keede -- this is a really nice PR -- its structure is 💯 and the fact you got everything working is very impressive

I had a few small comments (unecessary dependency and a rename) but otherwise this PR is ready to go. I also think if necessary we could do those refactors as follow on PRs

Again thank you so much

FYI @mertak-synnada

alamb · 2025-02-20T13:03:14Z

datafusion-cli/src/functions.rs

@@ -28,10 +28,10 @@ use arrow::record_batch::RecordBatch;
 use arrow::util::pretty::pretty_format_batches;
 use datafusion::catalog::{Session, TableFunctionImpl};
 use datafusion::common::{plan_err, Column};
+use datafusion::datasource::memory::MemorySourceConfig;


alamb · 2025-02-20T13:03:32Z

datafusion/core/Cargo.toml

@@ -222,3 +222,7 @@ required-features = ["nested_expressions"]
 [[bench]]
 harness = false
 name = "dataframe"
+
+[[bench]]
+harness = false


alamb · 2025-02-20T13:06:01Z

datafusion/datasource/src/memory.rs

+
+/// Data source configuration for reading in-memory batches of data
+#[derive(Clone)]
+pub struct MemorySourceConfig {


I am just confirming that the plan is that datasource will have MemorySourceConfig / MemorySource and that we will move the other format specific things (like parquet) to their own crates like datafusion-datasource-parquet ?

Yes, That's what I had in my mind so far. However, now that I think about it, I don't think format-specific crate(except parquet) will have more than 2(fileformat, Source+mod) files, If we are going to keep a separate crate for them, maybe we can keep a separate crate for MemorySourceConfig(1 file, Source + mod.rs ) too. What do you think?

alamb · 2025-02-20T13:08:58Z

datafusion/physical-plan/src/joins/nested_loop_join.rs

-    use crate::memory::MemorySourceConfig;
-    use crate::source::DataSourceExec;
+    use crate::test::MockMemorySourceConfig;
+    // use crate::test::MockMemorySourceConfig;


yes, at a few more places too. I think I have cleaned this out with latest commit.

alamb · 2025-02-20T13:09:07Z

datafusion/physical-plan/src/joins/test_utils.rs

 use crate::repartition::RepartitionExec;
-use crate::source::DataSourceExec;
+use crate::test::MockMemorySourceConfig;
+// use crate::test::MockMemorySourceConfig;


Suggested change

// use crate::test::MockMemorySourceConfig;

alamb · 2025-02-20T13:12:46Z

datafusion/physical-plan/src/test.rs


 pub mod exec;

+#[derive(Clone, Debug)]
+pub struct MockMemorySourceConfig {


Some suggestions:

Document that this implements an in memory DataSource for testing purposes (and maybe we can add a link to the real memory source) and that it duplicates much of the MemorySourceConfig to avoid a dependency between physical-plan and datasource

Rename it to something like TestMemoryExec as this is not a config but rather something that impl ExecutionPlan and is used as a source of data in the tests

mertak-synnada

LGTM! Thank you for all the hard work. I’ve added a few optional minor comments

mertak-synnada · 2025-02-20T14:25:31Z

datafusion/physical-plan/src/test.rs

+    /// A memory table can be ordered by multiple expressions simultaneously.
+    /// [`EquivalenceProperties`] keeps track of expressions that describe the
+    /// global ordering of the schema. These columns are not necessarily same; e.g.
+    /// ```text
+    /// ┌-------┐
+    /// | a | b |
+    /// |---|---|
+    /// | 1 | 9 |
+    /// | 2 | 8 |
+    /// | 3 | 7 |
+    /// | 5 | 5 |
+    /// └---┴---┘
+    /// ```
+    /// where both `a ASC` and `b DESC` can describe the table ordering. With
+    /// [`EquivalenceProperties`], we can keep track of these equivalences
+    /// and treat `a ASC` and `b DESC` as the same ordering requirement.
+    ///
+    /// Note that if there is an internal projection, that projection will be
+    /// also applied to the given `sort_information`.


Maybe we can remove this documentation and add a link to the original MemorySourceConfig

mertak-synnada · 2025-02-20T14:27:25Z

datafusion/datasource/src/memory.rs

+}
+
+#[cfg(test)]
+mod memory_exec_tests {


Nit: This looks forgotten. While moving the tests, I think we can rename the module as memory_source_tests since there's no MemoryExec

logan-keede · 2025-02-20T15:34:49Z

@alamb @mertak-synnada TYSM for the review!! I think I have addressed your suggestions in the latest commit. Let me know if they look good to you (especially the documentation part) or if you have anything else.

alamb · 2025-02-21T11:48:37Z

datafusion/physical-plan/src/test.rs

@@ -49,8 +49,15 @@ use futures::{Future, FutureExt};

 pub mod exec;

+/// `TestMemoryExec` is a mock equivalent to [`MemorySourceConfig`] with [`ExecutionPlan`] implemented for testing.
+/// i.e. It has some but not all the functionality of [`MemorySourceConfig`].


alamb · 2025-02-21T11:49:32Z

Thanks @logan-keede and @mertak-synnada -- this looks great and thanks (again) for driving the plan forward

* build moves only tests+benches pending * unstable * some tests fixed * Mock MemorySourceConfig and DataSource * some cleanup * test pass but Mock is not efficient * temporary stable * one struct, test pass, cleaning pending * cleaning * more cleaning * clippy * 🧹🧹*cleaning*🧹🧹 * adding re-export * fix:cargo fmt * fix: doctest * fix: leftout doctest * fix: circular dependency * clean, rename, document, improve

build moves only tests+benches pending

2fb9085

logan-keede marked this pull request as draft February 14, 2025 18:27

github-actions bot added physical-expr Physical Expressions optimizer Optimizer rules core Core DataFusion crate substrait proto Related to proto crate labels Feb 14, 2025

logan-keede added 2 commits February 15, 2025 16:54

unstable

2d135a9

some tests fixed

34cd4ae

Mock MemorySourceConfig and DataSource

731e65a

logan-keede added 7 commits February 17, 2025 01:39

some cleanup

33a5408

test pass but Mock is not efficient

781034b

temporary stable

00c8008

one struct, test pass, cleaning pending

f041090

cleaning

408521c

more cleaning

7804ae5

Merge branch 'main' of https://github.com/apache/datafusion into move…

462376d

…_DataSource

logan-keede changed the title ~~WIP: move DataSource to datafusion-datasource~~ refactor: move DataSource to datafusion-datasource Feb 18, 2025

clippy

7cfd35a

logan-keede marked this pull request as ready for review February 18, 2025 19:11

🧹🧹*cleaning*🧹🧹

c70e109

logan-keede added 4 commits February 19, 2025 02:03

adding re-export

e9966f8

fix:cargo fmt

c73f00d

fix: doctest

7f0189d

fix: leftout doctest

673cb4a

alamb mentioned this pull request Feb 19, 2025

alamb's review queue #14698

Closed

fix: circular dependency

c0d2e60

AdamGS mentioned this pull request Feb 19, 2025

[WIP] Datafusion 46 support spiraldb/vortex#2417

Draft

alamb approved these changes Feb 20, 2025

View reviewed changes

mertak-synnada approved these changes Feb 20, 2025

View reviewed changes

clean, rename, document, improve

4edec1d

alamb approved these changes Feb 21, 2025

View reviewed changes

alamb merged commit b66beb8 into apache:main Feb 21, 2025
26 checks passed

alamb mentioned this pull request Feb 22, 2025

WIP : create datafusion-datasource-avro crate #14651

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move `DataSource` to `datafusion-datasource` #14671

refactor: move `DataSource` to `datafusion-datasource` #14671

logan-keede commented Feb 14, 2025 •

edited

Loading

alamb commented Feb 15, 2025

logan-keede commented Feb 15, 2025 •

edited

Loading

alamb commented Feb 15, 2025

alamb commented Feb 15, 2025

logan-keede commented Feb 15, 2025

logan-keede commented Feb 16, 2025

alamb commented Feb 16, 2025

logan-keede commented Feb 16, 2025

logan-keede commented Feb 18, 2025

alamb commented Feb 19, 2025

alamb commented Feb 19, 2025

alamb left a comment

alamb Feb 20, 2025

alamb Feb 20, 2025

alamb Feb 20, 2025

logan-keede Feb 20, 2025

alamb Feb 20, 2025

logan-keede Feb 20, 2025

alamb Feb 20, 2025

alamb Feb 20, 2025

mertak-synnada left a comment

mertak-synnada Feb 20, 2025

mertak-synnada Feb 20, 2025

logan-keede commented Feb 20, 2025

alamb Feb 21, 2025

alamb commented Feb 21, 2025

refactor: move DataSource to datafusion-datasource #14671

refactor: move DataSource to datafusion-datasource #14671

Conversation

logan-keede commented Feb 14, 2025 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

alamb commented Feb 15, 2025

logan-keede commented Feb 15, 2025 • edited Loading

alamb commented Feb 15, 2025

alamb commented Feb 15, 2025

logan-keede commented Feb 15, 2025

logan-keede commented Feb 16, 2025

alamb commented Feb 16, 2025

logan-keede commented Feb 16, 2025

logan-keede commented Feb 18, 2025

alamb commented Feb 19, 2025

alamb commented Feb 19, 2025

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mertak-synnada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logan-keede commented Feb 20, 2025

Choose a reason for hiding this comment

alamb commented Feb 21, 2025

refactor: move `DataSource` to `datafusion-datasource` #14671

refactor: move `DataSource` to `datafusion-datasource` #14671

logan-keede commented Feb 14, 2025 •

edited

Loading

logan-keede commented Feb 15, 2025 •

edited

Loading