jerryshao commented on code in PR #9960:
URL: https://github.com/apache/gravitino/pull/9960#discussion_r2893683077
##########
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:
**Design gap: `?--flag` is silently always included when the next arg is not
also optional.**
The pair-skip only fires when `templateArgs.get(i + 1).startsWith("?")`. A
template such as:
```
["?--strategy", "{{strategy}}"] // note: no ? on the value
```
will always emit `--strategy` even when `strategy` is absent, because the
pair check never fires (the non-optional `{{strategy}}` doesn't start with `?`).
Similarly, a standalone `?--flag` at the end of the list is always included
because the bounds check `i + 1 < templateArgs.size()` fails silently.
Recommendation: document that the `?` **must be applied to both the flag and
its value** for the pair to work, or add a validation check at template
registration time that warns when a `?`-prefixed literal is followed by a
non-optional arg.
--
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]
