Skip to content

Conversation

@SreeramGarlapati
Copy link
Collaborator

@SreeramGarlapati SreeramGarlapati commented Nov 10, 2021

Delete operations on iceberg table can translate to Snapshots of type DataOperations.DELETE or DataOperations.OVERWRITE.

DataOperations.DELETE - when the delete operation on the table translates to full file deletes (for ex: deletes wiping out partitions using partitionKey level predicates)

DataOperations.OVERWRITE - is the most common case of Delete.

@rdblue / @aokolnychyi / @RussellSpitzer / @kbendick - pl. lemme know what you folks think & once we achieve principle level alignment - I will add unittests.

@github-actions github-actions bot added the spark label Nov 10, 2021
@SreeramGarlapati
Copy link
Collaborator Author

SreeramGarlapati commented Nov 10, 2021

@SreeramGarlapati
Copy link
Collaborator Author

I stumbled on the exact same change made by @kbendick : #3267
@kbendick - truly apologize - I did not get a chance to look at this (I had to turn off my git notifications at some point in the past). Truly appreciate - if you can reopen your branch and get that going. Or - I can merge your changes into this PR. Either way works.

@tprelle
Copy link
Contributor

tprelle commented Nov 12, 2021

Hi @SreeramGarlapati @kbendick,
I have some concern to link Overwrite and Delete because in table v1 so Copy on Write mode. A merge with an update will create a overwrite snapshot. So if you skip overwrite snapshot you will miss all theses changes.
For example delta.io allow user to choose to resend everything if it's copy on write mode.

"Cannot process delete snapshot : %s. Set read option %s to allow skipping snapshots of type delete",
snapshot.snapshotId(), SparkReadOptions.STREAMING_SKIP_DELETE_SNAPSHOTS);
return false;
case DataOperations.OVERWRITE:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that this should use the same configuration to skip deletes and overwrites. Overwrites are different and I think that we should at a minimum have a different property. I would also prefer to have some additional clarity on how we plan to eventually handle this. We could skip overwrites, but what about use cases where they are probably upserts? What about when they're created by copy-on-write MERGE operations?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1. I would prefer they be two separate configs, but also that we have a plan for the longer term to handle sending out these row deltas.

I'd be ok with getting a PR in to ignore OVERWRITE, but this isn't something we should ignore in the longer term (or even really the near-to-medium term) as others have mentioned.

Personally I would consider using a schema similar to the delta.io change capture feed that has a dataframe with the before image / after image (row before and after update) and then the type of operation for each row (insert, delete, update_before, update_after).

Copy link
Contributor

Choose a reason for hiding this comment

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

I connected with @SreeramGarlapati to contribute on this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have added a separate config to skip overwrites. I will discuss with @SreeramGarlapati and will update on the plan to eventually handle upserts.

Copy link
Collaborator Author

@SreeramGarlapati SreeramGarlapati Jan 5, 2022

Choose a reason for hiding this comment

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

all in all, there are 2 options for reading upserts:

  1. for updates which are written with - copy on write -- a new data file is created which has a combination of both old rows and these new updated rows. So, in this case - we can take a spark option from the user to take consent - that they are okay with data replay.
  2. for updates which are written with - merge on read - we will expose an option to read change data feed - where we will include a metadata column - which indicates whether a record is an INSERT vs DELETE.
    did this make sense - @rdblue & @kbendick

"Cannot process overwrite snapshot : %s. Set read option %s to allow skipping snapshots of type overwrite",
snapshot.snapshotId(), SparkReadOptions.STREAMING_SKIP_DELETE_SNAPSHOTS);
return false;
default:
Copy link
Member

Choose a reason for hiding this comment

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

Should add a test for this new conf option

Copy link
Contributor

Choose a reason for hiding this comment

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

I have added the Unit Test.

@rajarshisarkar
Copy link
Contributor

Hi @rdblue @kbendick @RussellSpitzer Request you to please review.

@rdblue rdblue merged commit a7bbe4d into apache:master Jan 28, 2022
@rdblue
Copy link
Contributor

rdblue commented Jan 28, 2022

Merged. Thanks @SreeramGarlapati and @rajarshisarkar!

sunchao pushed a commit to sunchao/iceberg that referenced this pull request May 9, 2023
Co-authored-by: Rajarshi Sarkar <srajars@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants