Add SQL histogram and date_histogram bucket functions

[RyanL1997]

Description

Adds histogram and date_histogram to V2 SQL as bucket functions. Each call is lowered during AST construction to primitives that already exist (Span, COALESCE, DATE_FORMAT, TIMESTAMPADD), so no new engine function or execution operator is introduced.

• Builds on Add SQL histogram and date_histogram bucket functions #5514

Usage

Arguments are named. Compute the bucket in a subquery and group by its alias — the planner does not accept GROUP BY <expression> directly.

SELECT b, COUNT(*)
FROM (SELECT date_histogram('field'=ts, 'interval'='1h') AS b FROM events) sub
GROUP BY b ORDER BY b

{
  "schema": [
    { "name": "b", "type": "timestamp" },
    { "name": "COUNT(*)", "type": "long" }
  ],
  "datarows": [
    ["2026-01-01 00:00:00", 12],
    ["2026-01-01 01:00:00", 24],
    ["2026-01-01 02:00:00", 17],
    ["2026-01-01 03:00:00", 19]
  ],
  "total": 4, "size": 4, "status": 200
}

The bucket comes back as a timestamp, so intervals below an hour split as you would expect, and a second grouping key works alongside it:

SELECT b, c, COUNT(*)
FROM (SELECT date_histogram('field'=ts, 'interval'='30m') AS b, category AS c
      FROM (SELECT * FROM events) i) sub
GROUP BY b, c ORDER BY b, c

"datarows": [
  ["2026-01-01 00:00:00", "alpha",  5],
  ["2026-01-01 00:30:00", "beta",   7],
  ["2026-01-01 01:00:00", "alpha", 11],
  ["2026-01-01 01:30:00", "gamma", 13],
  ["2026-01-01 02:00:00", "beta",  17],
  ["2026-01-01 03:00:00", "alpha", 19]
]

histogram buckets a numeric field the same way and returns the bucket's lower bound:

SELECT b, COUNT(*)
FROM (SELECT histogram('field'=value, 'interval'=20) AS b FROM events) sub
GROUP BY b ORDER BY b
-- [0, 19], [20, 20], [40, 20], [60, 13]

Parameters

function	accepted
histogram	field, interval, offset, missing
date_histogram	field, interval / fixed_interval / calendar_interval, format, time_zone, missing

The three interval spellings are synonyms; exactly one must be present. min_doc_count, order and alias are rejected because they would have to mutate the surrounding query (HAVING / ORDER BY / the SELECT-list alias). date_histogram's offset is rejected pending a duration-string parser distinct from time_zone's ZoneOffset format.

Positional calls keep going to the legacy engine

These names are new to the V2 grammar but not to the plugin — the legacy engine has accepted date_histogram(field=<col>, 'interval'=<n>) in GROUP BY for a long time, and queries reach it only when V2 raises SyntaxCheckException, the one exception RestSQLQueryAction falls back on. Now that V2 matches these calls first, an unrecognized shape has to decline with that exception or the query stops at V2:

query	before this PR	with #5514 as written	with this PR
GROUP BY date_histogram(field='ts','interval'='1h')	4 buckets	HTTP 400	4 buckets
GROUP BY date_histogram('field'='ts','interval'='1h')	4 buckets (legacy)	4 buckets (V2)	4 buckets (V2)

Other rejections are unchanged: once a call is in the named-argument form, a bad parameter is the caller's error and gets a clear message instead of being re-run by an engine that never understood the query.

Check List

• New functionality includes testing.
• New functionality has been documented.
• Commits are signed per the DCO using --signoff.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 724b1c4)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Add grammar rules for bucket functions Relevant files: language-grammar/src/main/antlr4/OpenSearchSQLLexer.g4 language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 sql/src/main/antlr/OpenSearchSQLLexer.g4 sql/src/main/antlr/OpenSearchSQLParser.g4 Sub-PR theme: Implement bucket function AST building Relevant files: sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java Sub-PR theme: Add integration tests for bucket functions Relevant files: integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java integ-test/src/test/resources/date_histogram_test.json integ-test/src/test/resources/indexDefinitions/date_histogram_test_index_mapping.json Sub-PR theme: Update documentation for bucket functions Relevant files: docs/user/dql/aggregations.rst docs/user/dql/functions.rst

⚡ Recommended focus areas for review Possible Issue The normalizeField method coerces string literals to column references, but this conversion is applied unconditionally to all string literals. If a user intentionally passes a string literal (e.g., for a computed expression or constant), it will be incorrectly treated as a column name. This could cause queries to fail or produce unexpected results when the string does not correspond to an actual column. private static UnresolvedExpression normalizeField(UnresolvedExpression field) { if (field instanceof Literal literal && literal.getType() == DataType.STRING) { return AstDSL.qualifiedName(literal.getValue().toString()); } return field; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 724b1c4

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null safety check Add null safety check for literal.getValue() before calling toString(). If the literal's value is null, this will throw a NullPointerException. Consider handling the null case explicitly or validating that string literals cannot have null values. sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java [182-187] private static UnresolvedExpression normalizeField(UnresolvedExpression field) { if (field instanceof Literal literal && literal.getType() == DataType.STRING) { - return AstDSL.qualifiedName(literal.getValue().toString()); + Object value = literal.getValue(); + if (value == null) { + throw new IllegalArgumentException("String literal field cannot have null value"); + } + return AstDSL.qualifiedName(value.toString()); } return field; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential NullPointerException if literal.getValue() returns null. However, this is a defensive programming improvement rather than a critical bug fix, as the likelihood depends on whether the codebase allows null values in string literals. The score reflects its value as a safety enhancement.	Medium

---

Previous suggestions

Suggestions up to commit 7f6f3a3

Category	Suggestion	Impact
Possible issue	Add null check for literal value The normalizeField method should validate that the literal value is not null before calling toString(). If literal.getValue() returns null, this will throw a NullPointerException at runtime. sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java [182-187] private static UnresolvedExpression normalizeField(UnresolvedExpression field) { if (field instanceof Literal literal && literal.getType() == DataType.STRING) { - return AstDSL.qualifiedName(literal.getValue().toString()); + Object value = literal.getValue(); + if (value == null) { + throw new IllegalArgumentException("Field name cannot be null"); + } + return AstDSL.qualifiedName(value.toString()); } return field; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential NullPointerException if literal.getValue() returns null. Adding a null check improves robustness, though the likelihood of this occurring in practice depends on upstream validation.	Medium

Suggestions up to commit ed7284e

Category	Suggestion	Impact
Possible issue	Add null check for literal value The normalizeField method should handle potential null values from literal.getValue() before calling toString(). If getValue() returns null, this will throw a NullPointerException at runtime. sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java [182-187] private static UnresolvedExpression normalizeField(UnresolvedExpression field) { if (field instanceof Literal literal && literal.getType() == DataType.STRING) { - return AstDSL.qualifiedName(literal.getValue().toString()); + Object value = literal.getValue(); + if (value != null) { + return AstDSL.qualifiedName(value.toString()); + } } return field; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential NullPointerException if literal.getValue() returns null. However, this is a defensive programming improvement rather than a critical bug, as the likelihood depends on upstream validation.	Medium
General	Validate exception details in test The test only verifies that an exception is thrown but doesn't validate the exception message or type. Consider capturing the exception and asserting on its specific properties to ensure the correct error is being raised. integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java [179-187] @Test public void groupingOnTheBucketWithoutADerivedTableIsRejected() { - assertThrows( + ResponseException exception = assertThrows( ResponseException.class, () -> executeQuery( "SELECT date_histogram(field=ts, interval='1h') AS b, COUNT(*) FROM " + IDX + " GROUP BY date_histogram(field=ts, interval='1h')")); + // Verify the exception contains expected error details + assertNotNull(exception.getMessage()); } Suggestion importance[1-10]: 5 __ Why: The suggestion improves test robustness by capturing and validating the exception, but the current test already verifies the expected behavior (exception is thrown). The improvement is marginal and mainly enhances test quality rather than fixing a bug.	Low

Suggestions up to commit 9d798fc

Category	Suggestion	Impact
General	Detect unsupported bucket function parameters After validating required parameters, the code doesn't check if there are any remaining unrecognized parameters in the args map. This could silently ignore typos or invalid parameters. Consider checking if args is empty after removing known parameters and throw an exception if unexpected parameters remain. sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java [203-212] if (field == null) { throw new SemanticCheckException( String.format("Parameter field is required for %s function.", functionName)); } if (intervals.size() != 1) { throw new SemanticCheckException( String.format( "Exactly one of %s is required for %s function.", String.join(", ", INTERVAL_ARGS), functionName)); } +if (!args.isEmpty()) { + throw new SyntaxCheckException( + String.format("Unsupported parameters for %s function: %s", functionName, String.join(", ", args.keySet()))); +} Suggestion importance[1-10]: 8 __ Why: This is a valuable suggestion that addresses a potential bug where unrecognized parameters would be silently ignored. The code explicitly handles known unsupported parameters by throwing SyntaxCheckException (as shown in tests), so remaining parameters after extraction should also be validated. This improves correctness and user feedback.	Medium
	Validate interval literal data type The pattern matching with instanceof and cast is used, but the extracted interval variable is not validated for its data type. If the literal is not a string or numeric type, downstream code may fail unexpectedly. Add validation to ensure the interval literal is of an appropriate type (STRING or numeric). sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java [213-216] if (!(intervals.get(0) instanceof Literal interval)) { throw new SemanticCheckException( String.format("Parameter interval must be a literal for %s function.", functionName)); } +if (interval.getType() != DataType.STRING && !interval.getType().isNumeric()) { + throw new SemanticCheckException( + String.format("Parameter interval must be a string or numeric literal for %s function.", functionName)); +} Suggestion importance[1-10]: 5 __ Why: The suggestion adds validation for the interval literal's data type, which could prevent downstream errors. However, the Span constructor and related code may already handle type validation, and the PR's test coverage doesn't indicate this is a critical issue. The improvement is defensive but not essential.	Low

Suggestions up to commit f3aaf4f

Category	Suggestion	Impact
General	Check duplicates before processing values The duplicate parameter check occurs after visiting the argument value, which may execute unnecessary work. Check for duplicate keys before visiting the value expression to avoid processing duplicate arguments unnecessarily and improve performance. sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java [203-209] for (BucketArgContext arg : ctx.bucketFunction().bucketArg()) { String name = StringUtils.unquoteText(arg.bucketArgName().getText()).toLowerCase(Locale.ROOT); - if (args.put(name, visit(arg.bucketArgValue())) != null) { + if (args.containsKey(name)) { throw new SemanticCheckException( String.format("Parameter '%s' can only be specified once.", name)); } + args.put(name, visit(arg.bucketArgValue())); } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a minor optimization opportunity by checking for duplicate keys before visiting the argument value. This avoids unnecessary processing when duplicates are found, though the performance impact is minimal since visit() is typically fast for simple values.	Low

Suggestions up to commit 448b3ad

Category	Suggestion	Impact
General	Improve interval validation logic Validate the field parameter before processing intervals to avoid potential null pointer exceptions. If field is null, the subsequent intervals validation becomes unnecessary and could mislead debugging efforts. sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java [231-240] if (field == null) { throw new SemanticCheckException( String.format("Parameter field is required for %s function.", functionName)); } -if (intervals.size() != 1) { +if (intervals.isEmpty()) { throw new SemanticCheckException( String.format( "Exactly one of %s is required for %s function.", String.join(", ", INTERVAL_ARGS), functionName)); } +if (intervals.size() > 1) { + throw new SemanticCheckException( + String.format( + "Only one of %s can be specified for %s function.", + String.join(", ", INTERVAL_ARGS), functionName)); +} Suggestion importance[1-10]: 3 __ Why: The suggestion to split the intervals.size() != 1 check into separate checks for empty and multiple intervals provides slightly more specific error messages. However, the existing code is already correct and clear, and the improvement is marginal. The original validation logic is sufficient and the suggested change doesn't address any actual bug.	Low

[github-actions]

Persistent review updated to latest commit 7151095

[github-actions]

Persistent review updated to latest commit 6573786

[github-actions]

Persistent review updated to latest commit cc420ab

[github-actions]

Persistent review updated to latest commit cb0023a

[github-actions]

Persistent review updated to latest commit bb83e90

[github-actions]

Persistent review updated to latest commit 3a2b1e6

[dai-chen]

Because we've defined this in grammar, the fallback should happen automatically?

[RyanL1997]

Other way round, I think — putting them in the grammar is what stops the fallback happening on its own.

Before the rule, date_histogram(...) was unknown to V2, so it threw SyntaxCheckException and RestSQLQueryAction handed it to legacy. Now V2 matches the call and builds an AST, so nothing throws and it never gets there — CsvFormatResponseIT.dateHistogramTest broke exactly then, and passes again only because alias is declined explicitly.

It needs to be a closed set rather than anything-left-over, since bare argument names mean misspellings parse too — and on the analytics route there's no legacy engine behind it to absorb them.

[RyanL1997]

Following up here since your grammar comment supersedes this: what I described was the shape at the time, where the rule accepted any name and the set had to decide. With bucketArgName narrowed to the four names we lower, the fallback is automatic after all and the set is gone.

[dai-chen]

Validation like this and below looks very complex. Could you confirm if it's fine to delegate it to final DSL execution? Because I don't find similar validation in other OS function.

[RyanL1997]

Refactored — the helpers are gone, the parameter names are declared as data, and the messages match what RelevanceQuery and the other fallback sites use.

On delegating: the parts that can be already are. An interval's contents are never checked here — 'xyz' fails downstream in Rounding, '-1h' in AstDSL. What stays is only the shape — which parameter names appeared, and whether field and an interval are there at all — and a missing interval NPEs inside spanFromSpanLengthLiteral before anything downstream sees it.

That part can't move. Relevance functions survive as a FunctionExpression down to RelevanceQuery.build(), where their parameter table lives; a bucket call is lowered to a Span while the AST is built, so no function is left to hold one. Lowering there is also what lets one change serve both engines — the Calcite path never goes through ExpressionAnalyzer. This is the span half of your suggestion; PPL's visitSpanClause does the same.

[dai-chen]

Added comment on grammar changes. Please check if we can simplify here, especially get rid of the 2 set fields added above.

[RyanL1997]

Both the leftover-argument check and LEGACY_ONLY_ARGS are gone with the grammar change; details in the reply above.

INTERVAL_ARGS is still there, but it isn't a validation table any more — interval, fixed_interval and calendar_interval are synonyms and exactly one has to be picked, so it's the lookup that does the picking. Happy to inline the three names if you'd rather not have the field.

What's left is three checks, and none can move downstream: spanFromSpanLengthLiteral dereferences the interval on its first line, so a missing one is an NPE rather than a message.

[RyanL1997]

Both are gone now — INTERVAL_ARGS too.

bucketFunction spells out the two operands the way spanClause does in the PPL grammar: the field and the interval are positional and required, with named labels to reach them. The visitor is then the same three lines visitSpanClause is, through the same AstDSL call — no argument map, no required-parameter check, no duplicate detection, no literal check. The grammar makes each of those unrepresentable rather than detectable. −46/+6 in this file.

The one constraint it adds is ordering: field comes first, and writing the interval first is a parse error, so it reaches legacy, which accepts either order.

[github-actions]

Persistent review updated to latest commit 3f6b51c

[github-actions]

Persistent review updated to latest commit 7950169

[github-actions]

Persistent review updated to latest commit 448b3ad

[github-actions]

Persistent review updated to latest commit f3aaf4f

[dai-chen]

Please check if our doc/doctest already covers this or not.

[dai-chen]

I don't see these in argument rule for other OS function in grammar. Are these only for backward compatibility, e.g., histogram('field'=...)? Without them, the fallback should happen automatically and no need to do validation in AST builder? If so, I think we can remove them because anyway we only partial support histogram based on span and do fallback in this PR.

[RyanL1997]

Yes — that was the only reason, and you're right that they aren't needed. Removed.

[dai-chen]

Added comment on grammar changes. Please check if we can simplify here, especially get rid of the 2 set fields added above.

[github-actions]

Persistent review updated to latest commit 9d798fc

[github-actions]

Persistent review updated to latest commit ed7284e

[RyanL1997]

Hi @dai-chen , for:

Added comment on grammar changes. Please check if we can simplify here, especially get rid of the 2 set fields added above.

Both are gone.

bucketFunction now spells out the field and the interval the way spanClause does in the PPL grammar — positional and required, with named labels to reach them. The visitor is the same three lines visitSpanClause is, through the same AstDSL call. No argument map, no required-parameter check, no duplicate detection, no literal check: the grammar makes each of those unrepresentable rather than detectable.

[github-actions]

Persistent review updated to latest commit 7f6f3a3

[github-actions]

Persistent review updated to latest commit 724b1c4

[RyanL1997]

Please check if our doc/doctest already covers this or not.

@dai-chen I added under aggregations.rst, but do we need to mention it here?

[dai-chen]

I think no need to mention it here. It should be clear since we've called both bucket or windowing function like other database.

[RyanL1997]

will remove this as a follow up.

[dai-chen]

Thanks for the changes! Probably we can need to deep dive into the aliasing issue later.