You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/10/09 09:05:45 UTC

[GitHub] [iceberg] kbendick opened a new pull request #3267: [Spark] Streaming Source - Skip OVERWRITE snapshot operations when streaming-skip-delete-snapshots is true

kbendick opened a new pull request #3267:
URL: https://github.com/apache/iceberg/pull/3267


   This PR adds
   
   1. Explicit handling for type DataOperations.OVERWRITE operations when set the Spark read option "streaming-skip-delete-snapshots" to true.
   2. More explicit handling of all possible DataOperations, as well as a catch all clause for unrecognized / unimplemented snapshot operation types.
   3. A unit test to show the behavior when `streaming-skip-delete-snapshots` is set to true (to show the behavior and pair with the unit test for the default case, which sets the flag to false).
   
   This closes https://github.com/apache/iceberg/issues/3265
   
   **Detailed explanation & Motivating Use Case**
   
   The Spark MicroBatch streaming source can currently only handle snapshots that do not mutate or delete any of the existing rows and still produce a "correct" stream.
   
   This means that it can presently handle two types of snapshots that generate data:
   - DataOperations.APPEND - New data files are added to the table.
   - DataOperations.REPLACE - Files are replaced, without changing the data in the table (e.g. data file rewrites)
   
   Users can choose to skip "delete" type snapshots via the read option "streaming-skip-delete-snapshots", which simply skips the given snapshot if it potentially contains any kind of mutation to a row in the table.
   
   OVERWRITE type snapshots are a form of delete and so they should arguably be skippable if users choose to skip deletes.
   
   Consider the case where users are simply interested in using Spark to get an Iceberg source stream of an append only table (which is more or less what they can do now), but data is then reprocessed as part of a batch operation to change its compression type for example. There would be no easy way to skip these out-of-band one time OVERWRITE operations committed to the table.
   
   However, they also can add data (as opposed to snapshots of purely DELETE type). While it would potentially be possible to grab only the added data files and still skip the deletes in a limited set of cases, given that users are skipping deletes, it seems fair to allow them to skip mixed deletes as well. I'd personally rather the time spent implementing that go into refactoring the Spark MicroBatch stream to truly produce CDC data (something I have begun working on a high level proposal for an API, given that it's not supported in Spark natively). But I'm very much open to discussion on this
   
   When we refactor the spark streaming source to handle deletions, we will of course be sure to handle commits that both delete and add data at the same time.  Hence why I think this more explicit addition is good enough for now.
   
   If we don't want to take this relatively simplistic (but in line with the status quo) approach, at the very least, a test should be added indicating the intended behavior when "streaming-skip-delete-snapshots" is true, as there's a test showing that OVERWRITE snapshots will fail an Iceberg spark streaming source when the option is not used / set to false (its default value).


-- 
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] kbendick commented on pull request #3267: [Spark] Streaming Source - Skip OVERWRITE snapshot operations when streaming-skip-delete-snapshots is true

Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #3267:
URL: https://github.com/apache/iceberg/pull/3267#issuecomment-939353699


   I see now that there are several open issues and a few open PRs related to this. So I'm probably going to close this PR in favor of only [simplifying the shouldProcess preconditions check](https://github.com/apache/iceberg/pull/3268).
   
   I did some research into CDC recently in other systems but I'm starting a new position soon so I'm not sure what the priority will be yet for where I spend my time. I will know more next week, but am happy to discuss with others if I don't have cycles to work on the Spark streaming source.


-- 
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] kbendick closed pull request #3267: [Spark] Streaming Source - Skip OVERWRITE snapshot operations when streaming-skip-delete-snapshots is true

Posted by GitBox <gi...@apache.org>.
kbendick closed pull request #3267:
URL: https://github.com/apache/iceberg/pull/3267


   


-- 
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] kbendick removed a comment on pull request #3267: [Spark] Streaming Source - Skip OVERWRITE snapshot operations when streaming-skip-delete-snapshots is true

Posted by GitBox <gi...@apache.org>.
kbendick removed a comment on pull request #3267:
URL: https://github.com/apache/iceberg/pull/3267#issuecomment-939347961


   I see now that there are several open issues and a few open PRs related to this. So I'm probably going to close this, open a new PR to simplify the preconditions check, and then go back and discuss this with others if anything.


-- 
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] kbendick commented on a change in pull request #3267: [Spark] Streaming Source - Skip OVERWRITE snapshot operations when streaming-skip-delete-snapshots is true

Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3267:
URL: https://github.com/apache/iceberg/pull/3267#discussion_r725461003



##########
File path: spark3/src/main/java/org/apache/iceberg/spark/source/SparkMicroBatchStream.java
##########
@@ -212,12 +212,22 @@ public void stop() {
 
   private boolean shouldProcess(Snapshot snapshot) {
     String op = snapshot.operation();
-    Preconditions.checkState(!op.equals(DataOperations.DELETE) || skipDelete,
-        "Cannot process delete snapshot: %s", snapshot.snapshotId());
-    Preconditions.checkState(
-        op.equals(DataOperations.DELETE) || op.equals(DataOperations.APPEND) || op.equals(DataOperations.REPLACE),
-        "Cannot process %s snapshot: %s", op.toLowerCase(Locale.ROOT), snapshot.snapshotId());

Review comment:
       Currently, the code handles `skipDeletes` via the first Preconditions.checkState assertion. Then, if that passed, it checks again that the operation is one of the insert only operations or delete again (as presumably if the snapshot operation were of type `DataOperations.DELETE`, the flag `skipDelete` would have been set to true per the first assertion).
   
   This was a little confusing for me at first seeing it checked twice, once in the negative and once in the positive.
   
   The second assertion here also arguably serves as a way of detecting potentially unhandled `DataOperation` types. Given that, I chose to use a switch statement to be a bit more explicit about what is and is not allowed, as well as having the benefit of a default case to catch any unhandled `DataOperations` (should they be added later for example).
   
   If we want to go back to using `Preconditions`, I'm more than happy to do that. As this code is arguably in the hot path for the driver during scheduling, we could collapse the two calls into one and change the ordering so that string comparisons are reduced (by checking for `skipDelete` first and then checking for DataOperations.APPEND, which should be the most common case).
   
   Let me know if anybody has issue with the switch statement.




-- 
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] kbendick commented on pull request #3267: [Spark] Streaming Source - Skip OVERWRITE snapshot operations when streaming-skip-delete-snapshots is true

Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #3267:
URL: https://github.com/apache/iceberg/pull/3267#issuecomment-939347961


   I see now that there are several open issues and a few open PRs related to this. So I'm probably going to close this, open a new PR to simplify the preconditions check, and then go back and discuss this with others if anything.


-- 
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] kbendick commented on a change in pull request #3267: [Spark] Streaming Source - Skip OVERWRITE snapshot operations when streaming-skip-delete-snapshots is true

Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3267:
URL: https://github.com/apache/iceberg/pull/3267#discussion_r725461003



##########
File path: spark3/src/main/java/org/apache/iceberg/spark/source/SparkMicroBatchStream.java
##########
@@ -212,12 +212,22 @@ public void stop() {
 
   private boolean shouldProcess(Snapshot snapshot) {
     String op = snapshot.operation();
-    Preconditions.checkState(!op.equals(DataOperations.DELETE) || skipDelete,
-        "Cannot process delete snapshot: %s", snapshot.snapshotId());
-    Preconditions.checkState(
-        op.equals(DataOperations.DELETE) || op.equals(DataOperations.APPEND) || op.equals(DataOperations.REPLACE),
-        "Cannot process %s snapshot: %s", op.toLowerCase(Locale.ROOT), snapshot.snapshotId());

Review comment:
       Currently, the code handles `skipDeletes` via the first Preconditions.checkState assertion. Then, if that passed, it checks again that the operation is one of the insert only operations OR delete again (as presumably if the snapshot operation were of type `DataOperations.DELETE`, the flag `skipDelete` would have been set to true per the first assertion).
   
   This was a little confusing for me at first seeing it checked twice, once in the negative and once in the positive.
   
   The second assertion here also arguably serves as a way of detecting potentially unhandled `DataOperation` types. Given that, I chose to use a switch statement to be a bit more explicit about what is and is not allowed, as well as having the benefit of a default case to catch any unhandled `DataOperations` (should they be added later for example).
   
   If we want to go back to using `Preconditions`, I'm more than happy to do that. As this code is arguably in the hot path for the driver during scheduling, we could collapse the two calls into one (as they have the same precondition statement anyway) and change the ordering so that string comparisons are reduced (by checking for `skipDelete` first and then checking for DataOperations.APPEND, which should be the most common case).
   
   Let me know if anybody has issue with the use of the switch statement.




-- 
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