iceberg-python icon indicating copy to clipboard operation
iceberg-python copied to clipboard

[Bug] Cannot use PyIceberg with multiple FS

Open kevinjqliu opened this issue 1 year ago • 15 comments

Apache Iceberg version

main (development)

Please describe the bug 🐞

PyIceberg assumes the same FS implementation is used for reading both metadata and data. However, I want to use a catalog with local FS as the warehouse while referencing S3 files as data.

See this example Jupyter notebook to reproduce

Problem

The fs implementation is determined by metadata location, which is then passed down to the function which reads the data file. https://github.com/apache/iceberg-python/blob/d8b5c17cadbc99e53d08ade6109283ee73f0d83e/pyiceberg/io/pyarrow.py#L1428-L1430

Possible solution

Determine fs implementation based on the file path of the current file

kevinjqliu avatar Aug 11 '24 17:08 kevinjqliu

This is a good point, I've heard that folks store their metadata on HDFS, and the data itself on S3.

I don't think the example with the add-files is the best, it would be better to support the write.data.path and write.metadata.path configuration

Fokko avatar Aug 20 '24 08:08 Fokko

Oh interesting, thanks! Here's the config definition for write.data.path and write.metadata.path https://iceberg.apache.org/docs/latest/configuration/#write-properties

I also found an old issue referencing this #161

Another example usage is to use both local FS and S3(minio), which might be easier to set up and test against

kevinjqliu avatar Sep 01 '24 15:09 kevinjqliu

I will have a look this issue.

TiansuYu avatar Sep 02 '24 08:09 TiansuYu

A generic question: why we are implementing a custom https://github.com/apache/iceberg-python/blob/dc6d2429aafbffc626cba53aaac3f6198fc37eb3/pyiceberg/io/init.py#L320-L329

https://github.com/apache/iceberg-python/blob/dc6d2429aafbffc626cba53aaac3f6198fc37eb3/pyiceberg/io/init.py#L290-L300

instead of using fsspec.core.url_to_fs(file_path)[0] directly?

(On a side note: this looks a bit confusing to me, as why for gs, FSSPEC_FILE_IO is not added as a viable io method as we have added gcfs into extras.)

(Also I am not sure why pyarrow is not using fsspec as io layer but implement things on their own.)

EDIT: https://filesystem-spec.readthedocs.io/en/latest/api.html#fsspec.core.url_to_fs is a slightly better choice than fsspec.core.get_fs_token_path

TiansuYu avatar Sep 02 '24 11:09 TiansuYu

Read my comment here for the cause of the issue.

I dont think fixing SqlCatalog alone is the proper answer to this bug. The io layer seems to me ill written and has to be fixed somewhere in the uppper level (e.g. FsspecInputFile or InputFile).

Let me know what do you think, then we can come up with a way to properly address this. 😄

TiansuYu avatar Sep 02 '24 11:09 TiansuYu

Thanks for taking a look at this @TiansuYu

why we are implementing a custom

I think custom scheme parsing avoids picking one library over another (fsspec vs pyarrow). fsspec.core.get_fs_token_paths seems like an interesting replacement when using fsspec.

(On a side note: this looks a bit confusing to me, as why for gs, FSSPEC_FILE_IO is not added as a viable io method as we have added gcfs into extras.)

Good catch, fsspec should be included since GCS is supported https://github.com/apache/iceberg-python/blob/dc6d2429aafbffc626cba53aaac3f6198fc37eb3/pyiceberg/io/fsspec.py#L161-L176

(Also I am not sure why pyarrow is not using fsspec as io layer but implement things on their own.)

pyarrow is using its own native fs implementations https://github.com/apache/iceberg-python/blob/dc6d2429aafbffc626cba53aaac3f6198fc37eb3/pyiceberg/io/pyarrow.py#L348-L403

kevinjqliu avatar Sep 02 '24 13:09 kevinjqliu

I dont think fixing SqlCatalog alone is the proper answer to this bug. The io layer seems to me ill written and has to be fixed somewhere in the uppper level (e.g. FsspecInputFile or InputFile).

yea, the main issue is the assumption that the same io (and fs implementation) is used for reading both data and metadata files. The example you pointed to pass in the io parameter

https://github.com/apache/iceberg-python/blob/dc6d2429aafbffc626cba53aaac3f6198fc37eb3/pyiceberg/table/init.py#L655-L657

Instead, we would want to recreate io/fs based on the file currently being processed.

Here's another example of passing in the io parameter on the write path https://github.com/apache/iceberg-python/blob/dc6d2429aafbffc626cba53aaac3f6198fc37eb3/pyiceberg/table/init.py#L530-L532

kevinjqliu avatar Sep 02 '24 13:09 kevinjqliu

Generally, this problem should go away if we re-evaluate fs and io each time a file is read and written. Or other words, we should stop passing the io parameter around.

kevinjqliu avatar Sep 02 '24 13:09 kevinjqliu

@kevinjqliu I think resolving fs at file level should make the API cleaner. We can e.g. if no file_system given to FsspecInputFile (or similarly PyarrowInputFile), then we resolve them at file level.

I would say one benefit one might want to set fs on table level is to reuse that fs instance for performance boost. If we want to keep this, I would say we need to make two io configs, one for metadata, one for data, on the MetastoreCatalog or Catalog level.

TiansuYu avatar Sep 02 '24 13:09 TiansuYu

My preference is resolving fs at the file level. It's more flexible and the performance difference should be negligible. Another reason is to be able to write data across clouds. Technically, I can write to multiple clouds, across AWS, Azure, and GCP.

kevinjqliu avatar Sep 02 '24 13:09 kevinjqliu

I will make a PR according to this:

My preference is resolving fs at the file level. It's more flexible and the performance difference should be negligible. Another reason is to be able to write data across clouds. Technically, I can write to multiple clouds, across AWS, Azure, and GCP.

TiansuYu avatar Sep 02 '24 13:09 TiansuYu

Also reading on here: https://arrow.apache.org/docs/python/filesystems.html#using-arrow-filesystems-with-fsspec

There might be some opportunity that we can simplify the split between arrow and fsspec file_system.

TiansuYu avatar Sep 02 '24 13:09 TiansuYu

yep! There are definitely opportunities to consolidate the two. I opened #310 with some details.

kevinjqliu avatar Sep 02 '24 14:09 kevinjqliu

Reading on table spec, I just realised that there is a field location in https://iceberg.apache.org/spec/#table-metadata-fields that specifies a base location of the table. Does Iceberg Java actually allows the split between metadata and data location?

TiansuYu avatar Sep 02 '24 14:09 TiansuYu

Its configurable via the write properties. See this comment https://github.com/apache/iceberg-python/issues/1041#issuecomment-2323380629

kevinjqliu avatar Sep 02 '24 14:09 kevinjqliu

Any updates on this issue , I face a similar issue when creating a table on S3 as well fyi - I think this will prevent from making pyarrow the default io system FSSPEC_FILE_IO = "pyiceberg.io.fsspec.FsspecFileIO" catalog = load_catalog(name='glue', **{'type': 'glue', 'py-io-impl': FSSPEC_FILE_IO})

Sairam90 avatar Dec 20 '24 15:12 Sairam90

Hey guys, I can pick this up together with #1279 if no one is currently working on this.

jiakai-li avatar Dec 20 '24 22:12 jiakai-li

assigned to you @jiakai-li

kevinjqliu avatar Dec 20 '24 22:12 kevinjqliu

@kevinjqliu I guess we can close this issue and #1279 now? At the meantime, I'm keen to work on the write.data.path and write.metadata.path if that's something we want to enable and no one else is currently working on it?

jiakai-li avatar Jan 07 '25 03:01 jiakai-li

@jiakai-li we can close this issue! Fixed by #1453

At the meantime, I'm keen to work on the write.data.path and write.metadata.path if that's something we want to enable and no one else is currently working on it?

Let's open a new issue for those https://github.com/apache/iceberg-python/issues/1492

kevinjqliu avatar Jan 07 '25 15:01 kevinjqliu