You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "namrathamyske (via GitHub)" <gi...@apache.org> on 2023/01/23 18:59:12 UTC
[GitHub] [iceberg] namrathamyske opened a new pull request, #6650: Core, API: BaseReplacePartitions to branch test Refactoring
namrathamyske opened a new pull request, #6650:
URL: https://github.com/apache/iceberg/pull/6650
follow up from https://github.com/apache/iceberg/pull/5234, to refactoring minor comments left in last PR. @rdblue @amogh-jahagirdar please take a look at this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 merged pull request #6650: Core, API: BaseReplacePartitions to branch test Refactoring
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 merged PR #6650:
URL: https://github.com/apache/iceberg/pull/6650
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #6650: Core, API: BaseReplacePartitions to branch test Refactoring
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #6650:
URL: https://github.com/apache/iceberg/pull/6650#discussion_r1121154697
##########
core/src/main/java/org/apache/iceberg/util/SnapshotUtil.java:
##########
@@ -394,4 +396,42 @@ public static Schema schemaFor(Table table, Long snapshotId, Long timestampMilli
return table.schema();
}
+
+ /**
+ * Fetch the snapshot at the head of the given branch in the given table.
+ *
+ * <p>This method calls {@link Table#currentSnapshot()} instead of using branch API {@link
+ * Table#snapshot(String)} for the main branch to ensure existing tests still goes through the old
Review Comment:
good point, I can rephrase this to avoid saying tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] amogh-jahagirdar commented on a diff in pull request #6650: Core, API: BaseReplacePartitions to branch test Refactoring
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #6650:
URL: https://github.com/apache/iceberg/pull/6650#discussion_r1120746475
##########
core/src/test/java/org/apache/iceberg/TestOverwrite.java:
##########
@@ -182,16 +182,16 @@ public void testOverwriteFailsDelete() {
@Test
public void testOverwriteWithAppendOutsideOfDelete() {
TableMetadata base = TestTables.readMetadata(TABLE_NAME);
- long baseId =
- latestSnapshot(base, branch) == null ? -1 : latestSnapshot(base, branch).snapshotId();
+ Snapshot latestSnapshot = latestSnapshot(base, branch);
Review Comment:
We don't need to handle in this PR but given #6651 we will end up using the SnapshotUtil api which is introduced. We can either update this PR with the proposed utility API since there's consensus on that or just later refactor to use the utility API when it gets merged as part of branch writes. Since this is a test class, I'm okay either way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] amogh-jahagirdar commented on a diff in pull request #6650: Core, API: BaseReplacePartitions to branch test Refactoring
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #6650:
URL: https://github.com/apache/iceberg/pull/6650#discussion_r1120746475
##########
core/src/test/java/org/apache/iceberg/TestOverwrite.java:
##########
@@ -182,16 +182,16 @@ public void testOverwriteFailsDelete() {
@Test
public void testOverwriteWithAppendOutsideOfDelete() {
TableMetadata base = TestTables.readMetadata(TABLE_NAME);
- long baseId =
- latestSnapshot(base, branch) == null ? -1 : latestSnapshot(base, branch).snapshotId();
+ Snapshot latestSnapshot = latestSnapshot(base, branch);
Review Comment:
We don't need to handle in this PR but given #6651 we will end up using the SnapshotUtil api which is introduced. We can either update this PR with the proposed utility API or just later refactor to use the utility API. Since this is a test class, I'm okay either way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #6650: Core, API: BaseReplacePartitions to branch test Refactoring
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on PR #6650:
URL: https://github.com/apache/iceberg/pull/6650#issuecomment-1449434428
Merging since all comments are addressed, thanks for the review @yyanyy and @amogh-jahagirdar!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #6650: Core, API: BaseReplacePartitions to branch test Refactoring
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on PR #6650:
URL: https://github.com/apache/iceberg/pull/6650#issuecomment-1448987284
@namrathamyske I updated based on Amogh's comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #6650: Core, API: BaseReplacePartitions to branch test Refactoring
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #6650:
URL: https://github.com/apache/iceberg/pull/6650#discussion_r1120748082
##########
core/src/test/java/org/apache/iceberg/TestOverwrite.java:
##########
@@ -182,16 +182,16 @@ public void testOverwriteFailsDelete() {
@Test
public void testOverwriteWithAppendOutsideOfDelete() {
TableMetadata base = TestTables.readMetadata(TABLE_NAME);
- long baseId =
- latestSnapshot(base, branch) == null ? -1 : latestSnapshot(base, branch).snapshotId();
+ Snapshot latestSnapshot = latestSnapshot(base, branch);
Review Comment:
maybe we can just add the `SnapshotUtil` change in this PR? I don't think we can resolve the comments in that PR in time based on the current situation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] yyanyy commented on a diff in pull request #6650: Core, API: BaseReplacePartitions to branch test Refactoring
Posted by "yyanyy (via GitHub)" <gi...@apache.org>.
yyanyy commented on code in PR #6650:
URL: https://github.com/apache/iceberg/pull/6650#discussion_r1121011113
##########
core/src/main/java/org/apache/iceberg/util/SnapshotUtil.java:
##########
@@ -394,4 +396,42 @@ public static Schema schemaFor(Table table, Long snapshotId, Long timestampMilli
return table.schema();
}
+
+ /**
+ * Fetch the snapshot at the head of the given branch in the given table.
+ *
+ * <p>This method calls {@link Table#currentSnapshot()} instead of using branch API {@link
+ * Table#snapshot(String)} for the main branch to ensure existing tests still goes through the old
Review Comment:
nit/nonblocker: this class seems to be public, not sure if we want to mention the internal testing strategy here. Same goes for `latestSnapshot`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] namrathamyske commented on pull request #6650: Core, API: BaseReplacePartitions to branch test Refactoring
Posted by "namrathamyske (via GitHub)" <gi...@apache.org>.
namrathamyske commented on PR #6650:
URL: https://github.com/apache/iceberg/pull/6650#issuecomment-1440875668
@jackye1995 can you also include this PR for the release ? This is just refactoring
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org