Skip to content

Xyseries command implementation - #5343

Merged
noCharger merged 13 commits into
opensearch-project:mainfrom
asifabashar:xyseries2
Jul 21, 2026
Merged

Xyseries command implementation#5343
noCharger merged 13 commits into
opensearch-project:mainfrom
asifabashar:xyseries2

Conversation

@asifabashar

@asifabashar asifabashar commented Apr 14, 2026

Copy link
Copy Markdown
Contributor

Description

Problem Statement
xyseries command is missing in PPL which is part of todo in roadmap

This RFC proposes adding a new PPL transforming command, xyseries, with syntax and behavior aligned with SPL .

xyseries converts row-oriented grouped results into a wide table where:

  • One field is the X axis (x-field) and stays as a row key.
  • One field (y-name-field) provides a part of column name from proivded paramter values to choose from and used as pivot values in conjuction with (y-data-field) field name , where data for this column are pivoted cells that are agrregated value fields.

OpenSearch PPL already supports stats, chart, and timechart, but there is no direct equivalent for SPL xyseries.

Adding xyseries improves:

  • Query readability for pivot-style result shaping.
  • Dashboard interoperability for charts requiring wide-table series layout.

Current State
There is no xyseries compatiable command.

Long-Term Goals
Provide xyseries functionality.

Proposal
Summarizes the suggested solution or improvement.

Approach

User Syntax

xyseries  [sep=<string>] [format=<string>] <x-field> <y-name-field> in (<value1>, <value2>, ...) <y-data-field>[,<y-data-field2>

Arguments

  • x-field (required): row key in output.
  • y-name-field (required): Number of values used to generate output series column names based on provided in paramter values such as <value1>, <value2> etc
  • y-data-field... (required, at least one): value field(s) used to fill cells.

Options

Option Default Behavior
sep ":" Separator between y-data-field name and 'in' parameter values , etc.
format None Naming template for output series columns represented as $AGG$<sep>$VAL$ . For each data row pivot value for y-name-field and $AGG$ (y-data-field name). The format parameter inserts the (y-data-field name) uses “:” as the built-in separator and then pivot value. $VAL$

If format is omitted:

  • Single y-data-field: output column is based row number .
  • Multiple y-data-field: output column is $AGG$<sep>$VAL$ where $VAL$ represent provided pivot values in 'in' operator.

Examples

Actual data:
ppl
 source=weblogs| stats count(host) as host_cnt, count(method) as method_cnt by url, response
host_cnt method_cnt response url
3 3 200 /page1
1 1 404 /page1
5 5 200 /page2
2 2 500 /page2

After xyseries transformation:
ppl

source=weblogs
| stats count(host) as host_cnt, count(method) as method_cnt by url, response
| xyseries url response in ("200","404","500") host_cnt, method_cnt

url host_cnt: 200 host_cnt: 404 host_cnt: 500 method_cnt: 200 method_cnt: 404 method_cnt: 500
/page1 3 1 null 3 1 null
/page2 5 null 2 5 null 2

If format is not specified, by default, you’d see something like:
url:count(host) and url:count(method) as column names.

If you provide your own separator through the format option, it overrides anything defined with sep. If for example, sep is set to “-”, but format specifies “+”, and because format has higher priority, the “+” is used.
Here, $VAL$ and $AGG$ are placeholders represent and the respectively. In the output, you can see that the name field (url) and the data field count(host) may appear in the position of $VAL$ and $AGG$, depending on how the format string arranges them.

Semantics

Input shape and type rules

Support xyseries only when pivot values are explicitly provided.

  • x-field and y-name-field must exist in input schema..
  • y-name-field values are converted to string for output column naming.
  • y-data-field One or more fields that contain the data to chart. If there are multiple fields specified, separate the field names with commas.

Null handling

  • Rows with null in a given y-data-field do not contribute a value for that generated series column.

Implementation Plan

Validate required fields/options and defaults.
Project to x, y_name selected from provided pivot values in 'in' operator, and selected y_data fields.
Build generated series column name using format or default naming with sep .
Pivot to wide schema based on provided pivot values passed as parameter.

Today our direction is to keep PPL commands translatable to SQL/Calcite plans as much as possible. We also plan to run PPL on Spark; SQL-native xyseries (with explicit in (...) values) is portable to Spark SQL, while post-processing would require separate Spark-side custom implementation.
Composability: if xyseries is implemented after query execution, it effectively must be terminal and cannot be reliably chained with downstream commands
. (Reference: Comments from penghuo )

out of scope

grouping option which does not apply for multifile input in OpenSearch.
Alternative
chart , timeseries can be used for similar use cases but not exactly same.

Limitations

As the rows will be transposed to columns, a limit is required field as calcite planning phase number of rows are not known.

Related Issues

Resolves [ #5142 ]

Check List

  • [ x] New functionality includes testing.
  • [x ] New functionality has been documented.
  • [x ] New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • [x ] New PPL command checklist all confirmed.
  • API changes companion pull request created.
  • [ x] 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.

@github-actions

github-actions Bot commented Apr 14, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit cf5afad)

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

When format is null and singleDataField is true, the generated column name is just the pivot value. If multiple pivot values are identical (e.g., duplicates in the in clause), this produces duplicate column names, causing the duplicate name check at line 4159 to fail. The code should either deduplicate pivot values earlier or handle this scenario explicitly.

if (format != null) {
  return format.replace("$AGG$", yDataFieldName).replace("$VAL$", pivotValue);
}
if (singleDataField) {
  return pivotValue;
}
return yDataFieldName + separator + pivotValue;
Possible Issue

If yNameRef is a non-atomic type (e.g., array, struct), the code throws an exception at line 4102. However, the exception is only thrown after checking !SqlTypeUtil.isCharacter(yNameRef.getType()) at line 4099. If yNameType is a character type but also non-atomic (unlikely but theoretically possible), the non-atomic check is bypassed, potentially causing a cast failure later.

if (!SqlTypeUtil.isCharacter(yNameRef.getType())) {
  if (!SqlTypeUtil.isAtomic(yNameType)) {
    throw new IllegalArgumentException(
        "xyseries y-name-field must be a scalar type, got: " + yNameType.getSqlTypeName());
  }
  RelDataType varchar =
      rx.getTypeFactory()
          .createTypeWithNullability(
              rx.getTypeFactory().createSqlType(SqlTypeName.VARCHAR), true);
  axis = rx.makeCast(varchar, yNameRef, true);
} else {
  axis = yNameRef;
}

@github-actions

github-actions Bot commented Apr 14, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to cf5afad

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Validate non-empty pivot values

When pivotValues is empty, the pivot operation will produce no output columns except
the x-field. This may lead to unexpected results. Consider validating that
pivotValues is not empty and throwing an IllegalArgumentException with a clear
message if it is.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [4091]

 List<String> pivotValues = node.getPivotValues() != null ? node.getPivotValues() : List.of();
+if (pivotValues.isEmpty()) {
+  throw new IllegalArgumentException("xyseries requires at least one pivot value in the IN clause");
+}
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that empty pivotValues could lead to unexpected results. However, this is a validation check that asks the user to ensure proper input, which typically scores in the moderate range (not above 7).

Medium
General
Validate column name uniqueness earlier

The duplicate name check occurs after all projections are built. If a duplicate is
detected, the error is thrown but the RelBuilder state may be inconsistent. Move
this validation earlier, before building projections, to fail fast and avoid partial
state modifications.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [4158-4167]

+// Validate uniqueness before building projections
 Set<String> seenNames = new HashSet<>();
-for (String name : reorderNames) {
-  if (!seenNames.add(name)) {
-    throw new IllegalArgumentException(
-        "xyseries produced duplicate output column name '"
-            + name
-            + "'. Use a format template containing both $AGG$ and $VAL$ so column names"
-            + " are unique.");
+for (String aggName : yDataFieldNames) {
+  for (String pivotVal : pivotValues) {
+    boolean singleDataField = yDataFieldNames.size() == 1;
+    String name = generateColumnName(aggName, pivotVal, separator, format, singleDataField);
+    if (!seenNames.add(name)) {
+      throw new IllegalArgumentException(
+          "xyseries produced duplicate output column name '"
+              + name
+              + "'. Use a format template containing both $AGG$ and $VAL$ so column names"
+              + " are unique.");
+    }
   }
 }
 
+// Build projections after validation
+for (String aggName : yDataFieldNames) {
+  for (String pivotVal : pivotValues) {
+    String pivotColName = pivotVal + "_" + aggName;
+    ...
+
Suggestion importance[1-10]: 6

__

Why: Moving the validation earlier is a good practice for fail-fast behavior and avoiding inconsistent state. However, the impact is moderate since the current implementation already catches duplicates before returning the result.

Low
Validate supported types before casting

The cast to VARCHAR may fail at runtime for certain non-atomic types or produce
unexpected string representations. Consider adding explicit type checks for
supported scalar types (e.g., numeric, boolean, date) before casting to ensure
predictable behavior.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [4099-4110]

 if (!SqlTypeUtil.isCharacter(yNameRef.getType())) {
   if (!SqlTypeUtil.isAtomic(yNameType)) {
     throw new IllegalArgumentException(
         "xyseries y-name-field must be a scalar type, got: " + yNameType.getSqlTypeName());
+  }
+  if (!SqlTypeUtil.isNumeric(yNameType) 
+      && !SqlTypeUtil.isBoolean(yNameType) 
+      && !SqlTypeUtil.isDatetime(yNameType)) {
+    throw new IllegalArgumentException(
+        "xyseries y-name-field type not supported for casting: " + yNameType.getSqlTypeName());
   }
   RelDataType varchar =
       rx.getTypeFactory()
           .createTypeWithNullability(
               rx.getTypeFactory().createSqlType(SqlTypeName.VARCHAR), true);
   axis = rx.makeCast(varchar, yNameRef, true);
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion adds more specific type checking before casting to VARCHAR. While this could improve error messages and prevent unexpected behavior, the existing isAtomic check already provides basic validation. The added checks are defensive but not critical.

Low

Previous suggestions

Suggestions up to commit ea1ea99
CategorySuggestion                                                                                                                                    Impact
Possible issue
Check x-field name collision with columns

The duplicate name check occurs after all column names are generated. If a duplicate
exists, the error message suggests using a format template, but doesn't account for
the x-field name potentially colliding with generated column names. Add a check to
ensure the x-field name doesn't conflict with any generated pivot column names.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [4158-4167]

 Set<String> seenNames = new HashSet<>();
+seenNames.add(xFieldName); // Reserve x-field name
 for (String name : reorderNames) {
   if (!seenNames.add(name)) {
+    String hint = name.equals(xFieldName) 
+        ? "The x-field name conflicts with a generated column name."
+        : "Use a format template containing both $AGG$ and $VAL$ so column names are unique.";
     throw new IllegalArgumentException(
-        "xyseries produced duplicate output column name '"
-            + name
-            + "'. Use a format template containing both $AGG$ and $VAL$ so column names"
-            + " are unique.");
+        "xyseries produced duplicate output column name '" + name + "'. " + hint);
   }
 }
Suggestion importance[1-10]: 8

__

Why: This identifies a potential bug where the xFieldName could collide with generated pivot column names, which would cause incorrect behavior. The suggestion correctly adds the x-field name to the set before checking for duplicates and provides a more specific error message.

Medium
Validate non-empty pivot values

When pivotValues is empty, the pivot operation will produce no output columns beyond
the x-field. This likely indicates a user error or misconfiguration. Consider
validating that pivotValues is non-empty and throwing an IllegalArgumentException
with a clear message if it's empty to fail fast.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [4091]

 List<String> pivotValues = node.getPivotValues() != null ? node.getPivotValues() : List.of();
+if (pivotValues.isEmpty()) {
+  throw new IllegalArgumentException(
+      "xyseries requires at least one pivot value in the IN clause");
+}
Suggestion importance[1-10]: 7

__

Why: Adding validation for empty pivotValues would provide better error handling and fail fast with a clear message. However, this is a defensive check rather than fixing a critical bug, as the current code would still execute (just producing no pivot columns).

Medium
General
Add available columns to error message

The pivot column name construction assumes a specific naming pattern
({value}_{agg}), but this may not match Calcite's actual pivot output naming
convention. If Calcite uses a different pattern, the field lookup will fail. Verify
the actual column naming used by b.pivot() to ensure correct field references.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [4143-4151]

 for (String pivotVal : pivotValues) {
-  // Reference pivot output column by its generated name: {value}_{agg}
+  // Calcite pivot uses pattern: {value}_{agg}
+  // Verify this matches actual pivot output before field lookup
   String pivotColName = pivotVal + "_" + aggName;
   try {
     reorderProjections.add(b.field(pivotColName));
   } catch (IllegalArgumentException e) {
     throw new IllegalStateException(
-        "xyseries: expected pivot output column '" + pivotColName + "' not found", e);
+        "xyseries: expected pivot output column '" + pivotColName + "' not found. "
+        + "Available columns: " + b.peek().getRowType().getFieldNames(), e);
   }
   ...
 }
Suggestion importance[1-10]: 6

__

Why: Enhancing the error message with available column names would improve debuggability when the pivot column lookup fails. This is a useful improvement for troubleshooting but doesn't address a functional issue.

Low
Suggestions up to commit 8818917
CategorySuggestion                                                                                                                                    Impact
Possible issue
Validate non-empty pivot values

When pivotValues is empty, the pivot operation will produce no output columns beyond
the x-field. This likely indicates a user error or misconfiguration. Consider
validating that pivotValues is non-empty and throwing an IllegalArgumentException
with a clear message if it is empty.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [4091]

-List<String> pivotValues = node.getPivotValues() != null ? node.getPivotValues() : List.of();
+List<String> pivotValues = node.getPivotValues();
+if (pivotValues == null || pivotValues.isEmpty()) {
+  throw new IllegalArgumentException(
+      "xyseries requires at least one pivot value in the IN clause");
+}
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that empty pivotValues would produce no output columns beyond the x-field, which is likely a user error. However, the improved code changes the logic by removing the null-coalescing to empty list, which may break existing behavior if null is intentionally handled elsewhere.

Medium
Validate non-empty y-data fields

If yDataFields is empty, the xyseries operation will produce no data columns, only
the x-field. This is likely a user error. Validate that at least one y-data field is
provided and throw an IllegalArgumentException if the list is empty.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [4088-4089]

 List<String> yDataFieldNames =
     node.getYDataFields().stream().map(this::resolveFieldName).collect(Collectors.toList());
+if (yDataFieldNames.isEmpty()) {
+  throw new IllegalArgumentException(
+      "xyseries requires at least one y-data field");
+}
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that empty yDataFields would produce no data columns, which is likely a user error. Adding validation here would provide better error messages to users and prevent downstream issues.

Medium
General
Validate column names before building projections

The duplicate column name check occurs after all projections are built. If a
duplicate is detected, the error is thrown but the RelBuilder state may already be
modified. Consider performing this validation earlier, before building the
projections, to avoid partial state changes.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [4158-4168]

 Set<String> seenNames = new HashSet<>();
-for (String name : reorderNames) {
-  if (!seenNames.add(name)) {
-    throw new IllegalArgumentException(
-        "xyseries produced duplicate output column name '"
-            + name
-            + "'. Use a format template containing both $AGG$ and $VAL$ so column names"
-            + " are unique.");
+for (String aggName : yDataFieldNames) {
+  for (String pivotVal : pivotValues) {
+    boolean singleDataField = yDataFieldNames.size() == 1;
+    String name = generateColumnName(aggName, pivotVal, separator, format, singleDataField);
+    if (!seenNames.add(name)) {
+      throw new IllegalArgumentException(
+          "xyseries produced duplicate output column name '"
+              + name
+              + "'. Use a format template containing both $AGG$ and $VAL$ so column names"
+              + " are unique.");
+    }
   }
 }
 
+List<RexNode> reorderProjections = new ArrayList<>();
+List<String> reorderNames = new ArrayList<>();
+reorderProjections.add(b.field(xFieldName));
+reorderNames.add(xFieldName);
+
+for (String aggName : yDataFieldNames) {
+  for (String pivotVal : pivotValues) {
+    String pivotColName = pivotVal + "_" + aggName;
+    try {
+      reorderProjections.add(b.field(pivotColName));
+    } catch (IllegalArgumentException e) {
+      throw new IllegalStateException(
+          "xyseries: expected pivot output column '" + pivotColName + "' not found", e);
+    }
+    boolean singleDataField = yDataFieldNames.size() == 1;
+    reorderNames.add(generateColumnName(aggName, pivotVal, separator, format, singleDataField));
+  }
+}
+
Suggestion importance[1-10]: 6

__

Why: Moving the duplicate name validation earlier is a good practice to avoid partial state changes in RelBuilder. However, this requires duplicating the column name generation logic, which reduces maintainability. The current approach is acceptable since the validation happens before the final project call.

Low
Suggestions up to commit 9d6225c
CategorySuggestion                                                                                                                                    Impact
Possible issue
Initialize type variable before use

The variable yNameType is used before being properly initialized from
yNameRef.getType(). The code retrieves the type twice: once in the outer condition
and again to assign yNameType. This creates a potential inconsistency if the type
changes between calls. Store the type once at the beginning.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [4097-4111]

-if (!SqlTypeUtil.isCharacter(yNameRef.getType())) {
+RelDataType yNameType = yNameRef.getType();
+RexNode axis;
+if (!SqlTypeUtil.isCharacter(yNameType)) {
   if (!SqlTypeUtil.isAtomic(yNameType)) {
     throw new IllegalArgumentException(
         "xyseries y-name-field must be a scalar type, got: " + yNameType.getSqlTypeName());
   }
   RelDataType varchar =
       rx.getTypeFactory()
           .createTypeWithNullability(
               rx.getTypeFactory().createSqlType(SqlTypeName.VARCHAR), true);
   axis = rx.makeCast(varchar, yNameRef, true);
 } else {
   axis = yNameRef;
 }
Suggestion importance[1-10]: 7

__

Why: The code retrieves yNameRef.getType() twice - once in the condition check and once to assign yNameType. While this works, it's inefficient and could theoretically lead to inconsistency. The suggestion to store the type once at the beginning is a good practice for code clarity and efficiency.

Medium
Suggestions up to commit ae0ee9f
CategorySuggestion                                                                                                                                    Impact
General
Validate duplicate names during generation

The duplicate name check occurs after all column names are generated and added to
reorderNames. If a duplicate exists, the error is thrown after unnecessary work.
Move this validation earlier or integrate it during name generation to fail fast and
avoid building invalid projection lists.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [4142-4168]

 Set<String> seenNames = new HashSet<>();
-for (String name : reorderNames) {
-  if (!seenNames.add(name)) {
-    throw new IllegalArgumentException(
-        "xyseries produced duplicate output column name '"
-            + name
-            + "'. Use a format template containing both $AGG$ and $VAL$ so column names"
-            + " are unique.");
+for (String aggName : yDataFieldNames) {
+  for (String pivotVal : pivotValues) {
+    String pivotColName = pivotVal + "_" + aggName;
+    try {
+      reorderProjections.add(b.field(pivotColName));
+    } catch (IllegalArgumentException e) {
+      throw new IllegalStateException(
+          "xyseries: expected pivot output column '" + pivotColName + "' not found", e);
+    }
+    boolean singleDataField = yDataFieldNames.size() == 1;
+    String colName = generateColumnName(aggName, pivotVal, separator, format, singleDataField);
+    if (!seenNames.add(colName)) {
+      throw new IllegalArgumentException(
+          "xyseries produced duplicate output column name '"
+              + colName
+              + "'. Use a format template containing both $AGG$ and $VAL$ so column names"
+              + " are unique.");
+    }
+    reorderNames.add(colName);
   }
 }
Suggestion importance[1-10]: 6

__

Why: The suggestion proposes moving duplicate name validation into the column generation loop to fail faster. This is a reasonable optimization that avoids building invalid projection lists before detecting duplicates. However, the impact is moderate since the current approach still catches duplicates before the query executes, and the performance difference is negligible for typical use cases with small numbers of columns.

Low
Use consistent variable reference

The type check uses yNameRef.getType() but the error message references yNameType.
While both currently point to the same value, this inconsistency could lead to
confusion. Use yNameType consistently in the condition to match the error message
and improve code clarity.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [4099-4111]

-if (!SqlTypeUtil.isCharacter(yNameRef.getType())) {
+if (!SqlTypeUtil.isCharacter(yNameType)) {
   if (!SqlTypeUtil.isAtomic(yNameType)) {
     throw new IllegalArgumentException(
         "xyseries y-name-field must be a scalar type, got: " + yNameType.getSqlTypeName());
   }
   RelDataType varchar =
       rx.getTypeFactory()
           .createTypeWithNullability(
               rx.getTypeFactory().createSqlType(SqlTypeName.VARCHAR), true);
   axis = rx.makeCast(varchar, yNameRef, true);
 } else {
   axis = yNameRef;
 }
Suggestion importance[1-10]: 3

__

Why: The suggestion correctly identifies a minor inconsistency where yNameRef.getType() is used in the condition but yNameType is referenced in the error message. However, since both variables hold the same value (as yNameType is assigned from yNameRef.getType() on line 4097), this is purely a stylistic improvement with minimal impact on functionality or maintainability.

Low
Suggestions up to commit 48ec5c1
CategorySuggestion                                                                                                                                    Impact
Possible issue
Validate non-empty pivot values

Validate that pivotValues is not empty before proceeding. An empty pivot list will
cause the pivot operation to produce no data columns, resulting in an output with
only the x-field column and no meaningful results.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [4091]

 List<String> pivotValues = node.getPivotValues() != null ? node.getPivotValues() : List.of();
+if (pivotValues.isEmpty()) {
+  throw new IllegalArgumentException(
+      "xyseries: at least one pivot value must be specified in the IN clause");
+}
Suggestion importance[1-10]: 8

__

Why: This is a critical validation. An empty pivotValues list would result in a pivot operation that produces no data columns, making the output meaningless. The suggestion correctly identifies this edge case and provides appropriate error handling.

Medium
Detect duplicate pivot values

Check for duplicate pivot values before adding to pivotValueMap. Duplicate values in
the in clause will cause the last occurrence to silently overwrite earlier ones,
potentially producing unexpected results without warning the user.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [4121-4124]

+Set<String> seenPivotValues = new HashSet<>();
 for (String val : pivotValues) {
+  if (!seenPivotValues.add(val)) {
+    throw new IllegalArgumentException(
+        "xyseries: duplicate pivot value '" + val + "' in IN clause");
+  }
   pivotValueMap.put(val, ImmutableList.of(b.literal(val)));
 }
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that duplicate pivot values in the in clause will cause silent overwrites in the LinkedHashMap, potentially leading to unexpected results. Adding validation improves robustness and user experience by providing clear error messages.

Medium
Validate y-data field names

Validate that yDataFieldNames is not empty and contains no duplicates. Empty or
duplicate data field names will cause incorrect aggregation or column name
collisions in the pivot output.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [4088-4089]

 List<String> yDataFieldNames =
     node.getYDataFields().stream().map(this::resolveFieldName).collect(Collectors.toList());
+if (yDataFieldNames.isEmpty()) {
+  throw new IllegalArgumentException("xyseries: at least one y-data field must be specified");
+}
+Set<String> uniqueDataFields = new HashSet<>(yDataFieldNames);
+if (uniqueDataFields.size() != yDataFieldNames.size()) {
+  throw new IllegalArgumentException("xyseries: duplicate y-data field names are not allowed");
+}
Suggestion importance[1-10]: 7

__

Why: The suggestion addresses two important edge cases: empty yDataFieldNames and duplicate field names. Both scenarios would lead to incorrect behavior - empty list would produce no aggregations, and duplicates could cause column name collisions that are already checked later but would be better caught earlier.

Medium

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 59ad4dc

@github-actions

github-actions Bot commented Apr 14, 2026

Copy link
Copy Markdown
Contributor

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 70744ff.

'Diff too large, requires skip by maintainers after manual review'


Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.


⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit c7b7536

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit a72ffe1

1 similar comment
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit a72ffe1

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 13ddcb5

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 1858a98

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit bfd736e

Signed-off-by: Asif Bashar <abashar@apple.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 033181c

Signed-off-by: Asif Bashar <abashar@apple.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit c7e3b31

Signed-off-by: Asif Bashar <abashar@apple.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit f9cc584

Signed-off-by: Asif Bashar <abashar@apple.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 48ec5c1

@noCharger

Copy link
Copy Markdown
Collaborator

@asifabashar Can we fix the remaining CI by updating the logical and physical plan?

…lict.

Signed-off-by: Asif Bashar <abashar@apple.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit ae0ee9f

Comment thread docs/user/ppl/index.md
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 9d6225c

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 8818917

Signed-off-by: Asif Bashar <abashar@apple.com>
Signed-off-by: Asif Bashar <abashar@apple.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit ea1ea99

Signed-off-by: Asif Bashar <abashar@apple.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit cf5afad

@noCharger

Copy link
Copy Markdown
Collaborator

Performance note: coordinator-side pivot aggregate (non-blocking)

Recording a performance analysis for future reference. This is not a merge blocker; the design is sound for the intended use.

Plan shape. The upstream stats is pushed into OpenSearch as a composite aggregation and paged (size 1000). The pivot itself (MAX(dataField) FILTER (y = value)) runs as a Calcite EnumerableAggregate on the coordinator node. It is a hash aggregation keyed by the x-field, so all distinct x-field groups are resident in memory at once, each holding M x P accumulator cells (M = number of y-data-fields, P = number of pivot values). QUERY_SIZE_LIMIT (default 10000) sits above the sort and trims only the final output rows, so it does not bound this intermediate hash.

Two bounded axes.

  • Width = M x P columns. Self-bounded because every pivot value and data field is an explicit literal the user types in the query. It cannot grow from data. The only ceiling is the query string size and a JVM 64 KB per-method codegen limit on the generated aggregate or calc (same class of limit makeresults hit around 5900 inline units; the exact xyseries threshold is untested but is likely in the low thousands of M x P). A very large hand-generated in (...) could hit this and surface an opaque Janino codegen error.
  • Height = number of distinct x-field values (Gx). Data-driven and not capped at the intermediate stage.

Worst case. Peak coordinator memory is approximately Gx * (M x P) * bytes_per_cell. For example Gx = 10,000,000, M = 5, P = 20 gives 1e9 cells, roughly 16 GB on a single node.

Backstop that already exists. The node-level real-memory circuit breaker samples actual heap and trips before node OOM, aborting the query with a CircuitBreakingException rather than crashing the node. The SQL plugin memory monitor (plugins.query.memory_limit) is a second guard. So the realistic worst case is a failed query on high-cardinality x, not a node crash.

Suggested follow-ups (optional, not blocking):

  1. Docs: state that xyseries targets low-cardinality x (chart-shaping), consistent with SPL usage; high-cardinality x can trip the memory breaker.
  2. Optional plan-time guard on generated column count (M x P) that fails fast with a clear message, converting a potential Janino 64 KB codegen crash into a friendly error.
  3. Engine-level improvement (separate work): a sorted-input streaming aggregate with limit push-down would cap peak memory at 10000 * (M x P) instead of Gx * (M x P). This needs input ordered by x and aggregate/limit push-down, so it is out of scope here.

@noCharger noCharger left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change!

@noCharger
noCharger merged commit 6d3c58d into opensearch-project:main Jul 21, 2026
40 checks passed
@noCharger noCharger added the v3.8.0 Issues and PRs related to version v3.8.0 label Jul 22, 2026
// Xyseries tests

@Test
public void testXyseriesCommandBasic() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add test to verify xyseries command can translate to SQL

Comment thread docs/user/ppl/index.md
| [mvexpand command](cmd/mvexpand.md) | 3.6 | stable (since 3.6) | Expand a multi-valued field into separate documents (one per value). |
| [graphlookup command](cmd/graphlookup.md) | 3.6 | experimental (since 3.6) | Performs recursive graph traversal on a collection using a BFS algorithm.|

| [xyseries command](cmd/xyseries.md) | 3.8 | stable (since 3.8) | Converts row-oriented grouped results into a wide table format suitable for chart visualizations. One field serves as the X axis (row key), one field provides pivot values for generating output column names, and one or more data fields fill the pivoted cells. Only rows matching the explicitly provided pivot values in the `in` clause are included. |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stable -> experimental

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

Labels

feature PPL Piped processing language v3.8.0 Issues and PRs related to version v3.8.0

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants