mxm commented on code in PR #15780:
URL: https://github.com/apache/iceberg/pull/15780#discussion_r3116115444
##########
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:
I don't think we should break backwards-compatibility, unless we have to.
The revised solution looks fine to me. The config method is package-level
visible, so cannot be overridden by the user via subclasssing.
I'm also OK with a wrapper if we make sure to reuse it and not re-create it
for every record.
##########
flink/v2.1/flink/src/main/java/org/apache/iceberg/flink/sink/dynamic/DynamicRecord.java:
##########
@@ -79,7 +82,8 @@ public void setTableIdentifier(TableIdentifier
tableIdentifier) {
}
public String branch() {
- return branch;
+ return MoreObjects.firstNonNull(
+ branch, flinkWriteConf == null ? null : flinkWriteConf.branch());
Review Comment:
Slight preference for the second one, but it has different semantics from
the diff above. `MoreObjects.firstNonNull(..)` will throw if all arguments are
null.
##########
flink/v2.1/flink/src/test/java/org/apache/iceberg/flink/sink/dynamic/TestDynamicIcebergSink.java:
##########
@@ -1422,9 +1498,12 @@ private void verifyResults(List<DynamicIcebergDataImpl>
dynamicData) throws IOEx
dynamicData.forEach(
r -> {
+ String branch =
+ MoreObjects.firstNonNull(
+ r.branch,
writeProperties.get(FlinkWriteOptions.BRANCH.key()));
Review Comment:
`firstNonNull` only ever allows too arguments. I think it's fine, but I have
no issues with the ternary if either.
--
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]
