test: Add native Iceberg partition transform test cases#26889
test: Add native Iceberg partition transform test cases#26889PingLiuPing wants to merge 1 commit intoprestodb:masterfrom
Conversation
Reviewer's GuideAdds a comprehensive suite of native Iceberg partition transform tests and supporting utilities, including a shared IcebergPartitionTestBase, several focused TestNG test classes for transforms, partitioned writes, schema evolution, and TPCH-based partitioning, plus a new test-scope Hadoop dependency for native execution tests. Class diagram for new Iceberg native partition test suiteclassDiagram
class IcebergPartitionTestBase
class TestPartitionTransforms
class TestPartitionedWrite
class TestSchemaEvolution
class TestTpchPartitionTransforms
IcebergPartitionTestBase <|-- TestPartitionTransforms
IcebergPartitionTestBase <|-- TestPartitionedWrite
IcebergPartitionTestBase <|-- TestSchemaEvolution
IcebergPartitionTestBase <|-- TestTpchPartitionTransforms
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
8adedc8 to
8652bcd
Compare
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Several helper methods swallow exceptions silently (e.g., dropTableOnJavaRunner and cleanupTables in TestTpchPartitionTransforms); consider at least logging the exception or narrowing the catch to avoid hiding real failures during test setup/teardown.
- There is some duplicated SQL DDL/insert string construction across tests (e.g., in TestPartitionedWrite and TestTpchPartitionTransforms); extracting common helpers or templates in the base class could make future changes to schemas/partitioning less error‑prone.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several helper methods swallow exceptions silently (e.g., dropTableOnJavaRunner and cleanupTables in TestTpchPartitionTransforms); consider at least logging the exception or narrowing the catch to avoid hiding real failures during test setup/teardown.
- There is some duplicated SQL DDL/insert string construction across tests (e.g., in TestPartitionedWrite and TestTpchPartitionTransforms); extracting common helpers or templates in the base class could make future changes to schemas/partitioning less error‑prone.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
8652bcd to
55ab559
Compare
hantangwangd
left a comment
There was a problem hiding this comment.
@PingLiuPing thanks for adding these tests. One question: do the tests about varbinary need to wait until facebookincubator/velox#15882 is landed into Prestissimo's Velox before they can pass?
| import static com.facebook.presto.testing.assertions.Assert.assertEquals; | ||
| import static java.lang.String.format; | ||
|
|
||
| public class TestPartitionedWrite |
There was a problem hiding this comment.
| public class TestPartitionedWrite | |
| @Test(singleThreaded = true) | |
| public class TestPartitionedWrite |
nit: the test cases in this class may fail in a concurrent execution environment.
|
|
||
| import static java.lang.String.format; | ||
|
|
||
| public class TestSchemaEvolution |
There was a problem hiding this comment.
| public class TestSchemaEvolution | |
| @Test(singleThreaded = true) | |
| public class TestSchemaEvolution |
The same as above.
Thank you for the comment, [facebookincubator/velox#15882] has been merged, and I see velox just been advanced today, I will rebase the code. |
Description
Add iceberg partition transform testcases.
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.