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 7950169)

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 bucket function grammar rules Relevant files: language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 sql/src/main/antlr/OpenSearchSQLParser.g4 Sub-PR theme: Implement bucket function AST lowering 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

⚡ Recommended focus areas for review Possible Issue The normalizeField method coerces a STRING literal to a column reference, but only checks the literal type, not whether the string content is a valid column name. If a user passes an arbitrary string (e.g., 'not-a-column'), it becomes a qualified name that may fail later during resolution. This could produce confusing error messages instead of catching the issue early. private static UnresolvedExpression normalizeField(UnresolvedExpression field) { if (field instanceof Literal literal && literal.getType() == DataType.STRING) { return AstDSL.qualifiedName(literal.getValue().toString()); } return field; } Possible Issue The substituteMissing method uses IFNULL because V2 has no implementation for coalesce, but the comment states V2 registers coalesce as a name. If V2 later implements coalesce, this code will continue using ifnull without any indication that the workaround is still necessary. Consider adding a TODO or version check to revisit this when coalesce becomes available. /** * Substitutes {@code missing} for a null field before bucketing. V2 registers `coalesce` as a * name but has no implementation for it, so the query fails at execution with "unsupported * function name"; `ifnull` is the two-argument form V2 actually evaluates. */ private static UnresolvedExpression substituteMissing( UnresolvedExpression field, UnresolvedExpression missing) { return missing == null ? field : new Function(IFNULL.getName().getFunctionName(), List.of(field, missing)); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 7950169

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Check duplicates before processing values The duplicate parameter check occurs after calling visit(arg.bucketArgValue()), which may perform expensive operations or have side effects. Move the duplicate check before visiting the value to avoid unnecessary processing when a duplicate is detected. sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java [193-200] Map<String, UnresolvedExpression> args = new LinkedHashMap<>(); 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]: 6 __ Why: Valid optimization that avoids unnecessary visit() calls when duplicates are detected. The improved code checks for duplicates before processing, which is more efficient, though the impact is moderate since duplicate parameters are likely rare.	Low

---

Previous suggestions

Suggestions up to commit 3f6b51c

Category	Suggestion	Impact
General	Check duplicates before processing values The duplicate parameter check occurs after visiting bucketArgValue(), which may execute expensive operations or have side effects. Check for duplicate keys before visiting the value to avoid unnecessary processing when a duplicate is detected. sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java [193-200] Map<String, UnresolvedExpression> args = new LinkedHashMap<>(); 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]: 6 __ Why: Valid optimization to check for duplicate parameter names before visiting bucketArgValue(), which could avoid unnecessary processing. However, the impact is moderate since duplicate parameters are likely rare in practice, and the visit() operation may not always be expensive.	Low

Suggestions up to commit 3a2b1e6

Category	Suggestion	Impact
Possible issue	Validate literal type is string or numeric The method returns a Literal but doesn't validate that the literal is actually a string or numeric type. A boolean or other non-string/non-numeric literal would pass through unchecked, potentially causing issues downstream when the interval is processed. sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java [244-253] private static Literal stringOrNumericArg(Map<String, UnresolvedExpression> args, String name) { UnresolvedExpression value = args.remove(name); if (value == null) { return null; } if (!(value instanceof Literal literal)) { throw new SemanticCheckException(name + " must be a literal; got " + value); } + DataType type = literal.getType(); + if (type != DataType.STRING && !type.isNumeric()) { + throw new SemanticCheckException(name + " must be a string or numeric literal; got " + type); + } return literal; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that stringOrNumericArg doesn't validate the literal's data type. Adding type validation would prevent invalid literal types (like boolean) from being accepted as interval values, improving robustness. However, this is a defensive check rather than fixing a critical bug, as downstream processing may already handle invalid types.	Medium

Suggestions up to commit bb83e90

Category	Suggestion	Impact
General	Validate before modifying arguments map The method modifies the args map by removing keys in stringOrNumericArg, but if multiple interval parameters are present, all are removed before the error is thrown. This could mask which specific parameters were duplicated. Consider checking for multiple intervals before removing any from the map. sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java [222-237] private static Literal intervalOf(Map<String, UnresolvedExpression> args, String functionName) { - List<Literal> supplied = - Stream.of("interval", "fixed_interval", "calendar_interval") - .map(key -> stringOrNumericArg(args, key)) - .filter(Objects::nonNull) - .collect(Collectors.toList()); - if (supplied.isEmpty()) { + List<String> intervalKeys = Stream.of("interval", "fixed_interval", "calendar_interval") + .filter(args::containsKey) + .collect(Collectors.toList()); + if (intervalKeys.isEmpty()) { throw new SemanticCheckException( functionName + " requires one of: interval, fixed_interval, calendar_interval"); } - if (supplied.size() > 1) { + if (intervalKeys.size() > 1) { throw new SemanticCheckException( functionName + " accepts only one of: interval, fixed_interval, calendar_interval"); } - return supplied.get(0); + return stringOrNumericArg(args, intervalKeys.get(0)); } Suggestion importance[1-10]: 7 __ Why: The suggestion improves code clarity by checking for multiple intervals before removing any from the map. This makes the error detection more explicit and avoids potential confusion about which parameters were present. The improved approach is cleaner and more maintainable, though the original code is functionally correct.	Medium
	Check for duplicates before processing The duplicate parameter check occurs after visiting the argument value, which could trigger unnecessary computation or side effects before detecting the duplicate. Move the duplicate check before visiting the value to fail fast and avoid processing invalid arguments. sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java [184-190] Map<String, UnresolvedExpression> args = new LinkedHashMap<>(); 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("Duplicate parameter: " + name); } + args.put(name, visit(arg.bucketArgValue())); } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that checking for duplicates before visiting the argument value is more efficient and avoids unnecessary computation. However, the impact is relatively minor since visit() is unlikely to have significant side effects in this context, and the original code is functionally correct.	Low

Suggestions up to commit cb0023a

Category	Suggestion	Impact
General	Check duplicates before visiting arguments The duplicate parameter check occurs after visiting the argument value, which means the visitor traverses the AST even for duplicate parameters. Move the duplicate check before visiting bucketArgValue() to avoid unnecessary AST traversal and potential side effects from visiting duplicate arguments. sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java [185-190] 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("Duplicate parameter: " + name); } + args.put(name, visit(arg.bucketArgValue())); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that checking for duplicates before visiting bucketArgValue() prevents unnecessary AST traversal and potential side effects. This is a good optimization that improves efficiency and code clarity.	Medium
	Validate literal type is string/numeric The method validates that the argument is a Literal but doesn't verify it's actually a string or numeric type. A boolean or other literal type could pass through. Add validation to ensure the literal is of an expected type (STRING, INTEGER, LONG, DOUBLE, etc.) to prevent invalid interval values. sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java [239-248] private static Literal stringOrNumericArg(Map<String, UnresolvedExpression> args, String name) { UnresolvedExpression value = args.remove(name); if (value == null) { return null; } if (!(value instanceof Literal literal)) { throw new SemanticCheckException(name + " must be a literal; got " + value); } + DataType type = literal.getType(); + if (type != DataType.STRING && type != DataType.INTEGER && type != DataType.LONG && type != DataType.DOUBLE) { + throw new SemanticCheckException(name + " must be a string or numeric literal"); + } return literal; } Suggestion importance[1-10]: 6 __ Why: The suggestion adds type validation to ensure only string or numeric literals are accepted for interval parameters. While this improves robustness, the impact is moderate since invalid types would likely fail downstream during span creation or execution.	Low

Suggestions up to commit 981a438

Category	Suggestion	Impact
General	Remove misleading interval examples The error message references interval examples ('1d', '15m') but this method is used for non-interval parameters like format and time_zone. The examples should be removed or made generic to avoid confusion when the error is raised for parameters that don't accept interval values. sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java [245-255] private static Literal stringArg(Map<String, UnresolvedExpression> args, String name) { UnresolvedExpression value = args.remove(name); if (value == null) { return null; } if (!(value instanceof Literal literal) || literal.getType() != DataType.STRING) { throw new SemanticCheckException( - name + " must be a string literal (e.g. '1d', '15m'); got " + value); + name + " must be a string literal; got " + value); } return literal; } Suggestion importance[1-10]: 6 __ Why: The error message includes interval examples ('1d', '15m') that are misleading when stringArg is used for non-interval parameters like format and time_zone. Removing these examples improves clarity and prevents user confusion.	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

[RyanL1997]

Test report — with and without the analytics engine

Verified locally on both routes at 510eaaf4.

This PR's tests

	default route	analytics engine
DateHistogramBucketFunctionIT	10 run, 10 passed	7 passed, 3 skipped, 0 failed
CsvFormatResponseIT (pre-existing)	25 passed	—
:sql:test	156 passed, coverage 1.0000 line / 1.0000 branch	—

The seven that run on both routes return identical values: hourly 12/24/17/19, half-hourly 5/7/11/13/17/19, daily 72, numeric 19/20/20/13.

The three skipped ones assert results only the legacy V1 engine produces — the positional date_histogram(field=<col>, …) spelling, and alias. That engine is reached through RestSQLQueryAction's SyntaxCheckException fallback; the analytics route enters through RestUnifiedQueryAction, which has none, so these have never worked there. Gated with @RequiresCapability(LEGACY_ENGINE_FALLBACK) rather than deleted — they guard the shapes a V2 grammar addition can quietly take away from the legacy engine, which is the regression CI caught here.

Full suite on the analytics engine

Whole sql.sql.* + sql.legacy.* suite, same machine, same cluster, run twice — once with main, once with this branch.

	main	this PR
Total	968	978
Passed	456	465
Failed	154	152
Skipped	358	361

Existing tests removed	0
Existing tests newly skipped	0
pass → fail	13
fail → pass	15
Added by this PR	10

The 13 and the 15 are the same kind of test — testPow, testDivide, ipTypeShouldPassJdbcFormatter — none related to these functions, moving in both directions. Re-running three of those classes twice on main, same cluster, nothing changed flipped 5 of 106 on its own. That noise floor covers the 13.

Also fixed

The dataset carried explicit document ids. Parquet-backed indices are append-only and reject them, so all 72 bulk items failed and every assertion saw an empty index. Dropping the ids lets one dataset serve both routes. timewrap_test.json has the same problem.

Harness notes

From dai-chen/sql-1 feature/ae-compat-test-refactor. Four things needed adjusting on macOS:

• protoc is a prerequisite the README doesn't list — the native build fails without it
• start-cluster.sh uses GNU sed -i
• the plugin version is pinned to 3.8.0.0-SNAPSHOT
• the run.gradle anchor it patches no longer exists on current main

[github-actions]

Persistent review updated to latest commit 510eaaf

[dai-chen]

Could you elaborate why this is not supported with current changes? I thought this should be handled by V2 only with grammar changes.

[RyanL1997]

thats correct and verified with fix. The previous assumption I made was wrong.

[RyanL1997]

With the grammar change V2 handles these directly, so two of the three gates are gone. The only one left is alias: no lowering here, and no legacy engine to fall back to on the analytics route.

One thing that isn't grammar-related — the bucket still has to be projected in a derived table before it can be grouped on. GROUP BY date_histogram(...) straight over a base table can't resolve the span's field.

[github-actions]

Persistent review updated to latest commit b2cdea1

[github-actions]

Persistent review updated to latest commit b954e10

[RyanL1997]

Hi @dai-chen, I just update this PR with the refactor commit I missed to push. Please re-take a look.

[github-actions]

Persistent review updated to latest commit 981a438

[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.

[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 now match what RelevanceQuery uses.

Delegating to execution isn't possible here though: 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 there's no function left downstream to check. Lowering there is also what lets one change serve both engines — the Calcite path never goes through ExpressionAnalyzer, so the AST is the only point they share. This is the span half of your suggestion; PPL's visitSpanClause does the same thing.

[github-actions]

Persistent review updated to latest commit 3f6b51c

[github-actions]

Persistent review updated to latest commit 7950169