Test Trino/Spark compatibility for storage formats#8751
Conversation
alexjo2144
left a comment
alexjo2144
left a comment
There was a problem hiding this comment.
Nits/suggesions, but looks good to me
...roduct-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergSparkCompatibility.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Style suggestion, maybe specify Trino support using a constructor argument, like AVRO(false) ?
There was a problem hiding this comment.
thought about that, but that would be much more code to write. i chose simpler version
|
CI hit #8719 |
phd3
left a comment
phd3
left a comment
There was a problem hiding this comment.
@findepi you beat me to it, I had made the same changes locally to address #7180 (comment) 😁
LGTM % @alexjo2144's comments
...roduct-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergSparkCompatibility.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This is overkill. Iceberg by specification only has three formats. Hard-code this test to Avro and don't clutter TestingIcebergStorageFormat with supported/unsupported. This will make it easy to remove when we support Avro (which should be soon).
There was a problem hiding this comment.
This is specifically to self-test the unsupportedStorageFormats, otherwise we may easily forget to update the @DataProviders here when adding AVRO support.
There was a problem hiding this comment.
You could solve this by adding a single TODO comment to this test to remove this test and update TestingStorageFormat when Avro is added. This test will fail at that point. No need to complicate all the code.
There was a problem hiding this comment.
No need to complicate all the code.
the only "complexity" is here, and i don't quite understand why we're calling this a complexity at all.
i specifically named the other factory method "storageFormats", so that no change is needed when we add Avro support and can remove "unsupportedStorageFormats".
...roduct-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergSparkCompatibility.java
Outdated
Show resolved
Hide resolved
...roduct-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergSparkCompatibility.java
Outdated
Show resolved
Hide resolved
...roduct-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergSparkCompatibility.java
Outdated
Show resolved
Hide resolved
...roduct-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergSparkCompatibility.java
Outdated
Show resolved
Hide resolved
findepi
force-pushed
the
findepi/test-trino-spark-compatibility-for-storage-formats-f3f073
branch
2 times, most recently
from
23d621a to
2cb2a6d
Compare
|
thanks for review. AC, and also added test coverage for decimal (both directions), and timestamp with time zone (Trino writing, Spark reading; the other direction was already tested). |
findepi
force-pushed
the
findepi/test-trino-spark-compatibility-for-storage-formats-f3f073
branch
from
2cb2a6d to
e4d2e78
Compare
findepi
force-pushed
the
findepi/test-trino-spark-compatibility-for-storage-formats-f3f073
branch
from
e4d2e78 to
31318da
Compare
|
Rebased. All comments have been addressed. Will merge once the tests pass. |
We already have Spark compatibility tests for Hive and Iceberg connectors, so distinguished test class names will be helpful.
Iceberg `timestamptz` is mapped to `timestamp with time zone` in Trino and to `timestamp` in Spark. We already have a test writing Spark `timestamp` and reading in Trino. This commit adds a test writing `timestamp with time zone` in Trino, and reading in Spark.
findepi
force-pushed
the
findepi/test-trino-spark-compatibility-for-storage-formats-f3f073
branch
from
31318da to
9d4f434
Compare
findepi
deleted the
findepi/test-trino-spark-compatibility-for-storage-formats-f3f073
branch
4 participants
cc @joshthoward