dramaticlly commented on code in PR #13913:
URL: https://github.com/apache/iceberg/pull/13913#discussion_r2363850099
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/ExpireSnapshotsProcedure.java:
##########
@@ -104,13 +118,14 @@ public ProcedureParameter[] parameters() {
@Override
@SuppressWarnings("checkstyle:CyclomaticComplexity")
public Iterator<Scan> call(InternalRow args) {
- Identifier tableIdent = toIdentifier(args.getString(0),
PARAMETERS[0].name());
- 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) ? null : args.getBoolean(4);
- long[] snapshotIds = args.isNullAt(5) ? null :
args.getArray(5).toLongArray();
- Boolean cleanExpiredMetadata = args.isNullAt(6) ? null :
args.getBoolean(6);
+ ProcedureInput input = new ProcedureInput(spark(), tableCatalog(),
PARAMETERS, args);
+ Identifier tableIdent = input.ident(TABLE_PARAM);
+ Long olderThanMillis = input.asTimestampLong(OLDER_THAN_PARAM, null);
+ Integer retainLastNum = input.asInt(RETAIN_LAST_PARAM, null);
+ Integer maxConcurrentDeletes = input.asInt(MAX_CONCURRENT_DELETES_PARAM,
null);
+ boolean streamResult = input.asBoolean(STREAM_RESULTS_PARAM, false);
+ Long[] snapshotIds = input.asLongArray(SNAPSHOT_IDS_PARAM, null);
+ boolean cleanExpiredMetadata =
input.asBoolean(CLEAN_EXPIRED_METADATA_PARAM, false);
Review Comment:
Good catch on the boolean handling. I actually suggested @slfan1989 convert
these to primitive booleans because all the procedures here use boolean
parameters as explicit enablement flags, where null defaults to disabled.
This gives us a chance to clean up the `if $Boolean != null` pattern below
when setting the corresponding flags.
Since we typically backport changes from the latest Spark version to older
ones, making this change now should save us some backport work later, even
though it means a bit more review effort upfront. Our unit tests should catch
any behavioral changes from the semantic shift.
--
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]
