I looked at the tests here and from what I see, the only failure is in the duckdb execution engine for the io tests that you added. Below is a log of the test. The test_take tests seem to be fine.

self = <tests.fugue_duckdb.test_execution_engine.DuckBuiltInTests testMethod=test_io>

    def test_io(self):
        path = os.path.join(self.tmpdir, "a")
        path2 = os.path.join(self.tmpdir, "b.test.csv")
        path3 = os.path.join(self.tmpdir, "c.partition")
        with self.dag() as dag:
            b = dag.df([[6, 1], [2, 7]], "c:int,a:long")
            b.partition(num=3).save(path, fmt="parquet", single=True)
            b.save(path2, header=True)
        assert FileSystem().isfile(path)
        with self.dag() as dag:
            a = dag.load(path, fmt="parquet", columns=["a", "c"])
            a.assert_eq(dag.df([[1, 6], [7, 2]], "a:long,c:int"))
            a = dag.load(path2, header=True, columns="c:int,a:long")
            a.assert_eq(dag.df([[6, 1], [2, 7]], "c:int,a:long"))
        with self.dag() as dag:
            b = dag.df([[6, 1], [2, 7]], "c:int,a:long")
            b.partition(by='c').save(path3, fmt="parquet", single=False)
            b.save(path2, header=True)
>       assert FileSystem().isdir(path3)
E       AssertionError

So the issue here appears to be that the added tests support partitioned files as output (which is what the PR is for), but duckdb fails the test because it doesn't support partitioning yet.

How do we proceed with this @goodwanghan ?

The failure on duckdb test is expected because duckdb itself does not have partitioning feature. But it is quite straightforward to convert duckdbdataframe to arrow table (as_arrow), and then save to parquet with partition, so we should also make that change, then this looks super nice.

I apologize for the delay, my daughter was born in Feb, so I am extremely busy recently.

The PR looks great, we just need to change duckdb io function to pass the test suite.

You can see we have other work items to redesign the IO part for all engines. Because the current design is a bit messy. But please continue working on this PR and merge first. The code refactoring can happen later. As long as we have good unit tests, we can ship this feature first.

Thank you!

@LaurentErreca do you plan to finalize this PR soon?

I've been working on this issue. For duckdb to pass the test, it must be able to write hive partitioned dataset. I propose to use arrow with param partition_cols instead of the duckdb write function which uses a COPY TO function under the hood. This function doesn't support hive partitioned write.

I've been working on this issue. For duckdb to pass the test, it must be able to write hive partitioned dataset. I propose to use arrow with param partition_cols instead of the duckdb write function which uses a COPY TO function under the hood. This function doesn't support hive partitioned write.

Yes exactly, we should just convert it to arrow and save (only if this partition key is specified). It is straightforward to convert duckdbdataframe to arrow table (as_arrow), and then save to parquet with partition.

Codecov Report

Merging #306 (b3e86b5) into master (281fe2a) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #306   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          101       102    +1     
  Lines         9480      9511   +31     
=========================================
+ Hits          9480      9511   +31     

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 281fe2a...b3e86b5. Read the comment docs.

It looks good you only have a linting issue left @LaurentErreca

I've been working on this issue. For duckdb to pass the test, it must be able to write hive partitioned dataset. I propose to use arrow with param partition_cols instead of the duckdb write function which uses a COPY TO function under the hood. This function doesn't support hive partitioned write.

Yes exactly, we should just convert it to arrow and save (only if this partition key is specified). It is straightforward to convert duckdbdataframe to arrow table (as_arrow), and then save to parquet with partition.

This is done by replacing:
if p.file_format not in self._format_save: self._fs.makedirs(os.path.dirname(uri), recreate=True) ldf = ArrowDataFrame(df.native.arrow())
with:
if (p.file_format not in self._format_save) or ("partition_cols" in kwargs): self._fs.makedirs(os.path.dirname(uri), recreate=True) ldf = ArrowDataFrame(df.native.arrow())

I've been working on this issue. For duckdb to pass the test, it must be able to write hive partitioned dataset. I propose to use arrow with param partition_cols instead of the duckdb write function which uses a COPY TO function under the hood. This function doesn't support hive partitioned write.

Yes exactly, we should just convert it to arrow and save (only if this partition key is specified). It is straightforward to convert duckdbdataframe to arrow table (as_arrow), and then save to parquet with partition.

This is done by replacing: if p.file_format not in self._format_save: self._fs.makedirs(os.path.dirname(uri), recreate=True) ldf = ArrowDataFrame(df.native.arrow()) with: if (p.file_format not in self._format_save) or ("partition_cols" in kwargs): self._fs.makedirs(os.path.dirname(uri), recreate=True) ldf = ArrowDataFrame(df.native.arrow())

I think you should use df.as_arrow() instead of df.native.arrow() because as_arrow is working on any dataframe. df.native assumes it is a duckdb relation, sometimes it is not.