Add scalar min/max to BuiltinFunctionName

[LantaoJin]

Description

Add scalar min/max to BuiltinFunctionName.

Since parsing min/max aggregation function and scalar function (eval function) in AST parser are separated, we can use different names in BuiltinFunctionName with no changes in PPL interface/syntax.

eval a = max(b) -> SCALAR_MAX
stats max(b)/eventstats max(b)/streamstats max(b) -> (AGG) MAX

Related Issues

Resolves #4774

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.

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes introduce distinct enum entries for scalar MIN/MAX functions and update PPL function registration and translation mappings to use these specialized names instead of shared aggregation function names, preventing naming conflicts during PPL-to-SQL translation.

Changes

Cohort / File(s)	Summary
Aggregation resolution core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java	Updated aggregation function name resolution to use BuiltinFunctionName.ofAggregation() instead of the generic of() method for explicit aggregation mapping.
Enum expansion core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java	Added two new enum entries: SCALAR_MAX and SCALAR_MIN with corresponding function names, separating scalar variants from aggregate versions.
PPL function registration and mapping core/src/main/java/org/opensearch/sql/ppl/parser/PPLFuncImpTable.java, core/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java	Updated PPL scalar max/min function registration to use new SCALAR_MAX/SCALAR_MIN enum entries; renamed FUNCTION_NAME_MAPPING to EVAL_FUNCTION_NAME_MAPPING and added mappings for eval "max"/"min" commands to route to scalar variants.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

• Verify that all references to the renamed FUNCTION_NAME_MAPPING variable have been updated in AstExpressionBuilder.java
• Confirm new enum entries are properly registered and accessible across the codebase
• Ensure the mappings for "max" and "min" in EVAL_FUNCTION_NAME_MAPPING correctly reference the new SCALAR_MAX and SCALAR_MIN entries
• Check that PPLFuncImpTable.java registrations align with the new enum values

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Add scalar min/max to BuiltinFunctionName' accurately summarizes the main change—adding two new enum values (SCALAR_MAX and SCALAR_MIN) to the BuiltinFunctionName enum.
Linked Issues check	✅ Passed	The pull request fulfills the core requirement from #4774: introducing non-conflicting function names (SCALAR_MAX/SCALAR_MIN) and updating the PPL translator to map scalar calls to these internal names, resolving the MIN/MAX collision issue.
Out of Scope Changes check	✅ Passed	All changes—adding SCALAR_MAX/SCALAR_MIN to the enum, updating PPL function registration, and renaming the eval function mapping—are directly aligned with resolving the scalar/aggregate MIN/MAX conflict described in #4774.
Description check	✅ Passed	The PR description clearly explains the changes: adding scalar min/max to BuiltinFunctionName with separate handling for eval functions versus aggregation functions.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java (1)

61-63: SCALAR_MAX/MIN enum entries are consistent; consider visibility intent

Defining SCALAR_MAX(FunctionName.of("scalar_max")) and SCALAR_MIN(FunctionName.of("scalar_min")) and keeping aggregation/window mappings pointing at MAX/MIN preserves aggregate behavior while enabling separate scalar wiring through these new names.

If scalar_max / scalar_min are intended to be purely internal implementation details for PPL eval (never user‑facing as function names), consider marking them as internal (true in the enum constructor) so they don’t show up in generic function listings that rely on ALL_NATIVE_FUNCTIONS.

Also applies to: 367-424

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 11727a4 and bef2b4b.

📒 Files selected for processing (4)

• core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java (1 hunks)
• core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java (1 hunks)
• core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java (2 hunks)
• ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java (2 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

**/*.java

📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)

**/*.java: Use PascalCase for class names (e.g., QueryExecutor)
Use camelCase for method and variable names (e.g., executeQuery)
Use UPPER_SNAKE_CASE for constants (e.g., MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
Prefer Optional<T> for nullable returns in Java
Avoid unnecessary object creation in loops
Use StringBuilder for string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code

Files:

• core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java
• core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java
• ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java
• core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java

---

⚙️ CodeRabbit configuration file

**/*.java: - Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)

• Check for proper JavaDoc on public classes and methods
• Flag redundant comments that restate obvious code
• Ensure methods are under 20 lines with single responsibility
• Verify proper error handling with specific exception types
• Check for Optional usage instead of null returns
• Validate proper use of try-with-resources for resource management

Files:

• core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java
• core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java
• ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java
• core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java

**/ppl/**/*.java

⚙️ CodeRabbit configuration file

**/ppl/**/*.java: - For PPL parser changes, verify grammar tests with positive/negative cases

• Check AST generation for new syntax
• Ensure corresponding AST builder classes are updated
• Validate edge cases and boundary conditions

Files:

• ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java

**/calcite/**/*.java

⚙️ CodeRabbit configuration file

**/calcite/**/*.java: - Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor

• Verify SQL generation and optimization paths
• Document any Calcite-specific workarounds
• Test compatibility with Calcite version constraints

Files:

• core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java

🧠 Learnings (2)

📚 Learning: 2025-12-02T17:27:55.938Z

Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Update corresponding AST builder classes when making PPL grammar changes

Applied to files:

• ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java

📚 Learning: 2025-12-02T17:27:55.938Z

Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration

Applied to files:

• core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java

🧬 Code graph analysis (1)

core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java (1)

core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java (1)

• PPLBuiltinOperators (110-512)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
• GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
• GitHub Check: build-windows-macos (macos-14, 21, integration)

🔇 Additional comments (2)

core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java (1)

189-190: Scalar MAX/MIN registration correctly routed through dedicated operators

Using SCALAR_MAX/SCALAR_MIN here and wiring them to PPLBuiltinOperators.SCALAR_MAX/SCALAR_MIN cleanly separates scalar eval calls from aggregate MAX/MIN, without touching the aggregate registry. This is consistent with the new enum entries and should resolve the name collision issue described in the linked bug.

Also applies to: 873-875

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java (1)

2906-2919: Using ofAggregation here correctly isolates aggregate MIN/MAX from scalar variants

Switching to BuiltinFunctionName.ofAggregation(aggFunctionName) in buildAggCall ensures chart/timechart secondary aggregations always resolve min/max through the aggregate mapping, preventing collisions with the new scalar SCALAR_MIN/SCALAR_MAX. The existing switch logic for MIN/EARLIEST, MAX/LATEST, AVG, and the default sum path remains intact and correct.

[yuancu]

Can you please kindly add a javadoc on BuiltinFunctionName.of to instruct future developers to use BuiltinFunctionName.ofAggregation for aggregation functions?

[dai-chen]

Thanks for the fix!

[opensearch-trigger-bot]

The backport to 2.19-dev failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.19-dev 2.19-dev
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.19-dev
# Create a new branch
git switch --create backport/backport-4967-to-2.19-dev
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 7dfabcea94952ead0a27463d810097d74446acc4
# Push it to GitHub
git push --set-upstream origin backport/backport-4967-to-2.19-dev
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.19-dev

Then, create a pull request where the base branch is 2.19-dev and the compare/head branch is backport/backport-4967-to-2.19-dev.