ARROW-7965: [Python] Refine higher level dataset API

[kszucs]

Provides a more intuitive way to construct nested dataset:

# instead of using confusing factory function
dataset([
     factory("s3://old-taxi-data", format="parquet"),
     factory("local/path/to/new/data", format="csv")
])

# let the user to construct a new dataset directly from dataset objects
dataset([ 
    dataset("s3://old-taxi-data", format="parquet"),
    dataset("local/path/to/new/data", format="csv")
])

In the future we might want to introduce a new Dataset class which wraps functionality of both the dataset actory and the materialized dataset enabling optimizations over rediscovery of already materialized datasets.

[github-actions]

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

[jorisvandenbossche]

That makes the usage indeed a bit easier. But I am wondering: how expensive is "finishing" the factory? As now you are finishing all sub-datasets, just to reuse the non-finished factory later one.

[kszucs]

Yeah, that's a downside until we wrap both the DatasetFactory and Dataset with a single class which does the discovery lazily.

[nealrichardson]

I agree with the goal here, but I wonder if the solution should perhaps be in C++? That way we wouldn't have to reimplement this in R too.

[kszucs]

I agree with the goal here, but I wonder if the solution should perhaps be in C++? That way we wouldn't have to reimplement this in R too.

cc @bkietz

[jorisvandenbossche]

If pushing into C++, that will have the same problem of the double cost of the dataset discovery

[kszucs]

@jorisvandenbossche in order to reuse the inspected schemas AFAICS we need "cache" them in the C++ instances and return them from InspectSchemas() unless we request an explicit re-discovery.

[jorisvandenbossche]

Trying to put in words a bit more structured what I said yesterday in the meeting:

Right now, we need to keep a reference to the dataset factories to be able to reuse those factories, since UnionDatasetFactory is written to expect a vector of factories.

But instead of fixing this on the dataset side by holding this reference, can we change the implementation of UnionDatasetFactory to actually accepts a vector of datasets instead of a vector of factories?

Looking at the UnionDatasetFactory implementation, it doesn't look like that still needs the schema's of all files/fragments of its sub-datasets. Rather, it only needs the finished schema of its sub-datasets (and then it checks if those are compatible / can be unified). But assuming that those sub-datasets would also created with a factory, the schema returned from child_factory->Inspect() will be the same as the schema from the finished dataset from that child_factory.

[jorisvandenbossche]

OK, I wanted to put this in some pseudo python code to explain it better, which lead me to see that my suggestion is right now indeed not possible ;)

The current logic:

def UnionDatasetFactory(factories):
    # inspect
    schemas = []
    for fact in factories:
        schemas.append(fact.inspect())
    schema = UnifySchemas(schemas)

    # finish
    datasets = []
    for fact in factories:
        datasets.append(fact.finish(schema))
    return UnionDataset(datasets, schema)

The issue that makes it not possible right now to pass a vector of datasets, is that the sub-datasets need to be finished with a potentially different (unified) schema.
So my question is then: would it be possible to kind of create a new "view" on an existing dataset with a different (but compatible!) schema? In the end, you could create a new Dataset passing all building blocks of the original dataset (the format, filesystem, forest, file_partitions) but with only specifying a different schema?

[jorisvandenbossche]

@bkietz @fsaintjacques with your deeper knowledge of the C++ dataset code, could you confirm or not whether my idea above is possible: having a UnionDataset constructor taking a vector of Dataset of objects instead of vector of DatasetFactories ?

[bkietz]

@jorisvandenbossche this is already supported in C++, but currently the schemas must be identical rather than merely compatible. Creating a view of a dataset with differing schema would probably be straightforward, created https://issues.apache.org/jira/browse/ARROW-8164 to track this feature

[jorisvandenbossche]

Thanks for opening the issue!

[wesm]

What's the status of this patch? It's still in the 0.17.0 backlog

[jorisvandenbossche]

This is blocked by #6721 (which is blocked by a strange R failure, I think).

I added it to the 0.17 milestone, as it it would be nice to get this in, as I noted in the JIRA:

This depends on ARROW-8164, but if that gets merged quickly, it would be nice tackle this issue for 0.17 since that would enable us to remove factory() from the high-level user API (which wasn't there yet in 0.16, so this would avoid it ever being in a released version)

But it is also certainly not a blocker, since the datasets API is still experimental anyway.

[nealrichardson]

#6721 has been merged, so this is unblocked

[jorisvandenbossche]

Thanks a lot for this PR! The diff is a bit hard to interpret, but generally looks good to me.

One thing I am not really sure about is the string URI value to specify the filesystem. What's the use for that? If you have a URI, just pass it to the source? This seems a needless complication to me, and not something we support elsewhere? (or do we?)

[kszucs]

@github-actions crossbow submit conda-win-vs2015-py36

[github-actions]

Revision: dacded6

Submitted crossbow builds: ursa-labs/crossbow @ actions-90

Task	Status
conda-win-vs2015-py36

[kszucs]

@github-actions crossbow submit conda-win-vs2015-py36

[github-actions]

Revision: 5692edc

Submitted crossbow builds: ursa-labs/crossbow @ actions-111

Task	Status
conda-win-vs2015-py36

[jorisvandenbossche]

Failures are due to "ignore_prefix" -> "selector_ignore_prefix" rename on the C++ side.

Since all Ursabot python builds are passing, it also seems they are not running any of the parquet or dataset related tests ..

[kszucs]

@github-actions crossbow submit conda-win-vs2015-py36

[github-actions]

Revision: acbe2cb

Submitted crossbow builds: ursa-labs/crossbow @ actions-113

Task	Status
conda-win-vs2015-py36

[kszucs]

Failures are due to "ignore_prefix" -> "selector_ignore_prefix" rename on the C++ side.

Updated.

Since all Ursabot python builds are passing, it also seems they are not running any of the parquet or dataset related tests ..

Yes, but we're not actively maintaining the ursabot builds now.

[pitrou]

I only skimmed through this, a few comments.

[jorisvandenbossche]

Can be for a follow-up JIRA, but we might want to move this helper to pyarrow.fs, and use some this also in other places where we accept filesystems? (although I actually don't know if there are already many such places ..)

[jorisvandenbossche]

@kszucs can you also add this patch to this branch:

--- a/python/pyarrow/tests/test_parquet.py
+++ b/python/pyarrow/tests/test_parquet.py
@@ -2490,13 +2490,7 @@ def _assert_dataset_paths(dataset, paths, use_legacy_dataset):
         assert set(map(str, paths)) == {x.path for x in dataset.pieces}
     else:
         paths = [str(path.as_posix()) for path in paths]
-        if hasattr(dataset._dataset, 'files'):
-            assert set(paths) == set(dataset._dataset.files)
-        else:
-            # UnionDataset
-            # TODO(temp hack) remove this branch once ARROW-7965 is in (which
-            # will change this to a FileSystemDataset)
-            assert dataset.read().num_rows == 50
+        assert set(paths) == set(dataset._dataset.files)

(that's a clean-up for a hack I added yesterday becuase of a list of paths giving a UnionDataset, which this PR is fixing)

[pitrou]

Thank you. A few more comments.

[kszucs]

Thanks Joris, Ben, Antoine! Merging.