Core, API: BaseRowDelta, BaseOverwrite,BaseReplacePartitions, BaseRewrite to branch Impl #5234

namrathamyske · 2022-07-10T18:30:37Z

issue addressed from: #3896

This PR implements the performing RowDelta operations on a particular branch ref. Throws unsupported operations with other operations.

PR continued from:
#4926

Append changes to snapshot branch - changes after review comments remove space Adding support methods for supporting performing append/delete on branch Removing spaces Removing spaces Removing unwanted vars Adding check for null and invalid branch Adding check for null and invalid branch

…h-check-validations

namrathamyske · 2022-07-10T19:05:06Z

@amogh-jahagirdar @rdblue As per #4926 (comment) I have changed validation checks when we commit to a branch. Right now I have implemented for BaseRowDelta usecase. My future plan is to extend for others.

My thought process is every-time we check for validations, the start snapshot should start from an ancestor of the branch. If not we can throw a validation exception. Start snapshot should ideally never go before the snapshot with which branch was created. But we don't store the commit for which the branch was created in metadata. So i am quite confused how to go ahead. Any thoughts appreciated!

…h-checks-final

core/src/main/java/org/apache/iceberg/BaseRowDelta.java

amogh-jahagirdar

Awesome, I forgot you had this draft PR already so I think we can just continue on here. I closed out my PR #5595

core/src/test/java/org/apache/iceberg/TestRowDelta.java

core/src/main/java/org/apache/iceberg/BaseRowDelta.java

core/src/test/java/org/apache/iceberg/TestRowDelta.java

core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java

core/src/main/java/org/apache/iceberg/BaseOverwriteFiles.java

core/src/test/java/org/apache/iceberg/TestRowDelta.java

…h-checks-final

namrathamyske · 2022-08-28T20:37:45Z

In PR #5618 MergeAppend and TableTestBase commit and apply methods are implemented. Will make use of those methods in RowDelta after its merged.

…-checks-final

namrathamyske · 2023-01-18T02:46:08Z

@rdblue Appreciate your feedback on this!

amogh-jahagirdar · 2023-01-18T18:57:34Z

core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java

+      Snapshot parent) {
    // if there is no current table state, no files have been added
-    if (base.currentSnapshot() == null) {
+    if (parent == null) {


My bad, missed this when I raised the change to your branch. I think we should update this comment to say "If there is no parent, no files have been added"

rdblue · 2023-01-18T23:43:21Z

.palantir/revapi.yml

        \ T) throws java.io.IOException, com.fasterxml.jackson.core.JacksonException\
        \ @ org.apache.iceberg.rest.RESTSerializers.UpdateRequirementDeserializer"
      justification: "False positive - JacksonException is a subclass of IOException"
+    - code: "java.method.inheritedMovedToClass"


When the review is closer to approval, we should see how many of these breaking changes can be solved. For example, this one about moving the method we may be able to take care of by overriding rather than replacing the method.

rdblue · 2023-01-18T23:43:48Z

core/src/test/java/org/apache/iceberg/TableTestBase.java

+    for (DataFile file : expectedFiles) {
+      expectedFilePaths.add(file.path());
+    }
+    Set<CharSequence> actualFilePaths = Sets.newHashSet();


Nit: block spacing.

rdblue · 2023-01-20T22:23:07Z

core/src/main/java/org/apache/iceberg/BaseReplacePartitions.java

+            currentMetadata, startingSnapshotId, Expressions.alwaysTrue(), snapshot);
      } else {
-        validateAddedDataFiles(currentMetadata, startingSnapshotId, replacedPartitions);
+        validateAddedDataFiles(currentMetadata, startingSnapshotId, replacedPartitions, snapshot);


Minor: I think it makes more sense to use parent rather than snapshot for the parent snapshot variable. Not a blocker, but it makes it easier to understand what's going on.

rdblue · 2023-01-20T22:23:36Z

core/src/main/java/org/apache/iceberg/BaseRowDelta.java

+            "Snapshot %s is not an ancestor of %s",
+            startingSnapshotId,
+            parent.snapshotId());
+      }


Nit: whitespace after this block.

rdblue · 2023-01-20T22:25:42Z

core/src/main/java/org/apache/iceberg/BaseRowDelta.java

+      if (startingSnapshotId != null) {
+        Preconditions.checkArgument(
+            SnapshotUtil.isAncestorOf(parent.snapshotId(), startingSnapshotId, base::snapshot),
+            "Snapshot %s is not an ancestor of %s",


I think a bit more context would be helpful if you ever encounter this error. "Cannot validate changes: starting snapshot %s is not an ancestor of parent snapshot %s (branch %s)"

@amogh-jahagirdar Is this ancestor check even required anymore ? We are anyway doing ancestor check in

iceberg/core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java

Line 742 in a1d994c

ValidationException.check(

rdblue · 2023-01-20T22:30:43Z

core/src/test/java/org/apache/iceberg/TestOverwrite.java

    TableMetadata base = TestTables.readMetadata(TABLE_NAME);
-    long baseId = base.currentSnapshot().snapshotId();
+    long baseId =
+        latestSnapshot(base, branch) == null ? -1 : latestSnapshot(base, branch).snapshotId();


Minor: prefer to keep ternary expressions as simple as possible. If you're calling a method twice with the same arguments, you generally want to use a variable:

Snapshot latest = latestSnapshot(base, branch); long baseId = latest == null ? -1 : latest.snapshotId();

It's also a bit odd that the current snapshot is always defined, but here we handle the case where the branch does not yet exist. Why is that? Are we committing to main and then implicitly branching somewhere?

rdblue · 2023-01-20T22:31:15Z

core/src/test/java/org/apache/iceberg/TestOverwrite.java

+            .newOverwrite()
+            .overwriteByRowFilter(equal("date", "2018-06-08"))
+            .addFile(FILE_10_TO_14),
+        branch); // in 2018-06-09, NOT in 2018-06-08


Minor: Can the comment remain on the FILE_10_TO_14 line?

rdblue · 2023-01-20T22:43:07Z

core/src/test/java/org/apache/iceberg/TestReplacePartitions.java

    table.updateProperties().set(TableProperties.MANIFEST_MIN_MERGE_COUNT, "1").commit();

-    table.newFastAppend().appendFile(FILE_A).appendFile(FILE_B).commit();
+    table.newFastAppend().appendFile(FILE_A).appendFile(FILE_B).toBranch(branch).commit();


Why not use the commit method in these tests?

rdblue · 2023-01-20T22:49:06Z

core/src/test/java/org/apache/iceberg/TestRowDelta.java

+        table.newRowDelta().addRows(FILE_A).addDeletes(FILE_A_DELETES).addDeletes(FILE_B_DELETES);

-    Snapshot snap = table.currentSnapshot();
+    commit(table, rowDelta, branch);


Odd spacing here. I'd expect the commit to come after rowDelta insteadd of being combined with the latestSnapshot call.

core/src/test/java/org/apache/iceberg/TestRowDelta.java

core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java

rdblue

@namrathamyske and @amogh-jahagirdar, this is looking great! There are no major blockers, just minor comments. I think that means we should move forward with fixing the revapi failures (add back methods and deprecate them). After that, we can get this in.

Thanks!

…, update revapi

amogh-jahagirdar · 2023-01-21T03:43:12Z

Thanks for the reviews @rdblue ! @namrathamyske I raised a PR to your branch for adding back the old validation methods/marking them as deprecated and updating rev API namrathamyske#21. When you get a chance please take a look!

Core: Deprecate validation methods which don't accept parent snapshot…

rdblue · 2023-01-23T15:48:48Z

Thanks, @namrathamyske and @amogh-jahagirdar! Great to have this in.

amogh-jahagirdar · 2023-01-23T16:07:44Z

Thanks for the reviews @rdblue! and thanks @namrathamyske for the contribution!

nkeshavaprakash added 13 commits June 9, 2022 13:22

Adding snapshot to branch Impl

e6ea5f6

Adding check to see if it's a branch

b43b80b

Creating branch if ref absent

9cb39c2

Building after setting ref

ba341ec

Setting default branch as main

d1ca432

Fetching current when no main ref is set.

0d79ef0

checkStyle fixes

495c0f3

build fixes

f7464eb

Validation check for branch

39de1e4

Merge branch 'master' of https://github.com/apache/iceberg into branc…

7e75c68

…h-check-validations

Restricting toBranch only for BaseRowDelta

080d76e

removing new line

1dc4f89

github-actions bot added API core labels Jul 10, 2022

namrathamk added 3 commits August 23, 2022 00:45

Updating BaseRowDelta as per SnapshotProducer

41b2f62

Updating BaseRowDelta as per SnapshotProducer

0c18302

Merge branch 'master' of https://github.com/apache/iceberg into branc…

0acca3b

…h-checks-final

namrathamyske marked this pull request as ready for review August 23, 2022 06:57

Updating BaseRowDelta as per SnapshotProducer

49b6667

github-actions bot removed the API label Aug 23, 2022

amogh-jahagirdar reviewed Aug 23, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseRowDelta.java Outdated Show resolved Hide resolved

amogh-jahagirdar reviewed Aug 23, 2022

View reviewed changes

rdblue reviewed Aug 24, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseOverwriteFiles.java Show resolved Hide resolved

rdblue reviewed Aug 24, 2022

View reviewed changes

core/src/test/java/org/apache/iceberg/TestRowDelta.java Outdated Show resolved Hide resolved

namrathamk added 3 commits August 24, 2022 02:54

Merge branch 'master' of https://github.com/apache/iceberg into branc…

4ab3414

…h-checks-final

Merge branch 'master' of https://github.com/apache/iceberg into branc…

fb28c02

…h-checks-final

Updating BaseRowDelta as per SnapshotProducer

cfc5e67

Merge remote-tracking branch 'origin/branch-checks-final' into branch…

defff1d

…-checks-final

namrathamyske added 2 commits January 17, 2023 20:07

overwrite validation tests

66e1850

variable name change

fc8780c

amogh-jahagirdar reviewed Jan 18, 2023

View reviewed changes

rdblue reviewed Jan 18, 2023

View reviewed changes

rdblue reviewed Jan 20, 2023

View reviewed changes

core/src/test/java/org/apache/iceberg/TestRowDelta.java Outdated Show resolved Hide resolved

Uncomment test.

6b6aefc

rdblue reviewed Jan 21, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java Show resolved Hide resolved

rdblue reviewed Jan 21, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java Show resolved Hide resolved

rdblue reviewed Jan 21, 2023

View reviewed changes

Core: Deprecate validation methods which don't accept parent snapshot…

1a98e54

…, update revapi

Merge pull request #21 from amogh-jahagirdar/row-delta-rev-api

74bab58

Core: Deprecate validation methods which don't accept parent snapshot…

jackye1995 mentioned this pull request Jan 21, 2023

Core: Fix for deleting files when commiting transactions with multiple branches #6634

Merged

rdblue approved these changes Jan 23, 2023

View reviewed changes

rdblue merged commit eec20ed into apache:master Jan 23, 2023

namrathamyske mentioned this pull request Jan 23, 2023

Core, API: BaseReplacePartitions to branch test Refactoring #6650

Merged

nastra mentioned this pull request Feb 7, 2023

API, Core: Multi-Table transactions #6757

Closed

krvikash pushed a commit to krvikash/iceberg that referenced this pull request Mar 16, 2023

Core, API: Support branch writes in more operations (apache#5234)

319ba66

Core, API: BaseRowDelta, BaseOverwrite,BaseReplacePartitions, BaseRewrite to branch Impl #5234

Core, API: BaseRowDelta, BaseOverwrite,BaseReplacePartitions, BaseRewrite to branch Impl #5234

Uh oh!

Conversation

namrathamyske commented Jul 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

namrathamyske commented Jul 10, 2022

Uh oh!

Uh oh!

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

namrathamyske commented Aug 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

namrathamyske commented Jan 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

namrathamyske Jan 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rdblue left a comment

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar commented Jan 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rdblue commented Jan 23, 2023

Uh oh!

amogh-jahagirdar commented Jan 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

namrathamyske commented Jul 10, 2022 •

edited

Loading

namrathamyske commented Aug 28, 2022 •

edited

Loading

namrathamyske Jan 22, 2023 •

edited

Loading

amogh-jahagirdar commented Jan 21, 2023 •

edited

Loading