jerryshao commented on code in PR #9960:
URL: https://github.com/apache/gravitino/pull/9960#discussion_r2893683576
##########
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()
+ && i + 1 < templateArgs.size()) {
+ String nextArg = templateArgs.get(i + 1);
+ if (nextArg.startsWith("?")) {
+ String cleanNextArg = nextArg.substring(1);
+ String replacedNextArg = replacePlaceholder(cleanNextArg, jobConf);
+ if (isEmptyValue(replacedNextArg)) {
+ // Skip both this arg and next arg
+ i++; // Skip the next argument
+ continue;
+ }
+ }
+ }
+
+ result.add(replacedArg);
+ }
+
+ return result;
+ }
+
+ /**
+ * Check if a value should be considered empty for optional argument
filtering.
+ *
+ * @param value The value to check
+ * @return true if the value is null, empty, contains only whitespace, or is
an unreplaced
+ * placeholder
+ */
+ @VisibleForTesting
+ static boolean isEmptyValue(@Nullable String value) {
+ if (value == null || value.trim().isEmpty()) {
+ return true;
+ }
+ // Also consider unreplaced placeholders as empty (e.g., "{{key}}")
+ return PLACEHOLDER_PATTERN.matcher(value).matches();
+ }
Review Comment:
**`.matches()` only covers a pure single-placeholder string — compound args
slip through.**
`PLACEHOLDER_PATTERN.matcher(value).matches()` anchors to the entire string.
An arg whose template is `?{{prefix}}/{{suffix}}` (e.g. for a path like
`s3://bucket/path`) will resolve to `"{{prefix}}/{{suffix}}"` when both keys
are absent. `isEmptyValue` returns `false` here, so the raw placeholder string
leaks into the final command.
Consider using `.find()` instead of `.matches()`, or checking whether the
resolved string still contains any placeholder via
`PLACEHOLDER_PATTERN.matcher(value).find()`:
```java
return PLACEHOLDER_PATTERN.matcher(value).find() &&
value.replaceAll("\\{\\{[\\w.-]+\\}\\}", "").isBlank();
```
Or more simply: after replacement, if the result still contains `{{...}}`
patterns and nothing else meaningful, treat it as empty.
##########
core/src/test/java/org/apache/gravitino/job/TestJobManager.java:
##########
@@ -899,4 +901,125 @@ private JobEntity newJobEntity(String templateName,
JobHandle.Status status) {
AuditInfo.builder().withCreator("test").withCreateTime(Instant.now()).build())
.build();
}
+
+ @Test
+ public void testBuildArgumentsWithOptional() {
+ // Test Case 1: No optional arguments provided
+ List<String> templateArgs1 =
+ Lists.newArrayList(
+ "--catalog", "{{catalog}}", "--table", "{{table}}", "?--strategy",
"?{{strategy}}");
+ Map<String, String> jobConf1 = ImmutableMap.of("catalog", "hive", "table",
"db.table");
+
+ List<String> result1 =
JobManager.buildArgumentsWithOptional(templateArgs1, jobConf1);
+ Assertions.assertEquals(
+ Lists.newArrayList("--catalog", "hive", "--table", "db.table"),
result1);
+
+ // Test Case 2: One optional argument provided
+ Map<String, String> jobConf2 =
+ ImmutableMap.of("catalog", "hive", "table", "db.table", "strategy",
"binpack");
+
+ List<String> result2 =
JobManager.buildArgumentsWithOptional(templateArgs1, jobConf2);
+ Assertions.assertEquals(
+ Lists.newArrayList("--catalog", "hive", "--table", "db.table",
"--strategy", "binpack"),
+ result2);
+
+ // Test Case 3: Empty string should be filtered
+ Map<String, String> jobConf3 =
+ ImmutableMap.of("catalog", "hive", "table", "db.table", "strategy",
"");
+
+ List<String> result3 =
JobManager.buildArgumentsWithOptional(templateArgs1, jobConf3);
+ Assertions.assertEquals(
+ Lists.newArrayList("--catalog", "hive", "--table", "db.table"),
result3);
+
+ // Test Case 4: Whitespace should be filtered
+ Map<String, String> jobConf4 =
+ ImmutableMap.of("catalog", "hive", "table", "db.table", "strategy", "
");
+
+ List<String> result4 =
JobManager.buildArgumentsWithOptional(templateArgs1, jobConf4);
+ Assertions.assertEquals(
+ Lists.newArrayList("--catalog", "hive", "--table", "db.table"),
result4);
Review Comment:
**Missing edge case tests:**
1. `?--flag` as the **last** argument in the list (pair check silently skips
due to bounds) — should be covered to pin the current behavior.
2. `["?--flag", "{{val}}"]` where the value is non-optional — `?--flag` is
always emitted; this surprising behavior needs a test to document the
expectation.
3. `?{{prefix}}/{{suffix}}` (compound arg with two unreplaced placeholders)
— currently leaks as a raw placeholder string; test should document the current
behavior or the fix.
--
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]
