Add pyiceberg DataFusion e2e test #825

gruuya · 2024-12-18T14:51:39Z

Besides serving as an e2e test, this also excessive quite a bit of type conversion logic.

In addition it demonstrates the issue described in #813.

Fokko · 2024-12-18T18:37:05Z

crates/integration_tests/testdata/pyiceberg/load_types_table.py

+import pyarrow.parquet as pq
+
+# Generate a table with various types in memory and dump to a Parquet file
+ctx = SessionContext()


I'm reluctant to let Datafusion generate these files, for two reasons:

These files are imported into an Iceberg table and are missing things like Field-IDs etc.

I'd rather depend on purely Spark which uses the Java SDK underneath, which is more complete than PyIceberg. The Java impl is considered the reference implementation.

missing things like Field-IDs etc.

If by that you mean PARQUET:field_id note that pyiceberg actualy decorates the fields during table creation with those, I've just pushed an addendum to the tests that demonstrate this.

Granted I'm not sure about the state of pyiceberg compliance with the protocol.

Would you be more open to accepting this test if I switched to Java SDK for generating the Parquet files and creating the table?

I also see value in this, let's hear what others think!

Generally i like the idea of using pyiceberg for testing, maybe also alongside spark. This can help facilitate cross platform interoperability,
but im not sure when we would use one versus the other.

Alright just an update—I've tried doing the same with pyspark but it seems it is being very conservative and either rejecting some types outright (e.g. Illegal Parquet type: INT64 (TIMESTAMP(NANOS,false))), or coercing them on its own.

For instance if I just supply it with a Parquet file like type_test.parquet (output of load_types_table.py) with type hints

message arrow_schema { optional boolean cboolean; optional int32 cint8 (INTEGER(8,true)); optional int32 cint16 (INTEGER(16,true)); optional int32 cint32; optional int64 cint64; ...

and do

parquet_df = spark.read.parquet("types_test.parquet") parquet_df.writeTo("rest.default.types_test").using("iceberg").create()

the resulting data parquet files have incompatible type hints removed

message table { optional boolean cboolean = 1; optional int32 cint8 = 2; optional int32 cint16 = 3; optional int32 cint32 = 4; optional int64 cint64 = 5; ...

TLDR: I can't seem to replicate the issue in #814 with pyspark, though it could still be used to test out the non-corner cases.

but im not sure when we would use one versus the other.

I think there's definitely merit in having an integration types test like the one proposed here. The basic one can be done with pyspark, though it might be a bonus (or not) that apparently pyiceberg can hit some corner cases which pyspark can't.

(fwiw, this is something that we ran into in our dev work, so it's not a end-user-reported issue.)

apparently pyiceberg can hit some corner cases which pyspark can't.

Interesting! curious to hear about what you guys ran into.

I think Pyspark is good for integration tests through the spark API. PyIceberg allows more granular test cases since you can work with iceberg constructs

curious to hear about what you guys ran into.

Actually the only one so far seems to be this Iceberg-from-parquet creation discussed above—if the Parquet file has e.g. Int32 field with Int16 type hints spark will automatically coerce those in the resulting data file into Int32, whereas pyiceberg will leave it as is (and so during the scan in the actual record batches we get Int16, whilst the schema says it's Int32).

Ok, I've opened an alternative PR that adds an integration test starting off from spark in #850.

Granted this doesn't hit the original issue #813, but could still be worthwhile.

crates/integration_tests/testdata/pyiceberg/load_types_table.py

kevinjqliu · 2024-12-19T20:24:20Z

crates/integration_tests/testdata/pyiceberg/load_types_table.py

+import pyarrow.parquet as pq
+
+# Generate a table with various types in memory and dump to a Parquet file
+ctx = SessionContext()


Generally i like the idea of using pyiceberg for testing, maybe also alongside spark. This can help facilitate cross platform interoperability,
but im not sure when we would use one versus the other.

crates/integration_tests/testdata/pyiceberg/Dockerfile

kevinjqliu

I think E2E test should be pyiceberg writes and reading with datafusion. similar to #850

other than that, generally lgtm!

kevinjqliu · 2025-01-04T00:40:54Z

crates/integration_tests/testdata/pyiceberg/provision.py

+# Generate a table with various types in memory and dump to a Parquet file
+ctx = SessionContext()
+ctx.sql("""
+CREATE TABLE types_test (


lets generate these values in pyiceberg/pyarrow. it'll help us figure out the supportability of each types which is where the E2E test is useful.

Here are the supported types we use in pyiceberg's integration test
https://github.com/apache/iceberg-python/blob/acd6f5a8a19db709e835e2686b87d4db3dca254f/tests/conftest.py#L304-L349

Sounds good, I just switched to pyarrow to generate the Parquet file from which the table is constructed.

To be clear, if #850 gets accepted and merged, and if folks don't feel comfortable including pyiceberg in the testing scope for now, then this PR can just remain as an example for the issue in #813 (it still exercises it as demonstrated at the very end of the integration test).

…g it with DataFusion

…n test fixture

…on table

kevinjqliu

LGTM! I like this E2E test from pyiceberg writes to datafusion read.
In the future, I want to expose the IcebergTableProvider as python binding so we can also use the datafusion python library to query (#865)

kevinjqliu · 2025-01-04T18:09:33Z

crates/integration_tests/testdata/pyiceberg/Dockerfile

+
+FROM python:3.9-bullseye
+
+RUN pip install pyiceberg[pyarrow]==0.8


nit: 0.8.1 is the latest version

kevinjqliu · 2025-01-04T18:11:38Z

crates/integration_tests/testdata/pyiceberg/provision.py

+    identifier=f"default.types_test",
+    schema=parquet_file.schema,
+)
+iceberg_table.append(df=parquet_file)


iceberg_table.append(table) also works, do we need to roundtrip writing to parquet and then reading it back?

Oh I thought only appending via a parquet file reproduces #813, but turns out so does appending a pyarrow table directly; nice catch!

kevinjqliu · 2025-01-04T18:13:10Z

crates/integration_tests/testdata/pyiceberg/provision.py

+    ('ctimestamptz', pa.timestamp('us', tz='UTC')),
+    ('cutf8', pa.utf8()),
+    ('cbinary', pa.binary()),
+])


do we want to include other data types? including complex types? like in https://github.com/apache/iceberg-python/blob/acd6f5a8a19db709e835e2686b87d4db3dca254f/tests/conftest.py#L304-L349

Makes sense to me, could be valuable to include list/map/struct.

kevinjqliu

LGTM!

kevinjqliu · 2025-01-05T19:44:39Z

crates/integration_tests/tests/datafusion.rs

+    ];
+    assert_batches_eq!(expected, &batches);
+
+    // TODO: this isn't OK, and should be fixed with https://github.com/apache/iceberg-rust/issues/813


should we wait for this fix?

gruuya force-pushed the datafusion-integration-test branch from 64c1473 to 3a328ed Compare December 18, 2024 14:51

gruuya mentioned this pull request Dec 18, 2024

Reported and actual arrow schema of the table can be different #813

Open

gruuya force-pushed the datafusion-integration-test branch 2 times, most recently from 1b03c42 to c261ddf Compare December 18, 2024 14:56

Fokko reviewed Dec 18, 2024

View reviewed changes

kevinjqliu reviewed Dec 19, 2024

View reviewed changes

gruuya changed the title ~~Add an e2e test for writing an Iceberg table with pyiceberg and reading it with DataFusion~~ Add pyiceberg DataFusion e2e test Dec 26, 2024

gruuya mentioned this pull request Dec 26, 2024

Add pyspark DataFusion integration test #850

Open

kevinjqliu reviewed Jan 4, 2025

View reviewed changes

gruuya added 3 commits January 4, 2025 08:43

Add a e2e test for writing an Iceberg table with pyiceberg and readin…

3afd6f0

…g it with DataFusion

Add asserts for the pyiceberg resulting arrow schema

1a77faa

Use pyarrow as req of pyiceberg when installing it for the integratio…

451c5cc

…n test fixture

gruuya force-pushed the datafusion-integration-test branch from 64990ba to 5cb32a9 Compare January 4, 2025 09:23

Switch to pyarrow for Parquet file generation for the types integrati…

ba81d13

…on table

gruuya force-pushed the datafusion-integration-test branch from 5cb32a9 to ba81d13 Compare January 4, 2025 09:34

kevinjqliu previously approved these changes Jan 4, 2025

View reviewed changes

kevinjqliu requested a review from Fokko January 4, 2025 18:15

Directly append pyarrow table to types_test fixture

dcfed20

gruuya dismissed kevinjqliu’s stale review via dcfed20 January 5, 2025 07:38

kevinjqliu approved these changes Jan 5, 2025

View reviewed changes

kevinjqliu reviewed Jan 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pyiceberg DataFusion e2e test #825

Add pyiceberg DataFusion e2e test #825

gruuya commented Dec 18, 2024

Fokko Dec 18, 2024

gruuya Dec 18, 2024

Fokko Dec 18, 2024

kevinjqliu Dec 19, 2024

gruuya Dec 19, 2024

gruuya Dec 19, 2024

kevinjqliu Dec 20, 2024

gruuya Dec 21, 2024

gruuya Dec 26, 2024

kevinjqliu Dec 19, 2024

kevinjqliu left a comment

kevinjqliu Jan 4, 2025

gruuya Jan 4, 2025

kevinjqliu left a comment

kevinjqliu Jan 4, 2025

kevinjqliu Jan 4, 2025

gruuya Jan 5, 2025

kevinjqliu Jan 4, 2025

gruuya Jan 5, 2025

kevinjqliu left a comment

kevinjqliu Jan 5, 2025


		FROM python:3.9-bullseye

		RUN pip install pyiceberg[pyarrow]==0.8

Add pyiceberg DataFusion e2e test #825

Are you sure you want to change the base?

Add pyiceberg DataFusion e2e test #825

Conversation

gruuya commented Dec 18, 2024

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

Choose a reason for hiding this comment

kevinjqliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinjqliu 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

kevinjqliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment