Support PPL foreach command

[songkant-aws]

Description

Adds the PPL foreach command (Splunk-compatible), which runs a templated eval for each field in a field list, each element of a multivalue (array) field, or each element of a JSON array.

# multifield: double every matching field
source=idx | foreach value_* [ eval <<FIELD>>_double = <<FIELD>> * 2 ]

# multivalue: fold array elements into an accumulator
source=idx | eval nums = array(1,2,3), total = 0
           | foreach mode=multivalue nums [ eval total = total + <<ITEM>> ]

# json_array: parse JSON array text (literal or field) and iterate
source=idx | eval total = 0
           | foreach mode=json_array '[1,2,3]' [ eval total = total + <<ITEM>> ]

All four Splunk modes are supported: multifield (default), multivalue, json_array, and auto_collections (runtime detection). Placeholders <<FIELD>>/<<MATCHSTR>> (multifield) and <<ITEM>>/<<ITER>> (collection modes) are supported, with fieldstr/matchstr/itemstr/iterstr rename options.

Design

• Multifield mode expands each eval clause once per matching field via placeholder bindings on CalcitePlanContext; <<FIELD>> resolves to the current row field, <<MATCHSTR>> to the wildcard-captured segment.
• Collection modes rewrite each eval clause into a reduce call over the collection. Elements are packed into internal pairs [item, iter, captured-field...] by a foreach_pair_collection UDF so the two-parameter reduce lambda can reach the loop item, the loop index, and any referenced row fields; foreach_pair_item(pair, slot) extracts a slot with its plan-time type attached. Bindings are staged on the context and only activate inside the cloned lambda context, so the reduce call's own arguments resolve against the row normally.
• json_array element typing: json_array(...) calls and string literals are inspected at plan time (mixed string/number arrays rejected); for opaque expressions (a field holding JSON text — the primary Splunk use), the type is inferred from usage: arithmetic on <<ITEM>> selects DOUBLE, otherwise VARCHAR.
• Planning lives in a dedicated ForeachPlanner class rather than growing CalciteRelNodeVisitor.

Behavior verified against Splunk 10.4.0

Scenario	Splunk	This PR
mv field + multivalue	6	6 ✓
JSON-text field + json_array (sum of "[10,20,30]")	60	60 ✓
json_array mode fed a real array	silent no-op	iterates (more permissive, pinned in IT)
multivalue mode fed JSON text	silent no-op	plan-time error (louder, pinned in IT)

Known limitation (documented in tests and user doc): fields whose mapping is a scalar type (e.g. long) holding arrays are rejected by multivalue mode, because OpenSearch mappings do not declare array-ness and the plan-time type is scalar. Nested-typed fields map to ARRAY<ANY> and iterate correctly.

Related Issues

N/A (new PPL command)

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[songkant-aws]

Note for reviewers: two items from the new-command checklist are still pending and I'll push them to this PR shortly:

• PPLQueryDataAnonymizer.visitForeach (query anonymization)
• v2 Analyzer.visitForeach throwing the standard "only for Calcite" exception (currently the v2 path behavior is undefined)

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit f3957f4)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue In wildcardSegments, if the pattern contains no wildcards but matches the field name, an empty list is returned. Later code at line 163 calls String.join("", segments) on this empty list, producing an empty string for <<MATCHSTR>>. This may be intentional, but if a non-wildcard pattern like "exact_field" is used and matches, <<MATCHSTR>> becomes empty rather than the full field name or an error. Verify this is the desired behavior for exact-match patterns. private List<String> wildcardSegments(String pattern, String fieldName) { if (!WildcardUtils.matchesWildcardPattern(pattern, fieldName)) { return null; } if (!WildcardUtils.containsWildcard(pattern)) { return List.of(); Possible Issue itemRequiresNumericType recursively walks the AST to infer whether <<ITEM>> is used in numeric context. If the expression is deeply nested or contains cycles (though AST cycles are unlikely), this could cause a stack overflow. The method has no depth limit or visited-node tracking. Consider adding a recursion depth guard if expressions can be arbitrarily deep. private boolean itemRequiresNumericType( Node node, String itemName, boolean customIdentifier, CalcitePlanContext context) { if (node instanceof Compare compare) { if ((containsItem(compare.getLeft(), itemName, customIdentifier) && isNumericExpression(compare.getRight(), context)) || (containsItem(compare.getRight(), itemName, customIdentifier) && isNumericExpression(compare.getLeft(), context))) { return true; } } if (node instanceof Function function) { for (int i = 0; i < function.getFuncArgs().size(); i++) { if (containsItem(function.getFuncArgs().get(i), itemName, customIdentifier) && (ARITHMETIC_OPERATORS.contains(function.getFuncName()) || PPLFuncImpTable.INSTANCE.requiresNumericArgument(function.getFuncName(), i))) { return true; } } } return node.getChild().stream() .anyMatch(child -> itemRequiresNumericType(child, itemName, customIdentifier, context)); } Possible Issue eval returns null when index >= pair.size(), but does not validate that pair is actually a list or array. If args[0] is an unexpected type (not Object[] or List), the cast at line 60 will throw ClassCastException. Add a type check or document that callers must ensure args[0] is a valid pair structure. public static Object eval(Object... args) { if (args.length < 2 || args[0] == null || args[1] == null) { return null; } int index = ((Number) args[1]).intValue(); List<?> pair = args[0] instanceof Object[] array ? Arrays.asList(array) : (List<?>) args[0]; return index < 0 || index >= pair.size() ? null : pair.get(index); } Possible Issue cast at line 86 handles JsonElement with isJsonNull() returning "null" string for VARCHAR or null for other types. However, if elementType is DOUBLE and the element is JsonNull, line 101 returns null, but if it's a primitive holding null (unlikely but possible in malformed data), ((Number) value).doubleValue() at line 101 could throw NullPointerException. Ensure value is checked for null before casting to Number. private static Object cast(Object value, SqlTypeName elementType) { if (value instanceof JsonElement element) { if (element.isJsonNull()) { return elementType == SqlTypeName.VARCHAR ? "null" : null; } if (element.isJsonArray() || element.isJsonObject()) { return element.toString(); } value = element.getAsJsonPrimitive().isNumber() ? element.getAsNumber() : element.getAsString(); } if (value == null) { return null; } return switch (elementType) { case DOUBLE -> ((Number) value).doubleValue(); case VARCHAR -> value instanceof List<?> || value instanceof java.util.Map<?, ?> ? gson.toJson(value) : String.valueOf(value); case DECIMAL -> BigDecimal.valueOf(((Number) value).doubleValue()); default -> value; }; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to f3957f4

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Validate argument type before casting The cast (Number) args[1] assumes the second argument is always a Number, but if it's not, a ClassCastException will be thrown at runtime. Add a type check before casting to ensure args[1] is actually a Number, returning null if the type is incorrect. core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/ForeachPairItemFunctionImpl.java [55-62] public static Object eval(Object... args) { if (args.length < 2 || args[0] == null || args[1] == null) { + return null; + } + if (!(args[1] instanceof Number)) { return null; } int index = ((Number) args[1]).intValue(); List<?> pair = args[0] instanceof Object[] array ? Arrays.asList(array) : (List<?>) args[0]; return index < 0 || index >= pair.size() ? null : pair.get(index); } Suggestion importance[1-10]: 8 __ Why: The unchecked cast (Number) args[1] will throw ClassCastException if the argument is not a Number. Adding type validation before casting prevents runtime crashes and aligns with the defensive null checks already present in the method.	Medium
	Add null check for node parameter The recursive itemRequiresNumericType method lacks a base case check for null nodes, which could cause a NullPointerException when traversing the AST. Add a null guard at the start of the recursive variant to prevent crashes on malformed or incomplete expression trees. core/src/main/java/org/opensearch/sql/calcite/ForeachPlanner.java [481-501] -private boolean itemRequiresNumericType(Foreach node, CalcitePlanContext context) { - String itemName = node.getOptions().getOrDefault(OPTION_ITEMSTR, PLACEHOLDER_ITEM); - boolean customIdentifier = node.getOptions().containsKey(OPTION_ITEMSTR); - return node.getEvalClauses().stream() - .anyMatch( - clause -> - itemRequiresNumericType( - clause.getExpression(), itemName, customIdentifier, context)); -} +private boolean itemRequiresNumericType( + Node node, String itemName, boolean customIdentifier, CalcitePlanContext context) { + if (node == null) { + return false; + } + if (node instanceof Compare compare) { + ... Suggestion importance[1-10]: 7 __ Why: The recursive method itemRequiresNumericType could encounter null nodes when traversing AST structures. Adding a null guard prevents potential NullPointerException and improves robustness, though the impact depends on whether null nodes actually occur in practice.	Medium
	Handle invalid regex pattern compilation The convertWildcardPatternToRegex method may produce invalid regex patterns for certain wildcard inputs, causing PatternSyntaxException. Wrap the Pattern.compile call in a try-catch block to handle malformed patterns gracefully and return null when regex compilation fails. core/src/main/java/org/opensearch/sql/calcite/ForeachPlanner.java [173-189] private List<String> wildcardSegments(String pattern, String fieldName) { if (!WildcardUtils.matchesWildcardPattern(pattern, fieldName)) { return null; } if (!WildcardUtils.containsWildcard(pattern)) { return List.of(); } - Matcher matcher = - Pattern.compile(WildcardUtils.convertWildcardPatternToRegex(pattern)).matcher(fieldName); - if (!matcher.matches()) { + try { + Matcher matcher = + Pattern.compile(WildcardUtils.convertWildcardPatternToRegex(pattern)).matcher(fieldName); + if (!matcher.matches()) { + return null; + } + ... + } catch (java.util.regex.PatternSyntaxException e) { return null; } - ... Suggestion importance[1-10]: 7 __ Why: The Pattern.compile call could throw PatternSyntaxException if convertWildcardPatternToRegex produces invalid regex. Adding exception handling prevents crashes from malformed patterns and returns a safe null value, improving robustness.	Medium
	Handle exceptions during argument analysis The analyze call on function arguments can throw exceptions if arguments are invalid or unresolvable, but these exceptions are not caught. Wrap the stream operations in a try-catch block to handle potential analysis failures gracefully and return a safe default type (VARCHAR) instead of propagating the exception. core/src/main/java/org/opensearch/sql/calcite/ForeachPlanner.java [439-451] private SqlTypeName jsonElementType( UnresolvedExpression collection, CalcitePlanContext context, Foreach node) { if (collection instanceof Function function && BuiltinFunctionName.JSON_ARRAY .getName() .getFunctionName() .equalsIgnoreCase(function.getFuncName())) { - return elementTypeOf( - function.getFuncArgs().stream() - .map(arg -> rexVisitor.analyze(arg, context).getType()) - .map(RelDataType::getFamily) - .collect(Collectors.toSet())); + try { + return elementTypeOf( + function.getFuncArgs().stream() + .map(arg -> rexVisitor.analyze(arg, context).getType()) + .map(RelDataType::getFamily) + .collect(Collectors.toSet())); + } catch (RuntimeException e) { + return SqlTypeName.VARCHAR; + } } ... Suggestion importance[1-10]: 6 __ Why: The analyze call on function arguments could throw exceptions for invalid expressions. Wrapping in try-catch improves error handling, though the existing code at line 540 already catches RuntimeException in a similar context, suggesting this pattern may be intentional.	Low

---

Previous suggestions

Suggestions up to commit 273e330

Category	Suggestion	Impact
Possible issue	Handle exceptions during argument type analysis The analyze call on function arguments can throw runtime exceptions if arguments are malformed or unresolvable. Wrap the stream operations in a try-catch block to gracefully fall back to VARCHAR when type inference fails, preventing query planning crashes. core/src/main/java/org/opensearch/sql/calcite/ForeachPlanner.java [443-450] -private SqlTypeName jsonElementType( - UnresolvedExpression collection, CalcitePlanContext context, Foreach node) { - if (collection instanceof Function function - && BuiltinFunctionName.JSON_ARRAY - .getName() - .getFunctionName() - .equalsIgnoreCase(function.getFuncName())) { +if (collection instanceof Function function + && BuiltinFunctionName.JSON_ARRAY + .getName() + .getFunctionName() + .equalsIgnoreCase(function.getFuncName())) { + try { return elementTypeOf( function.getFuncArgs().stream() .map(arg -> rexVisitor.analyze(arg, context).getType()) .map(RelDataType::getFamily) .collect(Collectors.toSet())); + } catch (RuntimeException e) { + return SqlTypeName.VARCHAR; } - ... } Suggestion importance[1-10]: 8 __ Why: The suggestion addresses a real risk where malformed arguments could crash query planning. The existing code at line 540 already shows a try-catch pattern for similar analysis, making this consistent error handling. Wrapping the stream operations prevents planning failures and provides graceful degradation.	Medium
	Add null check for node parameter The recursive itemRequiresNumericType method lacks a base case check for null nodes, which could cause a NullPointerException if the AST contains null children. Add a null guard at the start of the recursive variant to prevent traversal failures. core/src/main/java/org/opensearch/sql/calcite/ForeachPlanner.java [481-501] -private boolean itemRequiresNumericType(Foreach node, CalcitePlanContext context) { - String itemName = node.getOptions().getOrDefault(OPTION_ITEMSTR, PLACEHOLDER_ITEM); - boolean customIdentifier = node.getOptions().containsKey(OPTION_ITEMSTR); - return node.getEvalClauses().stream() - .anyMatch( - clause -> - itemRequiresNumericType( - clause.getExpression(), itemName, customIdentifier, context)); +private boolean itemRequiresNumericType( + Node node, String itemName, boolean customIdentifier, CalcitePlanContext context) { + if (node == null) { + return false; + } + if (node instanceof Compare compare) { + ... + } + ... } Suggestion importance[1-10]: 7 __ Why: Adding a null check prevents potential NullPointerException when traversing AST nodes. While the AST structure typically doesn't contain null children, defensive programming improves robustness. The suggestion correctly identifies a potential issue and provides a simple fix.	Medium
General	Fail fast on unresolved template placeholders When a placeholder in a template literal is unresolved (binding == null), the method silently skips it and continues matching. This produces incorrect output where <> remains in the final string instead of failing fast. Throw a SemanticCheckException when an unresolved placeholder is encountered to alert users immediately. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [440-479] -private RexNode foreachTemplateLiteral(String value, CalcitePlanContext context) { - Matcher matcher = FOREACH_TEMPLATE.matcher(value); - List<RexNode> parts = new ArrayList<>(); - int start = 0; - boolean replaced = false; - while (matcher.find()) { - ForeachBinding binding = foreachBinding(matcher.group(1), context); - if (binding == null) { - continue; - } - ... +while (matcher.find()) { + ForeachBinding binding = foreachBinding(matcher.group(1), context); + if (binding == null) { + throw new SemanticCheckException("Unresolved foreach placeholder <<" + matcher.group(1) + ">>"); } ... } Suggestion importance[1-10]: 3 __ Why: The suggestion misunderstands the intended behavior. The code at line 448 shows continue is deliberate: unresolved placeholders in string literals are left as-is (e.g., "<<UNKNOWN>>" remains literal text), which matches Splunk's behavior. Only direct placeholder expressions (handled by visitForeachPlaceholder at line 428) throw errors. The suggestion would break valid use cases.	Low

Suggestions up to commit 273e330

Category	Suggestion	Impact
Possible issue	Add null check for node parameter The recursive itemRequiresNumericType method lacks a base case check for null nodes, which could cause a NullPointerException if the AST contains unexpected null children. Add a null guard at the start of the recursive variant to prevent traversal failures. core/src/main/java/org/opensearch/sql/calcite/ForeachPlanner.java [481-501] -private boolean itemRequiresNumericType(Foreach node, CalcitePlanContext context) { - String itemName = node.getOptions().getOrDefault(OPTION_ITEMSTR, PLACEHOLDER_ITEM); - boolean customIdentifier = node.getOptions().containsKey(OPTION_ITEMSTR); - return node.getEvalClauses().stream() - .anyMatch( - clause -> - itemRequiresNumericType( - clause.getExpression(), itemName, customIdentifier, context)); +private boolean itemRequiresNumericType( + Node node, String itemName, boolean customIdentifier, CalcitePlanContext context) { + if (node == null) { + return false; + } + if (node instanceof Compare compare) { + ... + } + ... } Suggestion importance[1-10]: 7 __ Why: The recursive itemRequiresNumericType method could encounter null nodes in the AST. Adding a null guard prevents potential NullPointerException and improves robustness, though the current codebase may not produce null children in practice.	Medium
General	Handle exceptions during argument analysis The analyze call on function arguments can throw exceptions if arguments are malformed or unresolvable, but these are not caught. Wrap the stream operations in a try-catch to handle analysis failures gracefully and default to VARCHAR when type inference is impossible. core/src/main/java/org/opensearch/sql/calcite/ForeachPlanner.java [439-451] private SqlTypeName jsonElementType( UnresolvedExpression collection, CalcitePlanContext context, Foreach node) { if (collection instanceof Function function && BuiltinFunctionName.JSON_ARRAY .getName() .getFunctionName() .equalsIgnoreCase(function.getFuncName())) { - return elementTypeOf( - function.getFuncArgs().stream() - .map(arg -> rexVisitor.analyze(arg, context).getType()) - .map(RelDataType::getFamily) - .collect(Collectors.toSet())); + try { + return elementTypeOf( + function.getFuncArgs().stream() + .map(arg -> rexVisitor.analyze(arg, context).getType()) + .map(RelDataType::getFamily) + .collect(Collectors.toSet())); + } catch (RuntimeException e) { + return SqlTypeName.VARCHAR; + } } ... } Suggestion importance[1-10]: 6 __ Why: The analyze call on function arguments can throw exceptions if arguments are malformed. Wrapping in try-catch and defaulting to VARCHAR improves error handling, though the impact is moderate since malformed arguments would likely fail earlier in the pipeline.	Low
	Handle regex compilation exceptions The regex compilation from convertWildcardPatternToRegex can throw PatternSyntaxException if the wildcard pattern produces invalid regex. Wrap the Pattern.compile call in a try-catch to handle malformed patterns and return null to signal no match. core/src/main/java/org/opensearch/sql/calcite/ForeachPlanner.java [173-189] private List<String> wildcardSegments(String pattern, String fieldName) { if (!WildcardUtils.matchesWildcardPattern(pattern, fieldName)) { return null; } if (!WildcardUtils.containsWildcard(pattern)) { return List.of(); } - Matcher matcher = - Pattern.compile(WildcardUtils.convertWildcardPatternToRegex(pattern)).matcher(fieldName); - if (!matcher.matches()) { + try { + Matcher matcher = + Pattern.compile(WildcardUtils.convertWildcardPatternToRegex(pattern)).matcher(fieldName); + if (!matcher.matches()) { + return null; + } + ... + } catch (PatternSyntaxException e) { return null; } - ... } Suggestion importance[1-10]: 6 __ Why: The Pattern.compile call could throw PatternSyntaxException if the wildcard pattern produces invalid regex. Adding exception handling prevents crashes from malformed patterns, though WildcardUtils likely validates patterns before conversion.	Low
	Guard against regex group extraction failures The matcher.group(1) call can throw IllegalStateException if the match operation fails or the group index is invalid. Although the pattern is controlled, defensive programming suggests wrapping group extraction in a try-catch to prevent unexpected crashes from regex engine anomalies. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [440-479] private RexNode foreachTemplateLiteral(String value, CalcitePlanContext context) { Matcher matcher = FOREACH_TEMPLATE.matcher(value); List<RexNode> parts = new ArrayList<>(); int start = 0; boolean replaced = false; while (matcher.find()) { - ForeachBinding binding = foreachBinding(matcher.group(1), context); - if (binding == null) { + try { + ForeachBinding binding = foreachBinding(matcher.group(1), context); + if (binding == null) { + continue; + } + if (matcher.start() > start) { + parts.add(context.rexBuilder.makeLiteral(value.substring(start, matcher.start()))); + } + ... + start = matcher.end(); + replaced = true; + } catch (IllegalStateException e) { continue; } - if (matcher.start() > start) { - parts.add(context.rexBuilder.makeLiteral(value.substring(start, matcher.start()))); - } - ... - start = matcher.end(); - replaced = true; } ... } Suggestion importance[1-10]: 5 __ Why: The matcher.group(1) call could theoretically throw IllegalStateException, though the pattern FOREACH_TEMPLATE is controlled and should always have group 1. This is defensive programming with low practical impact since the pattern is well-defined.	Low

Suggestions up to commit e293461

Category	Suggestion	Impact
General	Log JSON parsing exceptions The method catches JsonSyntaxException but silently ignores it, returning VARCHAR as a fallback. This masks potential JSON parsing errors that could indicate data quality issues. Consider logging the exception to aid debugging. core/src/main/java/org/opensearch/sql/calcite/ForeachPlanner.java [310-324] private SqlTypeName jsonElementType( UnresolvedExpression collection, CalcitePlanContext context, List<ForeachEvalClause> evalClauses, String itemName) { if (collection instanceof Function function && BuiltinFunctionName.JSON_ARRAY .getName() .getFunctionName() .equalsIgnoreCase(function.getFuncName())) { return elementTypeOf( function.getFuncArgs().stream() .map(arg -> rexVisitor.analyze(arg, context).getType()) .map(RelDataType::getFamily) .collect(Collectors.toSet())); } if (collection instanceof Literal literal && literal.getType() == DataType.STRING) { try { List<?> values = gson.fromJson(String.valueOf(literal.getValue()), List.class); if (values != null) { return elementTypeOf( values.stream() .filter(Objects::nonNull) .map(v -> v instanceof Number ? SqlTypeFamily.NUMERIC : SqlTypeFamily.CHARACTER) .collect(Collectors.toSet())); } - } catch (JsonSyntaxException ignored) { - ... + } catch (JsonSyntaxException e) { + log.debug("Failed to parse JSON literal for type inference: {}", literal.getValue(), e); } return SqlTypeName.VARCHAR; } return itemUsedInArithmetic(evalClauses, itemName) ? SqlTypeName.DOUBLE : SqlTypeName.VARCHAR; } Suggestion importance[1-10]: 5 __ Why: Adding logging for the caught JsonSyntaxException would aid debugging and improve observability. However, the comment in the code indicates that malformed JSON literals are expected to return null at runtime, suggesting this is intentional behavior rather than an error condition. The improvement is moderate.	Low
	Handle conversion exceptions gracefully The method does not validate that toList successfully converts the input. If toList throws an exception for unsupported types, it will propagate unchecked. Add explicit validation or handle the exception to prevent unexpected runtime failures. core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/ForeachPairCollectionFunctionImpl.java [57-73] public static Object eval(Object... args) { if (args.length == 0 || args[0] == null) { return null; } - List<?> source = toList(args[0]); - List<Object[]> pairs = new ArrayList<>(); - for (int i = 0; i < source.size(); i++) { - Object[] pair = new Object[args.length + 1]; - pair[0] = source.get(i); - pair[1] = i; - for (int j = 1; j < args.length; j++) { - pair[j + 1] = args[j]; + try { + List<?> source = toList(args[0]); + List<Object[]> pairs = new ArrayList<>(); + for (int i = 0; i < source.size(); i++) { + Object[] pair = new Object[args.length + 1]; + pair[0] = source.get(i); + pair[1] = i; + for (int j = 1; j < args.length; j++) { + pair[j + 1] = args[j]; + } + pairs.add(pair); } - pairs.add(pair); + return pairs; + } catch (IllegalArgumentException e) { + return null; } - return pairs; } Suggestion importance[1-10]: 4 __ Why: The suggestion to catch IllegalArgumentException from toList and return null improves robustness. However, the toList method already throws IllegalArgumentException with a descriptive message, and catching it here would suppress that information. The improvement is minor and may hide useful error details.	Low
	Remove redundant pattern matching check The method performs redundant pattern matching. After checking matchesWildcardPattern returns true, the subsequent matcher.matches() check is unnecessary and will always succeed. Remove the second null check to avoid redundant regex evaluation. core/src/main/java/org/opensearch/sql/calcite/ForeachPlanner.java [153-170] private List<String> wildcardSegments(String pattern, String fieldName) { if (!WildcardUtils.matchesWildcardPattern(pattern, fieldName)) { return null; } if (!WildcardUtils.containsWildcard(pattern)) { return List.of(); } Matcher matcher = Pattern.compile(WildcardUtils.convertWildcardPatternToRegex(pattern)).matcher(fieldName); - if (!matcher.matches()) { - return null; - } + matcher.matches(); List<String> segments = new ArrayList<>(); for (int i = 1; i <= matcher.groupCount(); i++) { segments.add(matcher.group(i)); } return segments; } Suggestion importance[1-10]: 3 __ Why: The suggestion identifies a redundant check, but the second matcher.matches() call is necessary to populate the matcher's groups before calling matcher.group(i). Removing the null check without calling matches() would break the code, and calling matches() without checking its result doesn't eliminate the redundancy meaningfully.	Low

Suggestions up to commit 2c04b27

Category	Suggestion	Impact
Possible issue	Validate type before casting to List The cast (List<?>) args[0] assumes args[0] is a List if it's not an Object[], but this assumption is not validated. If args[0] is neither type, a ClassCastException will occur at runtime. Add explicit type checking or handle unexpected types gracefully. core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/ForeachPairItemFunctionImpl.java [55-62] public static Object eval(Object... args) { if (args.length < 2 || args[0] == null || args[1] == null) { return null; } int index = ((Number) args[1]).intValue(); - List<?> pair = args[0] instanceof Object[] array ? Arrays.asList(array) : (List<?>) args[0]; + List<?> pair; + if (args[0] instanceof Object[] array) { + pair = Arrays.asList(array); + } else if (args[0] instanceof List<?> list) { + pair = list; + } else { + return null; + } return index < 0 || index >= pair.size() ? null : pair.get(index); } Suggestion importance[1-10]: 7 __ Why: The suggestion addresses a potential ClassCastException by adding explicit type checking before casting. This improves robustness by handling unexpected input types gracefully, though the current code may work if the caller guarantees the input type.	Medium
General	Clarify null handling in JSON parsing When gson.fromJson returns null or throws JsonSyntaxException, the method falls through to return VARCHAR. However, if values is null, the code should explicitly return VARCHAR rather than falling through, making the logic clearer and preventing potential issues if the code structure changes. core/src/main/java/org/opensearch/sql/calcite/ForeachPlanner.java [287-326] -private SqlTypeName jsonElementType( - UnresolvedExpression collection, - CalcitePlanContext context, - List<ForeachEvalClause> evalClauses, - String itemName) { - if (collection instanceof Function function - && BuiltinFunctionName.JSON_ARRAY - .getName() - .getFunctionName() - .equalsIgnoreCase(function.getFuncName())) { +if (collection instanceof Literal literal && literal.getType() == DataType.STRING) { + try { + List<?> values = gson.fromJson(String.valueOf(literal.getValue()), List.class); + if (values == null) { + return SqlTypeName.VARCHAR; + } return elementTypeOf( - function.getFuncArgs().stream() - .map(arg -> rexVisitor.analyze(arg, context).getType()) - .map(RelDataType::getFamily) + values.stream() + .filter(Objects::nonNull) + .map(v -> v instanceof Number ? SqlTypeFamily.NUMERIC : SqlTypeFamily.CHARACTER) .collect(Collectors.toSet())); - } - if (collection instanceof Literal literal && literal.getType() == DataType.STRING) { - try { - List<?> values = gson.fromJson(String.valueOf(literal.getValue()), List.class); - if (values != null) { - return elementTypeOf( - values.stream() - .filter(Objects::nonNull) - .map(v -> v instanceof Number ? SqlTypeFamily.NUMERIC : SqlTypeFamily.CHARACTER) - .collect(Collectors.toSet())); - } - } catch (JsonSyntaxException ignored) { - // Malformed JSON literals return null at runtime; type choice is irrelevant. - } + } catch (JsonSyntaxException ignored) { return SqlTypeName.VARCHAR; } - return itemUsedInArithmetic(evalClauses, itemName) ? SqlTypeName.DOUBLE : SqlTypeName.VARCHAR; } Suggestion importance[1-10]: 6 __ Why: The suggestion improves code clarity by making the null handling explicit rather than relying on fall-through logic. This makes the code more maintainable and less prone to errors if the structure changes, though the current implementation is functionally correct.	Low
	Remove redundant pattern matching check The method checks matchesWildcardPattern first, then compiles a regex and checks matcher.matches() again. If the first check passes, the second should always pass. Remove the redundant second null check to avoid confusion and potential logic errors. core/src/main/java/org/opensearch/sql/calcite/ForeachPlanner.java [153-169] private List<String> wildcardSegments(String pattern, String fieldName) { if (!WildcardUtils.matchesWildcardPattern(pattern, fieldName)) { return null; } if (!WildcardUtils.containsWildcard(pattern)) { return List.of(); } Matcher matcher = Pattern.compile(WildcardUtils.convertWildcardPatternToRegex(pattern)).matcher(fieldName); - if (!matcher.matches()) { - return null; + List<String> segments = new ArrayList<>(); + for (int i = 1; i <= matcher.groupCount(); i++) { + segments.add(matcher.group(i)); } - ... + return segments; } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a redundant check after matchesWildcardPattern. However, the redundancy may be intentional for defensive programming or due to different implementations of matchesWildcardPattern and regex matching. The impact is minor since it only affects code clarity.	Low
	Handle invalid input types gracefully The method throws IllegalArgumentException for unexpected input types, but the eval method that calls it only checks if args[0] is null, not if it's a valid collection type. This can lead to runtime exceptions. Consider returning null instead of throwing an exception to maintain consistency with other null-handling patterns in the codebase. core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/ForeachPairCollectionFunctionImpl.java [75-83] private static List<?> toList(Object value) { if (value instanceof List<?> list) { return list; } if (value instanceof Object[] array) { return List.of(array); } - throw new IllegalArgumentException("foreach pair collection requires an array input"); + return null; } Suggestion importance[1-10]: 4 __ Why: The suggestion proposes returning null instead of throwing an exception for invalid input types. While this may improve consistency with other null-handling patterns, throwing an exception for invalid input is often the correct behavior to fail fast and provide clear error messages.	Low

Suggestions up to commit 38c6ca9

Category	Suggestion	Impact
General	Add type validation for robustness The cast (List<?>) args[0] assumes args[0] is a List when it's not an array, but this assumption is not validated. If args[0] is neither an array nor a List, a ClassCastException will occur at runtime. Add validation to handle unexpected types gracefully. core/src/main/java/org/opensearch/sql/expression/function/CollectionUDF/ForeachPairItemFunctionImpl.java [55-62] public static Object eval(Object... args) { if (args.length < 2 || args[0] == null || args[1] == null) { return null; } int index = ((Number) args[1]).intValue(); - List<?> pair = args[0] instanceof Object[] array ? Arrays.asList(array) : (List<?>) args[0]; + List<?> pair; + if (args[0] instanceof Object[] array) { + pair = Arrays.asList(array); + } else if (args[0] instanceof List<?> list) { + pair = list; + } else { + return null; + } return index < 0 || index >= pair.size() ? null : pair.get(index); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential ClassCastException when args[0] is neither an array nor a List. Adding explicit type checking improves robustness and prevents runtime errors, though the current implementation may assume controlled input from the planner.	Medium
	Improve error message clarity When collection is null after attempting firstArrayField, the error message incorrectly states the mode requires "a field" even though AUTO_COLLECTIONS mode is designed to work without an explicit field by auto-detecting one. The error should clarify that no suitable array field was found when auto-detection fails. core/src/main/java/org/opensearch/sql/calcite/ForeachPlanner.java [174-182] private RelNode planCollection(Foreach node, CalcitePlanContext context) { Foreach.Mode mode = node.getMode(); UnresolvedExpression collection = node.getCollectionExpression(); if (collection == null && mode == Foreach.Mode.AUTO_COLLECTIONS) { collection = firstArrayField(context); } if (collection == null) { - throw new SemanticCheckException("foreach " + mode + " mode requires a field"); + String message = mode == Foreach.Mode.AUTO_COLLECTIONS + ? "foreach auto_collections mode requires a multivalue field or JSON array" + : "foreach " + mode + " mode requires a field"; + throw new SemanticCheckException(message); } Suggestion importance[1-10]: 6 __ Why: The suggestion improves error message clarity for AUTO_COLLECTIONS mode. However, the error message at line 255-256 already provides this clarity, so the improvement is moderate rather than critical.	Low
	Eliminate redundant pattern matching The method performs redundant pattern matching by calling matchesWildcardPattern first, then compiling and matching the regex again. Since the regex matcher already determines if the pattern matches, the initial check is unnecessary and wastes computation. Remove the first matchesWildcardPattern call and rely solely on matcher.matches(). core/src/main/java/org/opensearch/sql/calcite/ForeachPlanner.java [153-169] private List<String> wildcardSegments(String pattern, String fieldName) { - if (!WildcardUtils.matchesWildcardPattern(pattern, fieldName)) { - return null; - } if (!WildcardUtils.containsWildcard(pattern)) { - return List.of(); + return WildcardUtils.matchesWildcardPattern(pattern, fieldName) ? List.of() : null; } Matcher matcher = Pattern.compile(WildcardUtils.convertWildcardPatternToRegex(pattern)).matcher(fieldName); if (!matcher.matches()) { return null; } List<String> segments = new ArrayList<>(); for (int i = 1; i <= matcher.groupCount(); i++) { segments.add(matcher.group(i)); } return segments; } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies redundant pattern matching, but the optimization is minor. The first check provides early exit for non-matching patterns before regex compilation, which may be intentional for performance in the non-matching case.	Low

[songkant-aws]

Both pending checklist items are now in (0e74b5d34):

• Analyzer.visitForeach throws the standard only-for-Calcite UnsupportedOperationException on the v2 path, with a UT asserting the message.
• PPLQueryDataAnonymizer.visitForeach renders the command with masked options/targets/eval clauses (collection targets can embed literals such as JSON array strings); ForeachPlaceholder masks like a column reference. Covered by three anonymizer UTs (multifield, multivalue with options, json_array with a literal target).

The new-command checklist is now fully satisfied.

[github-actions]

Persistent review updated to latest commit 0e74b5d

[github-actions]

Persistent review updated to latest commit 38c6ca9

[github-actions]

Persistent review updated to latest commit 2c04b27

[songkant-aws]

CI notes on the two failures from run 29075374167:

• build-linux (25, unit): infrastructure — Got socket exception during request. It might be caused by SSL misconfiguration while fetching from Maven; unrelated to this change (same suites pass locally and on the fork's CI).
• build-linux (21, integration): testNoMvBasic/testNoMvWithEval failed under the randomized az-Cyrl test locale — their default-locale toLowerCase() turns ARRAY_JOIN into array_joın (dotless i). Pre-existing bug in those tests (added by the timewrap PR, Support PPL timewrap command  #5241); reproduced locally with the CI seed and fixed in e2934613a by using Locale.ROOT.

[github-actions]

Persistent review updated to latest commit e293461

[noCharger]

multivalue fed JSON text hard-errors while Splunk silently no-op. Is this on purpose?

[github-actions]

Persistent review updated to latest commit cc8fef4

[github-actions]

Persistent review updated to latest commit 273e330

[songkant-aws]

@noCharger This was not intentional and is fixed in cc8fef4. The planner now checks the collection shape before wrapping it: mode=multivalue on JSON text and mode=json_array on a native array both leave the row unchanged, matching Splunk no-op behavior. The two directions are covered in CalcitePPLForeachTest, CalciteForeachCommandIT, and ForeachFieldJsonIT, and the affected foreach IT classes also pass with pushdown disabled.

[github-actions]

Persistent review updated to latest commit f3957f4