pvary commented on code in PR #15780:
URL: https://github.com/apache/iceberg/pull/15780#discussion_r3116611308
##########
flink/v2.1/flink/src/main/java/org/apache/iceberg/flink/sink/dynamic/DynamicRecord.java:
##########
@@ -38,6 +40,7 @@ public class DynamicRecord {
private int writeParallelism;
private boolean upsertMode;
@Nullable private Set<String> equalityFields;
+ @Nullable private FlinkWriteConf flinkWriteConf;
Review Comment:
> Subset of fields are in constructor (No empty constructor) and for few
fields , we need to use set methods. May be move to empty constructor and
deprecate the old constructor ?
The goal is to try to enforce the users to reuse the records, and don't
create a new one whenever it is possible. Don't forget that this is on the hot
path
> writeParallelism and upsertMode are using Primitives, so not easy to
determine when not being explicitly set.
We haven't considered them to be unset. This was intentional at that time,
but might have been a mistake based on the current knowledge.
> I don't think we should break backwards-compatibility, unless we have to
Agreed, but technically we are allowed to for experimental features.
I really hate mixing the usage of the config and the actual data class. So I
would vote either for the interface or the wrapper class.
--
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]
