CodingCat commented on PR #7649:
URL: https://github.com/apache/iceberg/pull/7649#issuecomment-1562207304
> Thanks @CodingCat for the PR. Looks good to me overall. Left minor
comments. I don't have much context with `CommitMetadata`. My understanding is
that every time we generate a new snapshot. We will need to inject it if it is
there. Is it possible to have the functionality of setting it in a base class
of all snapshot producer?
thanks for the review @flyrain !
IIUC, you mean we move the code consuming CommitMetadata to a base class so
that we will not fall into the situation like this PR tries to address?
I thought about it when started working on this PR , it may involve a bit
refactoring for iceberg-spark ...
in the current implementations, `commitOperation` in
[SparkWrite](https://github.com/apache/iceberg/blob/82880be39654bb94aaf370338bdd61f706caa6da/spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/source/SparkWrite.java#L192)
is different with that in
[SparkPositionDeltaWrite](https://github.com/apache/iceberg/blob/82880be39654bb94aaf370338bdd61f706caa6da/spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/source/SparkPositionDeltaWrite.java#L259)
are pretty much the same , we can potentially move them to a base trait, my
only concern is
(1) do we want to do in this PR?
(2) in future, can we guarantee to use the same commit implementation for
all snapshot producers (at least in Spark)?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
