Skip to content

Support PPL format command - #5659

Open
songkant-aws wants to merge 10 commits into
opensearch-project:mainfrom
songkant-aws:feature/ppl-format-command
Open

Support PPL format command#5659
songkant-aws wants to merge 10 commits into
opensearch-project:mainfrom
songkant-aws:feature/ppl-format-command

Conversation

@songkant-aws

Copy link
Copy Markdown
Collaborator

Description

Adds the PPL format command, which collapses tabular results into a single search-expression string. It supports configurable row, column, and multivalue delimiters; maxresults; emptystr; null handling; and quote/backslash escaping.

source=web_logs | fields status, host | format

The command also supports runtime search predicates produced by subsearches:

search source=web_logs status>=500 [ search source=rules | fields host ]

The subsearch result is formatted into one scalar search string and combined with the static parent predicate before the OpenSearch request is executed. Explicit format output remains a normal one-row result and can continue through later pipeline commands.

Design

  • The grammar and AST represent format options explicitly, including the all-or-none positional delimiter group.
  • FormatPlanner lowers each input row to a formatted expression, aggregates rows globally, applies the row wrapper and fallback, and projects the single search field.
  • Search subqueries remain structured through parsing. When parent search requires an implicit formatted predicate, the planner builds an implicit Format scalar subquery.
  • RuntimeSearchCorrelator rewrites only scalar subqueries registered as implicit Format inputs into the left side of a LogicalCorrelate. Other scalar, IN, and EXISTS subqueries remain untouched.
  • The correlated right scan references the formatted string through its correlation variable. DynamicQueryStringSpec defers query-string compilation until the left input has produced its value.
  • The runtime query_string filter is appended through the Calcite pushdown path, so it is combined with an existing pushed filter using AND semantics rather than replacing it.
  • The legacy execution engine reports the command as Calcite-only.

Testing

  • Lexer, parser, AST, anonymizer, and search predicate compiler unit tests.
  • Full Calcite logical-plan tests for default options, custom delimiters, multivalue fields, escaping, empty input, explicit format, implicit format, multiple subsearches, and nested subsearches.
  • Integration tests for explicit format output, continued pipeline processing, static and dynamic predicate composition, NOT subsearches, multiple subsearches, nested subsearches, explain output, and analytics-engine execution.
  • Complete logical and physical explain-plan YAML golden files.
  • Scan-level coverage proving that an existing pushed filter and a late-bound dynamic query_string filter are conjoined.

Related Issues

Related to #5233

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 is not applicable because this change does not alter the REST API shape.
  • 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.

Signed-off-by: Songkan Tang <songkant@amazon.com>
Signed-off-by: Songkan Tang <songkant@amazon.com>
…mmand

Signed-off-by: Songkan Tang <songkant@amazon.com>
Signed-off-by: Songkan Tang <songkant@amazon.com>
Signed-off-by: Songkan Tang <songkant@amazon.com>
Signed-off-by: Songkan Tang <songkant@amazon.com>
Signed-off-by: Songkan Tang <songkant@amazon.com>
Signed-off-by: Songkan Tang <songkant@amazon.com>
Signed-off-by: Songkan Tang <songkant@amazon.com>
Signed-off-by: Songkan Tang <songkant@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

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

The planImplicitSearchField method limits the input to 1 row but does not verify that exactly one row was produced. If the input is empty, the aggregation will still produce one row with null values, and the CASE expression will incorrectly return the fallback row instead of recognizing that no raw search value exists. This can cause an empty subsearch to produce a formatted predicate when it should produce an empty result or match-none behavior.

private RelNode planImplicitSearchField(
    Format node,
    CalcitePlanContext context,
    List<RelDataTypeField> fields,
    RelDataTypeField searchField) {
  RelBuilder builder = context.relBuilder;
  builder.limit(0, 1);

  RexNode rawSearch = builder.cast(builder.field(searchField.getIndex()), SqlTypeName.VARCHAR);
  List<RexNode> fallbackFields =
      fields.stream()
          .filter(field -> field != searchField)
          .map(field -> formatField(field, node, context))
          .toList();
  RexNode fallbackRow = formatRow(fallbackFields, node, context);

  builder.project(List.of(rawSearch, fallbackRow), List.of(RAW_SEARCH_FIELD, FORMAT_ROW_FIELD));
  RexNode rawSearchRef = builder.field(RAW_SEARCH_FIELD);
  RexNode rowRef = builder.field(FORMAT_ROW_FIELD);
  builder.aggregate(
      builder.groupKey(),
      builder.aggregateCall(SqlStdOperatorTable.COUNT, rawSearchRef).as(RAW_SEARCH_COUNT_FIELD),
      builder.aggregateCall(SqlStdOperatorTable.MAX, rawSearchRef).as(RAW_SEARCH_VALUE_FIELD),
      builder.aggregateCall(SqlLibraryOperators.ARRAY_AGG, rowRef).as(FORMAT_ROWS_FIELD));

  RexNode hasRawSearch =
      builder.call(
          SqlStdOperatorTable.GREATER_THAN,
          builder.field(RAW_SEARCH_COUNT_FIELD),
          builder.literal(0));
  RexNode result =
      builder.call(
          SqlStdOperatorTable.CASE,
          hasRawSearch,
          builder.field(RAW_SEARCH_VALUE_FIELD),
          formatAggregatedRows(node, context));
  builder.project(List.of(result), List.of(SEARCH_FIELD), true);
  return builder.peek();
}
Possible Issue

The combineSubqueries method uses getFirst() on a list that is assumed to have at least one element, but the caller correlate does not enforce this precondition. If findImplicitFormatSubqueries returns an empty list, the check at line 58 prevents the call, but if it returns exactly one subquery, combineSubqueries is still called and will fail with getFirst() on an empty iteration at line 135. The logic should handle the single-subquery case without calling this method or ensure the method handles size 1 correctly.

private static RelNode combineSubqueries(List<RexSubQuery> subqueries, RexBuilder rexBuilder) {
  RelNode result = subqueries.getFirst().rel;
  for (int i = 1; i < subqueries.size(); i++) {
    result =
        LogicalJoin.create(
            result,
            subqueries.get(i).rel,
            List.of(),
            rexBuilder.makeLiteral(true),
            Set.of(),
            JoinRelType.INNER);
  }
  return result;
}
Possible Issue

The buildRuntimeQuery method iterates over queryParts and checks if each index is in runtimePredicateParts. However, if queryParts is empty or if runtimePredicateParts contains an index that is out of bounds for queryParts, the method will either produce an empty query string or fail. The code does not validate that runtimePredicateParts indices are within the bounds of queryParts, which could lead to incorrect query construction or an IndexOutOfBoundsException if the data structure is malformed.

private String buildRuntimeQuery(String[] queryParts) {
  StringBuilder query = new StringBuilder();
  for (int i = 0; i < queryParts.length; i++) {
    String part = queryParts[i];
    if (pushDownContext.getDynamicQueryString().runtimePredicateParts().contains(i)) {
      part = pushDownContext.getDynamicQueryString().compiler().compile(part);
    }
    query.append(part);
  }
  return query.toString();
}
Possible Issue

The formatFieldName method uses a regex to determine if a field name needs backtick quoting. However, the regex does not account for field names that are reserved keywords in PPL or OpenSearch query syntax (e.g., 'AND', 'OR', 'NOT'). If a field is named 'AND', the formatted output will be 'AND="value"', which will be misparsed as a boolean operator rather than a field name. Reserved words should always be quoted regardless of their character composition.

private String formatFieldName(String fieldName) {
  if (fieldName.matches("[A-Za-z_@][A-Za-z0-9_@-]*(\\.[A-Za-z_@][A-Za-z0-9_@-]*)*")) {
    return fieldName;
  }
  return "`" + fieldName.replace("`", "``") + "`";
}

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent potential NoSuchElementException

The method uses getFirst() which throws NoSuchElementException if the list is empty.
Add a guard to verify the list is non-empty before accessing elements, or handle the
empty case explicitly.

core/src/main/java/org/opensearch/sql/calcite/RuntimeSearchCorrelator.java [133-146]

 private static RelNode combineSubqueries(List<RexSubQuery> subqueries, RexBuilder rexBuilder) {
-  RelNode result = subqueries.getFirst().rel;
+  if (subqueries.isEmpty()) {
+    throw new IllegalArgumentException("Cannot combine empty subquery list");
+  }
+  RelNode result = subqueries.get(0).rel;
   for (int i = 1; i < subqueries.size(); i++) {
     result =
         LogicalJoin.create(
             result,
             subqueries.get(i).rel,
             List.of(),
             rexBuilder.makeLiteral(true),
             Set.of(),
             JoinRelType.INNER);
   }
   return result;
 }
Suggestion importance[1-10]: 7

__

Why: The method uses getFirst() which can throw NoSuchElementException if the list is empty. However, the caller correlate() already validates that subqueries is non-empty before calling this method (line 58), so this is a defensive improvement rather than a critical bug fix.

Medium
Add null safety for query parts

The method doesn't validate that queryParts is non-null or that its length matches
expectations. If runtimeQueryParts is null when passed to scan(), this will cause a
NullPointerException. Add null checks before processing.

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteEnumerableIndexScan.java [163-173]

 private String buildRuntimeQuery(String[] queryParts) {
+  if (queryParts == null || queryParts.length == 0) {
+    throw new IllegalArgumentException("Query parts cannot be null or empty");
+  }
   StringBuilder query = new StringBuilder();
   for (int i = 0; i < queryParts.length; i++) {
     String part = queryParts[i];
     if (pushDownContext.getDynamicQueryString().runtimePredicateParts().contains(i)) {
       part = pushDownContext.getDynamicQueryString().compiler().compile(part);
     }
     query.append(part);
   }
   return query.toString();
 }
Suggestion importance[1-10]: 6

__

Why: The method buildRuntimeQuery() is only called from scan() at line 148, where runtimeQueryParts is already checked for null before calling this method. However, adding explicit validation improves defensive programming and makes the method more robust if called from other contexts in the future.

Low
General
Validate implicit format field requirements

When no fields are present, the method returns early without checking if
node.isImplicit() is true. For implicit format subsearches, this could bypass
necessary correlation setup. Verify that early return is safe for implicit format
scenarios.

core/src/main/java/org/opensearch/sql/calcite/FormatPlanner.java [53-56]

 if (fields.isEmpty()) {
+  if (node.isImplicit()) {
+    throw new IllegalStateException("Implicit format subsearch requires at least one field");
+  }
   builder.values(new String[] {SEARCH_FIELD}, node.getEmptyString());
   return builder.peek();
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion raises a valid concern about early return for implicit format, but the current implementation appears intentional. The emptyString fallback is a valid result even for implicit subsearches. The suggestion may be overly restrictive without clear evidence of a bug.

Low

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

calcite calcite migration releated feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant