echonesis commented on code in PR #9960:
URL: https://github.com/apache/gravitino/pull/9960#discussion_r2910613390
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -787,6 +786,71 @@ static String replacePlaceholder(String inputString,
Map<String, String> replace
return result.toString();
}
+ /**
+ * Build arguments list with optional argument support.
+ *
+ * @param templateArgs Template argument list (may contain optional markers)
+ * @param jobConf Job configuration for placeholder replacement
+ * @return Final argument list with optional arguments filtered
+ */
+ @VisibleForTesting
+ static List<String> buildArgumentsWithOptional(
+ List<String> templateArgs, Map<String, String> jobConf) {
+ List<String> result = new ArrayList<>();
+
+ for (int i = 0; i < templateArgs.size(); i++) {
+ String arg = templateArgs.get(i);
+ boolean isOptional = arg.startsWith("?");
+ String cleanArg = isOptional ? arg.substring(1) : arg;
+
+ // Replace placeholder
+ String replacedArg = replacePlaceholder(cleanArg, jobConf);
+
+ // If optional and value is empty, skip this argument
+ if (isOptional && isEmptyValue(replacedArg)) {
+ continue;
+ }
+
+ // If this is an optional flag (not a placeholder itself) and the next
argument is also
+ // optional but empty, skip both (they form a pair like ?--flag
?{{value}}).
+ // Independent optional placeholders like ?{{opt1}} ?{{opt2}} are
handled individually.
+ if (isOptional
+ && !PLACEHOLDER_PATTERN.matcher(cleanArg).find()
Review Comment:
Thanks @jerryshao for the review.
I have updated the code to address these gaps:
1. Pair-skip logic: Updated **buildArgumentsWithOptional** to skip the
entire pair even if the value argument lacks the `?` marker.
2. Validation check: Added a validation check at template registration time
to warn if a `?`-prefixed literal is followed by a non-optional argument.
3. Standalone `?--flag`: Documented that standalone optional flags are
intentionally always included, and recommended pairing them with a sentinel
placeholder if they need to be conditionally suppressible.
--
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]
