API: Fix inconsistent TimeTransform Type #6482
Conversation
|
I am new to iceberg. Can anyone give approval to run the CI? |
| table.spec()); | ||
|
|
||
| // remove and add a field with TimeTransform(Years, Months, Days, Hours) | ||
| table.updateSpec().removeField("year_field_year").addField(year("year_field")).commit(); |
There was a problem hiding this comment.
Do we need to add a case that remove year_field_year first and then add year_field_year after the deletion is successful?
table.updateSpec().removeField("year_field_year").commit();
assert……
table.updateSpec().addField(year("year_field")).commit();
There was a problem hiding this comment.
I don't think we need this case. Other tests have include the case, eg. testAddAfterLastFieldRemoved
core/src/test/java/org/apache/iceberg/TestTableUpdatePartitionSpec.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/iceberg/TestTableUpdatePartitionSpec.java
Outdated
Show resolved
Hide resolved
| .year("year_field") | ||
| .build(), | ||
| table.spec()); | ||
| V2Assert.assertEquals( |
There was a problem hiding this comment.
Again, these two tests are essentially the same for me, right? Please correct me if I missed something.
There was a problem hiding this comment.
-
test1: adding a year field to the spec(
table.updateSpec().addField(year("year_field")).commit();)
I want to verify if the actual table transform type is same asPartitionSpec.builderForand the transform type class should beorg.apache.iceberg.transforms.Years. If not same, the test2 will also faill. -
test2:
table.updateSpec().removeField("year_field_year").addField(year("year_field")).commit();
Because i have encountered exception when removed and then add a same field, i want to check if this could work after the fix. To be exact, test2 is a e2e test.
There was a problem hiding this comment.
Oh, You mean V1Assert.assertEquals and V2Assert.assertEquals? I just follow other tests pattern and i think there maybe have some difference bettewn v2 and v1 formation, but i have not make more research.
|
Sorry for long time to come back. Will address comments as soon as :) @hililiwei |
zhangbutao
force-pushed
the
fix_timetranform_inconsistency
branch
from
1ac0018 to
891c820
Compare
| sourceColumn.fieldId(), | ||
| nextFieldId(), | ||
| targetName, | ||
| Transforms.year(sourceColumn.type())); |
There was a problem hiding this comment.
I'm hesitant to remove those because then there is a situation where we don't know the sourceColumn.
|
Thanks for letting me know and creating the PR in the first place, much appreciated 👍🏻 |
5 participants
After #5601 and #6220, there is inconsistent
TimeTransform Typein some codes. This causes an exception when remove and then add a same time partition(year, month, day, hour).I added a UT in this pr, and you can use the UT to reproduce this issue. This issue existed in iceberg1.1.0 and it blocked me upgrade iceberg from 1.0.0 to 1.1.0 in Hive repo. apache/hive#3851 and failed tests http://ci.hive.apache.org/blue/organizations/jenkins/hive-precommit/detail/PR-3851/1/tests