dramaticlly commented on code in PR #13913:
URL: https://github.com/apache/iceberg/pull/13913#discussion_r2369648366
##########
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:
> but we do actually have cases where we want to not pass an argument to the
underlying core API if the user didn't provide any input and thus rely on
whatever the default behavior of the core API is
From my understanding, I believe @nastra wanted us to keep original boxed
boolean to convey user intention, as unset default of null implies not apply
such flag to underlying spark action. Now we switch to false as primitive
boolean, it will always pass such flag to action.
Sorry I missed this initially, let's switch to `Boolean` just like before
this change and keep the minimal necessary change to adopt `ProcedureInput`.
Maybe also worth a comment to convey the message as well
--
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]
