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 19:47:09 UTC
[GitHub] [iceberg] kbendick opened a new pull request #3268: [SPARK] Simplify shouldProcess check in Spark3 streaming source
kbendick opened a new pull request #3268:
URL: https://github.com/apache/iceberg/pull/3268
Removes a redundant call to Preconditions.checkState in the Spark 3 MicroBatchStream `shouldProcess` method.
Also stores and reuses the result from checking if its an append operation, and places that first in the list of checks, as this is by far the most common case for anybody using the Spark 3 streaming source at present.
This method was a bit confusing for me on first read and given that the APPEND check is the most common case, processing it first and storing the results means that the check will likely involve far fewer string comparisons.
I was going to allow for also skipping OVERWRITE if users set a flag, but there is already open discussion around this and some older PRs. So closing my original PR in favor of just simplify the check and continuing the discussion elsewhere: 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 commented on pull request #3268: [SPARK] Simplify shouldProcess check in Spark3 streaming source
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #3268:
URL: https://github.com/apache/iceberg/pull/3268#issuecomment-939423726
Now that I've run the tests and had more coffee, I can see that without using the switch statement I was using previously, the two separated Preconditions are in fact required (or a relatively complicated boolean check).
--
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] rdblue commented on pull request #3268: [SPARK] Simplify shouldProcess check in Spark3 streaming source
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3268:
URL: https://github.com/apache/iceberg/pull/3268#issuecomment-940151632
Merged. Thanks, @kbendick!
--
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 #3268: [SPARK] Simplify shouldProcess check in Spark3 streaming source
Posted by GitBox <gi...@apache.org>.
kbendick removed a comment on pull request #3268:
URL: https://github.com/apache/iceberg/pull/3268#issuecomment-939423726
Now that I've run the tests and had more coffee, I can see that without using the switch statement I was using previously, the two separated Preconditions are in fact required (or a relatively complicated boolean check).
--
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] rdblue merged pull request #3268: [SPARK] Simplify shouldProcess check in Spark3 streaming source
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #3268:
URL: https://github.com/apache/iceberg/pull/3268
--
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 #3268: [SPARK] Simplify shouldProcess check in Spark3 streaming source
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #3268:
URL: https://github.com/apache/iceberg/pull/3268#issuecomment-939352969
There are a few corner cases that don't appear to be handled. For example if somebody runs a `CreateOrReplaceTable` operation on an existing table and somebody streams from the beginning it will either NPE or likely return to the beginning of the table entirely.
But given that the Spark streaming source is presently only able to handle a limited set of use cases, I don't think convoluting it for edge cases is the best idea. If anybody would like to discuss my findings, I'd be happy to do so as I spent some time looking into CDC in other systems and how it can be efficiently applied for the Spark stream (which doesn't have an in-built notion of CDC or deletes in general). There's a bit more of a break down on the other PR mentioned above.
--
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