[Feature] `showperc` with `top` and `rare` commands. by AjimelecGonzalez · Pull Request #5642 · opensearch-project/sql

AjimelecGonzalez · 2026-07-21T21:44:06Z

Description

Add showperc option to the top and rare PPL commands. When showperc=true is specified, a percent column is added to the output showing each value's percentage of the total count within its partition (group-by clause).

The percentage is computed as ROUND(100.0 * count / SUM(count) OVER (PARTITION BY group_fields), 2) and is rounded to 2 decimal places. When used with a by clause, percentages are calculated within each partition (summing to 100% per group).

Syntax

source=index | top showperc=true field_name [by group_field]
source=index | rare showperc=true field_name [by group_field]

`Top` Example

source=otellogs | top showperc=true severityText

Returns:

severityText	count	percent
ERROR	7	35.0
INFO	6	30.0
WARN	4	20.0
DEBUG	3	15.0

`Rare` Example

source=otellogs | rare showperc=true severityText

Returns:

severityText	count	percent
DEBUG	3	15.0
WARN	4	20.0
INFO	6	30.0
ERROR	7	35.0

Related Issues

Resolves #5530

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.

github-actions · 2026-07-21T21:45:31Z

PR Reviewer Guide 🔍

(Review updated until commit `b576e5a`)

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 Typo fix The old code had a typo 'countield' instead of 'countfield'. The new code fixes this typo but also adds showperc. While the typo fix is good, it was not mentioned in the PR description. This is a minor issue but worth noting for completeness. "countfield='%s' showcount=%s showperc=%s usenull=%s ", countField, showCount, showPerc, useNull) Division by zero When computing the percentage, if the SUM(count) window function returns zero (e.g., if all rows are filtered out before aggregation), the division at line 3256 will produce infinity or NaN. This could occur if the dataset is empty or if filters eliminate all rows before the rare/top aggregation. Consider adding a check or handling for zero total. RexNode percValue = context.relBuilder.call(SqlStdOperatorTable.DIVIDE, numerator, totalCast);

github-actions · 2026-07-21T21:46:05Z

PR Code Suggestions ✨

Latest suggestions up to b576e5a

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle division by zero safely Add null safety check for division operation. When `totalWindowOver` evaluates to zero or null, the division will fail or produce incorrect results. Consider using a CASE expression to handle zero/null denominators gracefully. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [3251-3256] RexNode hundred = context.relBuilder.literal(new BigDecimal("100.0")); RexNode countCast = context.relBuilder.cast(context.relBuilder.field(countFieldName), SqlTypeName.DOUBLE); RexNode totalCast = context.relBuilder.cast(totalWindowOver, SqlTypeName.DOUBLE); RexNode numerator = context.relBuilder.call(SqlStdOperatorTable.MULTIPLY, hundred, countCast); -RexNode percValue = context.relBuilder.call(SqlStdOperatorTable.DIVIDE, numerator, totalCast); +RexNode zero = context.relBuilder.literal(0.0); +RexNode isZero = context.relBuilder.equals(totalCast, zero); +RexNode percValue = context.relBuilder.call( + SqlStdOperatorTable.CASE, + isZero, + zero, + context.relBuilder.call(SqlStdOperatorTable.DIVIDE, numerator, totalCast)); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential division by zero issue when `totalWindowOver` is zero. However, in the context of `SUM(COUNT(*))` over a window, this scenario is unlikely in practice since it would require no rows in the partition. The suggested CASE expression adds defensive programming but may be overly cautious for this specific use case.	Medium
General	Improve field conflict error message The field name conflict check occurs after `showPerc` is evaluated but before the percentage calculation. If `showPerc` is false, this check is skipped, but a user-defined `percent` field could still conflict with the internal `row_number_rare_top` field or future features. Consider checking for reserved field names earlier or documenting this limitation. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [3234-3240] Boolean showPerc = (Boolean) argumentMap.get(RareTopN.Option.showPerc.name()).getValue(); if (showPerc) { - if (context.relBuilder.peek().getRowType().getFieldNames().contains("percent")) { + List<String> fieldNames = context.relBuilder.peek().getRowType().getFieldNames(); + if (fieldNames.contains("percent")) { throw new IllegalArgumentException( "Field `percent` already exists in the output. Consider renaming the conflicting" - + " field."); + + " field or using showperc=false."); } Suggestion importance[1-10]: 3 __ Why: The suggestion only adds "or using showperc=false" to the error message, which provides minimal improvement. The existing error message already clearly indicates the conflict and suggests renaming. The concern about checking reserved field names earlier is valid but not addressed by the `improved_code`, making this a minor enhancement.	Low

Previous suggestions

Suggestions up to commit 0039627

Category	Suggestion	Impact
Possible issue	Fix typo in field name check The field name check contains a typo: "precent" instead of "percent". This will fail to detect actual conflicts with the `percent` field being created, allowing duplicate field names in the output. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [3236-3240] -if (context.relBuilder.peek().getRowType().getFieldNames().contains("precent")) { +if (context.relBuilder.peek().getRowType().getFieldNames().contains("percent")) { throw new IllegalArgumentException( "Field `percent` already exists in the output. Consider renaming the conflicting" + " field."); } Suggestion importance[1-10]: 10 __ Why: This is a critical bug. The code checks for "precent" instead of "percent", which means it will fail to detect actual field name conflicts. This could lead to duplicate field names in the output, causing runtime errors or incorrect results.	High

Suggestions up to commit 0b5b6ed

Category Suggestion Impact

Possible issue

Exclude count field when needed

When showPerc=true but showCount=false, the countFieldName column is still needed
for percentage calculation but should be excluded from final output. The current
logic only handles showCount and doesn't account for this scenario.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [3280-3285]

 // 5. project final output: group by list + field list, optionally count and percent
 Boolean showCount = (Boolean) argumentMap.get(RareTopN.Option.showCount.name()).getValue();
-if (showCount) {
-  context.relBuilder.projectExcept(context.relBuilder.field(ROW_NUMBER_COLUMN_FOR_RARE_TOP));
+if (showCount || showPerc) {
+  if (showCount) {
+    context.relBuilder.projectExcept(context.relBuilder.field(ROW_NUMBER_COLUMN_FOR_RARE_TOP));
+  } else {
+    // showPerc=true but showCount=false: exclude both row_number and count columns
+    context.relBuilder.projectExcept(
+        context.relBuilder.field(ROW_NUMBER_COLUMN_FOR_RARE_TOP),
+        context.relBuilder.field(countFieldName));
+  }
 } else {

Suggestion importance[1-10]: 9

__

Why: This is a critical bug fix. When showPerc=true and showCount=false, the count field is needed for percentage calculation but should be excluded from the final output. The current code doesn't handle this case, which would result in the count field appearing in the output even when showCount=false. The test case testTopCommandShowPercWithoutShowCount confirms this scenario should be supported.

High

Handle division by zero error

The percentage calculation will fail with division by zero when the total count is
zero (empty result set). Add a null check or conditional logic to handle this edge
case before performing the division operation.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [3253-3278]

 Boolean showPerc = (Boolean) argumentMap.get(RareTopN.Option.showPerc.name()).getValue();
 if (showPerc) {
   RexNode totalWindowOver =
       PlanUtils.makeOver(
           context,
           BuiltinFunctionName.SUM,
           context.relBuilder.field(countFieldName),
           List.of(),
           partitionKeys,
           List.of(),
           WindowFrame.rowsUnbounded());
-  ...
+
+  RexNode hundred = context.relBuilder.literal(new BigDecimal("100.0"));
+  RexNode countCast =
+      context.relBuilder.cast(context.relBuilder.field(countFieldName), SqlTypeName.DOUBLE);
+  RexNode totalCast = context.relBuilder.cast(totalWindowOver, SqlTypeName.DOUBLE);
+  RexNode numerator = context.relBuilder.call(SqlStdOperatorTable.MULTIPLY, hundred, countCast);
+  
+  // Handle division by zero
+  RexNode zero = context.relBuilder.literal(0.0);
+  RexNode isZero = context.relBuilder.equals(totalCast, zero);
+  RexNode percValue = context.relBuilder.call(
+      SqlStdOperatorTable.CASE,
+      isZero,
+      zero,
+      context.relBuilder.call(SqlStdOperatorTable.DIVIDE, numerator, totalCast));
+
+  RexNode roundedPerc =
+      context.relBuilder.call(
+          SqlStdOperatorTable.ROUND, percValue, context.relBuilder.literal(2));
+
   context.relBuilder.projectPlus(context.relBuilder.alias(roundedPerc, "percent"));
 }

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential division by zero issue when calculating percentages. However, in the context of rare and top commands that aggregate data, an empty result set would not reach this code path, making this a less critical edge case. The suggestion is valid but may be overly defensive.

Medium

Suggestions up to commit dae1ecd

Category	Suggestion	Impact
Possible issue	Handle division by zero The percentage calculation doesn't handle the case where `totalWindowOver` could be zero, which would result in division by zero. Add a null check or conditional logic to prevent this error when the total count is zero. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [3265-3270] RexNode hundred = context.relBuilder.literal(new BigDecimal("100.0")); RexNode countCast = context.relBuilder.cast(context.relBuilder.field(countFieldName), SqlTypeName.DOUBLE); RexNode totalCast = context.relBuilder.cast(totalWindowOver, SqlTypeName.DOUBLE); RexNode numerator = context.relBuilder.call(SqlStdOperatorTable.MULTIPLY, hundred, countCast); -RexNode percValue = context.relBuilder.call(SqlStdOperatorTable.DIVIDE, numerator, totalCast); +RexNode zero = context.relBuilder.literal(0.0); +RexNode isZero = context.relBuilder.equals(totalCast, zero); +RexNode percValue = context.relBuilder.call( + SqlStdOperatorTable.CASE, + isZero, + zero, + context.relBuilder.call(SqlStdOperatorTable.DIVIDE, numerator, totalCast)); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential division by zero issue when `totalWindowOver` is zero. However, in the context of a `SUM` window function over counts, this scenario is unlikely in practice since records exist to be counted. The suggestion provides a valid defensive programming approach but may be overly cautious for this specific use case.	Medium

Suggestions up to commit ca7e450

Category	Suggestion	Impact
General	Use range-based window frame The window frame for computing the total should use `WindowFrame.rangeUnbounded()` instead of `WindowFrame.rowsUnbounded()` to ensure consistent behavior with the expected SQL output. The generated Spark SQL uses `RANGE BETWEEN UNBOUNDED PRECEDING` `AND UNBOUNDED FOLLOWING`, which corresponds to a range-based frame. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [3255-3263] RexNode totalWindowOver = PlanUtils.makeOver( context, BuiltinFunctionName.SUM, context.relBuilder.field(countFieldName), List.of(), partitionKeys, List.of(), - WindowFrame.rowsUnbounded()); + WindowFrame.rangeUnbounded()); Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that `WindowFrame.rangeUnbounded()` should be used instead of `WindowFrame.rowsUnbounded()` to match the expected Spark SQL output (`RANGE BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING`). This is a significant correctness issue that affects the semantic behavior of the window function.	Medium
Possible issue	Handle division by zero Division by zero can occur when `totalWindowOver` is zero or null, causing runtime errors. Add a null check or use a CASE expression to handle scenarios where the total count is zero, returning null or zero for the percentage in such cases. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [3265-3270] RexNode hundred = context.relBuilder.literal(new BigDecimal("100.0")); RexNode countCast = context.relBuilder.cast(context.relBuilder.field(countFieldName), SqlTypeName.DOUBLE); RexNode totalCast = context.relBuilder.cast(totalWindowOver, SqlTypeName.DOUBLE); RexNode numerator = context.relBuilder.call(SqlStdOperatorTable.MULTIPLY, hundred, countCast); -RexNode percValue = context.relBuilder.call(SqlStdOperatorTable.DIVIDE, numerator, totalCast); +RexNode zero = context.relBuilder.literal(0.0); +RexNode isZero = context.relBuilder.equals(totalCast, zero); +RexNode percValue = context.relBuilder.call( + SqlStdOperatorTable.CASE, + isZero, + context.relBuilder.literal(null), + context.relBuilder.call(SqlStdOperatorTable.DIVIDE, numerator, totalCast)); Suggestion importance[1-10]: 7 __ Why: The suggestion addresses a potential runtime error when `totalWindowOver` is zero. While this is a valid concern for robustness, the likelihood depends on the data context. Adding null/zero handling would improve code safety, though it may not be critical if the data guarantees non-zero totals.	Medium

Suggestions up to commit 54ab043

Category	Suggestion	Impact
Possible issue	Use rangeUnbounded for window frame The window frame for computing the total should use `WindowFrame.rangeUnbounded()` instead of `WindowFrame.rowsUnbounded()` to ensure consistent behavior with the expected Spark SQL output, which uses `RANGE BETWEEN UNBOUNDED PRECEDING AND` `UNBOUNDED FOLLOWING`. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [3255-3263] RexNode totalWindowOver = PlanUtils.makeOver( context, BuiltinFunctionName.SUM, context.relBuilder.field(countFieldName), List.of(), partitionKeys, List.of(), - WindowFrame.rowsUnbounded()); + WindowFrame.rangeUnbounded()); Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a discrepancy between the implementation using `WindowFrame.rowsUnbounded()` and the expected Spark SQL output which uses `RANGE BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING`. This could lead to incorrect results in certain scenarios, making it a significant correctness issue.	Medium
General	Handle division by zero scenario The division operation could result in division by zero if all counts in a partition are zero or null. Add a safeguard using a CASE expression to handle this edge case and return null or zero when the total is zero. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [3265-3270] RexNode hundred = context.relBuilder.literal(new BigDecimal("100.0")); RexNode countCast = context.relBuilder.cast(context.relBuilder.field(countFieldName), SqlTypeName.DOUBLE); RexNode totalCast = context.relBuilder.cast(totalWindowOver, SqlTypeName.DOUBLE); RexNode numerator = context.relBuilder.call(SqlStdOperatorTable.MULTIPLY, hundred, countCast); -RexNode percValue = context.relBuilder.call(SqlStdOperatorTable.DIVIDE, numerator, totalCast); +RexNode zero = context.relBuilder.literal(0.0); +RexNode isZero = context.relBuilder.equals(totalCast, zero); +RexNode percValue = context.relBuilder.call( + SqlStdOperatorTable.CASE, + isZero, + context.relBuilder.literal(null), + context.relBuilder.call(SqlStdOperatorTable.DIVIDE, numerator, totalCast)); Suggestion importance[1-10]: 6 __ Why: The suggestion addresses a potential edge case where division by zero could occur if all counts are zero. However, this scenario is unlikely in practice since the code only processes rows that passed the filter (line 3247-3250), meaning there will always be at least one row with a non-zero count. Still, adding this safeguard would improve robustness.	Low
General	Add null check for showPerc Add a null check for `showPerc` before the conditional to prevent potential `NullPointerException` if the argument is missing from the map. This ensures robustness when the option is not explicitly provided. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [3253-3278] Boolean showPerc = (Boolean) argumentMap.get(RareTopN.Option.showPerc.name()).getValue(); -if (showPerc) { +if (showPerc != null && showPerc) { ... context.relBuilder.projectPlus(context.relBuilder.alias(roundedPerc, "percent")); } Suggestion importance[1-10]: 2 __ Why: While null checking is generally good practice, the `showPerc` argument is initialized with a default value of `Literal.FALSE` in `ArgumentFactory.java` (line 314), ensuring it will never be null. This makes the null check unnecessary in this context.	Low

github-actions · 2026-07-21T21:58:53Z

Persistent review updated to latest commit ca7e450

github-actions · 2026-07-21T22:07:18Z

Persistent review updated to latest commit dae1ecd

github-actions · 2026-07-22T16:09:28Z

Persistent review updated to latest commit 0b5b6ed

github-actions · 2026-07-22T20:52:37Z

PR Code Analyzer ❗

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

Path	Line	Severity	Description
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java	3237	low	Conflict-detection check tests for misspelled field name 'precent' instead of 'percent', so the guard against duplicate output fields will never fire. This renders the protective error message dead code, meaning a user with a pre-existing 'percent' field will get a silently incorrect result rather than the intended error. Appears to be a typo rather than intentional, but worth flagging as an anomaly.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 0 | Low: 1

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 · 2026-07-22T20:53:27Z

Persistent review updated to latest commit 0039627

* Clean docs * Fix CalcitePPLRareTopNTest showperc tests * Calculate percentages before filtering Signed-off-by: Ajimelec Gonzalez <ajimelec@amazon.com>

github-actions · 2026-07-22T20:56:15Z

Persistent review updated to latest commit b576e5a

AjimelecGonzalez requested review from LantaoJin, RyanL1997, Swiddis, acarbonetto, ahkcs, anirudha, dai-chen, joshuali925, mengweieric, noCharger, penghuo, ps48, qianheng-aws, songkant-aws, vamsimanohar, ykmr1224 and yuancu as code owners July 21, 2026 21:44

AjimelecGonzalez force-pushed the feature/showperc branch from 54ab043 to ca7e450 Compare July 21, 2026 21:57

AjimelecGonzalez force-pushed the feature/showperc branch from ca7e450 to dae1ecd Compare July 21, 2026 22:06

AjimelecGonzalez force-pushed the feature/showperc branch from dae1ecd to 0b5b6ed Compare July 22, 2026 16:08

ahkcs added the feature label Jul 22, 2026

ahkcs reviewed Jul 22, 2026

View reviewed changes

AjimelecGonzalez force-pushed the feature/showperc branch from 0b5b6ed to 0039627 Compare July 22, 2026 20:51

[Feature] showperc with top and rare commands.

b576e5a

* Clean docs * Fix CalcitePPLRareTopNTest showperc tests * Calculate percentages before filtering Signed-off-by: Ajimelec Gonzalez <ajimelec@amazon.com>

AjimelecGonzalez force-pushed the feature/showperc branch from 0039627 to b576e5a Compare July 22, 2026 20:54

ahkcs approved these changes Jul 22, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Feature] `showperc` with `top` and `rare` commands.#5642

[Feature] `showperc` with `top` and `rare` commands.#5642
AjimelecGonzalez wants to merge 1 commit into
opensearch-project:mainfrom
AjimelecGonzalez:feature/showperc

AjimelecGonzalez commented Jul 21, 2026

Uh oh!

github-actions Bot commented Jul 21, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 21, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 21, 2026

Uh oh!

github-actions Bot commented Jul 21, 2026

Uh oh!

github-actions Bot commented Jul 22, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jul 22, 2026

Uh oh!

github-actions Bot commented Jul 22, 2026

Uh oh!

github-actions Bot commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

AjimelecGonzalez commented Jul 21, 2026

Description

Syntax

Top Example

Rare Example

Related Issues

Check List

Uh oh!

github-actions Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit b576e5a)

Uh oh!

github-actions Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions Bot commented Jul 21, 2026

Uh oh!

github-actions Bot commented Jul 21, 2026

Uh oh!

github-actions Bot commented Jul 22, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jul 22, 2026

PR Code Analyzer ❗

Uh oh!

github-actions Bot commented Jul 22, 2026

Uh oh!

github-actions Bot commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

`Top` Example

`Rare` Example

github-actions Bot commented Jul 21, 2026 •

edited

Loading

(Review updated until commit `b576e5a`)

github-actions Bot commented Jul 21, 2026 •

edited

Loading