aokolnychyi commented on a change in pull request #4152:
URL: https://github.com/apache/iceberg/pull/4152#discussion_r812492289
##########
File path:
spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/procedures/ExpireSnapshotsProcedure.java
##########
@@ -83,6 +85,7 @@ public StructType outputType() {
Long olderThanMillis = args.isNullAt(1) ? null :
DateTimeUtil.microsToMillis(args.getLong(1));
Integer retainLastNum = args.isNullAt(2) ? null : args.getInt(2);
Integer maxConcurrentDeletes = args.isNullAt(3) ? null : args.getInt(3);
+ boolean streamResult = args.isNullAt(4) ? false : args.getBoolean(4);
Review comment:
I know I suggested to convert it to `boolean` when I saw it being
defaulted to `false`. However, I now remember why we use boxed primitives for
other arguments: we want to inherit default values from the action. In this
case, the procedure defines a default value. Instead, it should act as a
wrapper that exposes certain configs from the underlying action. If those
configs are not set, the action itself should pick a default value.
It should probably become `Boolean` with null as the default value.
```
Boolean streamResult = args.isNullAt(4) ? null : args.getBoolean(4);
...
if (streamResult != null) {
action.option(ExpireSnapshots.STREAM_RESULTS,
Boolean.toString(streamResult));
}
```
That way, the handling will be consistent with other properties and the
procedure will inherit any changes in default action values that may happen in
the future.
--
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]
