Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support complex type in migrate procedure #17779

Merged
merged 1 commit into from
Jan 31, 2024
Merged

Support complex type in migrate procedure #17779

merged 1 commit into from
Jan 31, 2024

Conversation

chenjian2664
Copy link
Contributor

@chenjian2664 chenjian2664 commented Jun 7, 2023 •
edited by ebyhr
Loading

Description

Fix #17583

Additional context and related issues

https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/TableMetadata.java#L103 Will reassign columnId start from 1, we need to reset nextFieldId.

Release notes

(x) Release notes are required, with the following suggested text:

# Iceberg
* Add support for `array`, `map` and `row` types in migrate procedure. ({issue}`17583`)

@cla-bot cla-bot bot added the cla-signed label Jun 7, 2023
@chenjian2664 chenjian2664 requested a review from ebyhr June 7, 2023 00:33
ebyhr
ebyhr reviewed Jun 7, 2023
View reviewed changes
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a product test.

@github-actions github-actions bot added the iceberg Iceberg connector label Jun 7, 2023
@chenjian2664
Copy link
Contributor Author

Please add a product test.

Could you suggest how to represent the array, map, row value in TestIcebergProcedureCalls, I am confusing about this part

@chenjian2664 chenjian2664 self-assigned this Jun 7, 2023
@chenjian2664 chenjian2664 requested a review from ebyhr June 8, 2023 05:57
@chenjian2664 chenjian2664 requested a review from findinpath June 20, 2023 06:19
@chenjian2664
Copy link
Contributor Author

https://github.com/trinodb/trino/actions/runs/5319631815/jobs/9632630276?pr=17779#step:7:62380 Is the sth wrong with delta lake tests?

@findinpath
Copy link
Contributor

@chenjian2664 pls rebase on top of master to address the code conflicts.

@chenjian2664 chenjian2664 requested a review from findinpath July 3, 2023 23:13
@chenjian2664
Copy link
Contributor Author

chenjian2664 commented Jul 5, 2023 •
edited
Loading

It's strange that my commit is ignored by git..

@findinpath
Copy link
Contributor

@chenjian2664 pls push an empty commit.

@ebyhr / @findepi can you please do a check on this PR?

@github-actions github-actions bot added the mongodb MongoDB connector label Jul 5, 2023
@findinpath findinpath requested a review from findepi July 6, 2023 10:43
@findepi findepi requested a review from marcinsbd July 6, 2023 12:31
@findepi
Copy link
Member

findepi commented Jul 6, 2023

@marcinsbd PTAL

@chenjian2664
Copy link
Contributor Author

@findinpath @ebyhr @findepi Could you please take a look?

@chenjian2664
Copy link
Contributor Author

Does anyone can help on this? @findepi

@github-actions GitHub Actions
Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Jan 15, 2024
@mosabua
Copy link
Member

mosabua commented Jan 15, 2024

@ebyhr @findepi .. could you chime in here and potentially merge?

@github-actions github-actions bot removed the stale label Jan 16, 2024
ebyhr
ebyhr reviewed Jan 26, 2024
View reviewed changes
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good except for comments.

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMigrateProcedure.java Outdated Show resolved Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMigrateProcedure.java Outdated Show resolved Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMigrateProcedure.java Outdated Show resolved Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMigrateProcedure.java Outdated Show resolved Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMigrateProcedure.java Outdated Show resolved Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMigrateProcedure.java Outdated Show resolved Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMigrateProcedure.java Show resolved Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMigrateProcedure.java Show resolved Hide resolved
...no-product-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergProcedureCalls.java Outdated Show resolved Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMigrateProcedure.java Show resolved Hide resolved
@chenjian2664 chenjian2664 marked this pull request as draft January 26, 2024 10:35
@chenjian2664
Copy link
Contributor Author

chenjian2664 commented Jan 26, 2024 •
edited
Loading

Result mismatch

tests               | 2024-01-26 17:02:36 SEVERE: Failure cause:
tests               | java.lang.AssertionError: Could not find rows:
tests               | [1, [2, 3], {key=value}, {d=1}]
tests               | 
tests               | actual rows:
tests               | [1, [2, 3], {key=value}, {d=1}]
tests               | 	at io.trino.tests.product.iceberg.TestIcebergProcedureCalls.testMigrateHiveTableWithComplexType(TestIcebergProcedureCalls.java:133)
tests               | 	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
tests               | 	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
tests               | 	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
tests               | 	at org.testng.internal.Invoker.invokeMethod(Invoker.java:645)
tests               | 	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:851)
tests               | 	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1177)
tests               | 	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:129)
tests               | 	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:112)
tests               | 	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
tests               | 	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
tests               | 	at java.base/java.lang.Thread.run(Thread.java:1583)
tests               | 
tests               | 2024-01-26 17:02:36 INFO: [9 of 101] io.trino.tests.product.iceberg.TestIcebergProcedureCalls.testMigrateHiveTableWithSmallintType [ORC] (Groups: profile_specific_tests, iceberg)

@chenjian2664 chenjian2664 marked this pull request as ready for review January 29, 2024 10:24
@chenjian2664 chenjian2664 requested a review from ebyhr January 30, 2024 09:00
@chenjian2664 chenjian2664 marked this pull request as draft January 30, 2024 09:16
@chenjian2664 @findinpath @ebyhr
Support complex type in migrate procedure
c64e68e 
Co-Authored-By: Marius Grama <findinpath@gmail.com>
Co-Authored-By: Yuya Ebihara <6237050+ebyhr@users.noreply.github.com>
@chenjian2664 chenjian2664 marked this pull request as ready for review January 30, 2024 14:44
@chenjian2664 chenjian2664 requested a review from ebyhr January 31, 2024 02:12
@ebyhr ebyhr merged commit e98fa8f into trinodb:master Jan 31, 2024
49 checks passed
@github-actions github-actions bot added this to the 438 milestone Jan 31, 2024
@chenjian2664 chenjian2664 deleted the iceberg_complex_migrate branch January 31, 2024 02:42
@colebow colebow mentioned this pull request Jan 31, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebyhr ebyhr ebyhr approved these changes

@findepi findepi Awaiting requested review from findepi

@marcinsbd marcinsbd Awaiting requested review from marcinsbd

@findinpath findinpath Awaiting requested review from findinpath

Assignees

@chenjian2664 chenjian2664

Labels
cla-signed iceberg Iceberg connector mongodb MongoDB connector
Milestone
438
Development

Successfully merging this pull request may close these issues.

Migrated Iceberg tables return incorrect result on complex types
5 participants
@chenjian2664 @findinpath @findepi @mosabua @ebyhr