From 65737864d5b805dad402df2cb59c5f914b6e646f Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Mon, 17 Aug 2026 11:22:35 -0700 Subject: [PATCH 01/18] Add SQL histogram and date_histogram bucket functions Adds parse-time support for `histogram` and `date_histogram` in V2 SQL with named-argument invocation. 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, and the lowering happens before the V2 and analytics-engine paths diverge. Supported parameters: histogram field, interval, offset, missing date_histogram field, interval / fixed_interval / calendar_interval, format, time_zone, missing `min_doc_count`, `order` and `alias` are rejected: they would have to mutate the surrounding query (HAVING / ORDER BY / the SELECT-list alias), which needs parser plumbing that reaches outside the function call. `date_histogram`'s `offset` is rejected pending a duration-string parser distinct from `time_zone`'s ZoneOffset format. These functions are new to the V2 grammar but not to the plugin, and that is where the care is needed. The legacy engine has accepted `date_histogram(field=, 'interval'=)` in GROUP BY since before V2 existed, and requests reach it only when V2 raises SyntaxCheckException -- the only type RestSQLQueryAction falls back on. Teaching V2 to match those calls means it answers them first, so declining an unrecognized call shape with SemanticCheckException would stop the query at V2 and silently drop a working feature. Measured on a live cluster, `SELECT COUNT(*) FROM idx GROUP BY date_histogram(field='ts','interval'='1h')` returned four buckets before the grammar change and HTTP 400 after it. Both expanders therefore decline an unrecognized shape with SyntaxCheckException. Every other rejection is unchanged on purpose: once a call is in the property-bag form these expanders own, a bad parameter is the caller's mistake, and handing it to an engine that never understood the query would answer a clear error with a confusing one. The expander unit tests assert the shape of the AST that gets built, which says nothing about whether the lowered Span survives analysis, planning and pushdown. DateHistogramBucketFunctionIT asserts bucket keys and counts against date_histogram_test, 72 documents on fixed timestamps chosen so an hourly grouping must yield 12/24/17/19 and a half-hourly one 5/7/11/13/17/19. It covers hourly, half-hourly and daily intervals, the fixed_interval and calendar_interval synonyms, a second grouping key, a WHERE clause, numeric histogram buckets, and both positional forms still reaching the legacy engine. One test records a limitation rather than a guarantee. Selecting the bucket alongside a second grouping key directly off the table leaves the span's field typed UNDEFINED by the time the aggregate runs and the request fails; wrapping the scan in its own derived table resolves it, and a single grouping key is unaffected either way. Clients already emit the wrapped form, so this is pinned where it can be seen rather than left as folklore in a comment. Co-authored-by: Varun Signed-off-by: Jialiang Liang --- .../sql/legacy/SQLIntegTestCase.java | 8 +- .../sql/DateHistogramBucketFunctionIT.java | 193 +++++++++ .../test/resources/date_histogram_test.json | 144 +++++++ .../src/main/antlr4/OpenSearchSQLParser.g4 | 6 + sql/src/main/antlr/OpenSearchSQLParser.g4 | 6 + .../sql/sql/parser/AstExpressionBuilder.java | 15 +- .../parser/bucket/BucketFunctionExpander.java | 22 ++ .../parser/bucket/BucketFunctionRegistry.java | 32 ++ .../parser/bucket/BucketFunctionUtils.java | 44 +++ .../parser/bucket/DateHistogramExpander.java | 135 +++++++ .../sql/parser/bucket/HistogramExpander.java | 72 ++++ .../sql/sql/parser/bucket/NamedArguments.java | 142 +++++++ .../bucket/BucketFunctionRegistryTest.java | 53 +++ .../bucket/BucketFunctionUtilsTest.java | 56 +++ .../bucket/DateHistogramExpanderTest.java | 367 ++++++++++++++++++ .../parser/bucket/HistogramExpanderTest.java | 296 ++++++++++++++ .../sql/parser/bucket/NamedArgumentsTest.java | 249 ++++++++++++ 17 files changed, 1838 insertions(+), 2 deletions(-) create mode 100644 integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java create mode 100644 integ-test/src/test/resources/date_histogram_test.json create mode 100644 sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionExpander.java create mode 100644 sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionRegistry.java create mode 100644 sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionUtils.java create mode 100644 sql/src/main/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpander.java create mode 100644 sql/src/main/java/org/opensearch/sql/sql/parser/bucket/HistogramExpander.java create mode 100644 sql/src/main/java/org/opensearch/sql/sql/parser/bucket/NamedArguments.java create mode 100644 sql/src/test/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionRegistryTest.java create mode 100644 sql/src/test/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionUtilsTest.java create mode 100644 sql/src/test/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpanderTest.java create mode 100644 sql/src/test/java/org/opensearch/sql/sql/parser/bucket/HistogramExpanderTest.java create mode 100644 sql/src/test/java/org/opensearch/sql/sql/parser/bucket/NamedArgumentsTest.java diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java index fc15c908c63..e0f4b53e3f6 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java @@ -994,7 +994,13 @@ public enum Index { "timewrap_test", "timewrap_test", "{\"mappings\":{\"properties\":{\"@timestamp\":{\"type\":\"date\"},\"host\":{\"type\":\"keyword\"},\"requests\":{\"type\":\"integer\"},\"errors\":{\"type\":\"integer\"}}}}", - "src/test/resources/timewrap_test.json"); + "src/test/resources/timewrap_test.json"), + DATE_HISTOGRAM_TEST( + "date_histogram_test", + "date_histogram_test", + "{\"mappings\":{\"properties\":{\"ts\":{\"type\":\"date\",\"format\":\"yyyy-MM-dd" + + " HH:mm:ss\"},\"category\":{\"type\":\"keyword\"},\"value\":{\"type\":\"integer\"}}}}", + "src/test/resources/date_histogram_test.json"); private final String name; private final String type; diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java new file mode 100644 index 00000000000..cef009a68cc --- /dev/null +++ b/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java @@ -0,0 +1,193 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.sql; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; +import static org.opensearch.sql.util.MatcherUtils.rows; +import static org.opensearch.sql.util.MatcherUtils.verifyDataRows; +import static org.opensearch.sql.util.MatcherUtils.verifyDataRowsInOrder; + +import java.io.IOException; +import org.json.JSONObject; +import org.junit.Test; +import org.opensearch.client.ResponseException; +import org.opensearch.sql.legacy.SQLIntegTestCase; + +/** + * Execution coverage for {@code date_histogram} and {@code histogram}. The expander unit tests + * assert the AST that gets built; these assert what comes back after analysis, planning and + * pushdown, against 72 documents on fixed timestamps: + * + * 
+ *   00:00 x5 alpha   00:30 x7 beta    01:00 x11 alpha
+ *   01:45 x13 gamma  02:00 x17 beta   03:00 x19 alpha
+ *
+ * + * so hourly grouping must yield 12/24/17/19 and half-hourly 5/7/11/13/17/19. + */ +public class DateHistogramBucketFunctionIT extends SQLIntegTestCase { + + private static final String IDX = "date_histogram_test"; + + @Override + protected void init() throws Exception { + super.init(); + loadIndex(Index.DATE_HISTOGRAM_TEST); + } + + /** The planner rejects {@code GROUP BY }, so the bucket is aliased in a subquery. */ + private static String bucketed(String bucketExpr) { + return "SELECT b, COUNT(*) FROM (SELECT " + + bucketExpr + + " AS b FROM " + + IDX + + ") sub GROUP BY b ORDER BY b"; + } + + @Test + public void hourlyBucketsCarryKeysAndCounts() throws IOException { + JSONObject response = executeQuery(bucketed("date_histogram('field'=ts, 'interval'='1h')")); + + verifyDataRowsInOrder( + response, + rows("2026-01-01 00:00:00", 12), + rows("2026-01-01 01:00:00", 24), + rows("2026-01-01 02:00:00", 17), + rows("2026-01-01 03:00:00", 19)); + } + + /** A sub-hour interval must split 00:00/00:30 and 01:00/01:45 rather than merge them. */ + @Test + public void halfHourlyBucketsSplitWithinTheHour() throws IOException { + JSONObject response = executeQuery(bucketed("date_histogram('field'=ts, 'interval'='30m')")); + + verifyDataRowsInOrder( + response, + rows("2026-01-01 00:00:00", 5), + rows("2026-01-01 00:30:00", 7), + rows("2026-01-01 01:00:00", 11), + rows("2026-01-01 01:30:00", 13), + rows("2026-01-01 02:00:00", 17), + rows("2026-01-01 03:00:00", 19)); + } + + @Test + public void dailyIntervalCollapsesEverythingIntoOneBucket() throws IOException { + JSONObject response = executeQuery(bucketed("date_histogram('field'=ts, 'interval'='1d')")); + + verifyDataRows(response, rows("2026-01-01 00:00:00", 72)); + } + + /** {@code fixed_interval} and {@code calendar_interval} are accepted as synonyms of interval. */ + @Test + public void intervalSynonymsProduceTheSameBuckets() throws IOException { + JSONObject viaFixed = + executeQuery(bucketed("date_histogram('field'=ts, 'fixed_interval'='1h')")); + JSONObject viaCalendar = + executeQuery(bucketed("date_histogram('field'=ts, 'calendar_interval'='1h')")); + + for (JSONObject response : new JSONObject[] {viaFixed, viaCalendar}) { + verifyDataRowsInOrder( + response, + rows("2026-01-01 00:00:00", 12), + rows("2026-01-01 01:00:00", 24), + rows("2026-01-01 02:00:00", 17), + rows("2026-01-01 03:00:00", 19)); + } + } + + /** Only resolves with the scan in its own derived table — see the test below. */ + @Test + public void bucketsCombineWithAnAdditionalGroupingKey() throws IOException { + JSONObject response = + executeQuery( + "SELECT b, c, COUNT(*) FROM (SELECT date_histogram('field'=ts, 'interval'='1h') AS b," + + " category AS c FROM (SELECT * FROM " + + IDX + + ") inner_scan) sub GROUP BY b, c ORDER BY b, c"); + + verifyDataRowsInOrder( + response, + rows("2026-01-01 00:00:00", "alpha", 5), + rows("2026-01-01 00:00:00", "beta", 7), + rows("2026-01-01 01:00:00", "alpha", 11), + rows("2026-01-01 01:00:00", "gamma", 13), + rows("2026-01-01 02:00:00", "beta", 17), + rows("2026-01-01 03:00:00", "alpha", 19)); + } + + /** + * A limitation, not a guarantee: two grouping keys over a bare table scan leave the span's field + * typed UNDEFINED. One key is fine, and a derived table resolves it. If this starts passing, the + * engine was fixed — relax the test. + */ + @Test + public void bucketWithASecondKeyNeedsTheScanInItsOwnDerivedTable() { + ResponseException error = + assertThrows( + ResponseException.class, + () -> + executeQuery( + "SELECT b, c, COUNT(*) FROM (SELECT date_histogram('field'=ts," + + " 'interval'='1h') AS b, category AS c FROM " + + IDX + + ") sub GROUP BY b, c ORDER BY b, c")); + + assertEquals(400, error.getResponse().getStatusLine().getStatusCode()); + assertTrue(error.getMessage().contains("UNDEFINED")); + } + + @Test + public void bucketsRespectAWhereClause() throws IOException { + JSONObject response = + executeQuery( + "SELECT b, COUNT(*) FROM (SELECT date_histogram('field'=ts, 'interval'='1h') AS b FROM " + + IDX + + " WHERE category = 'alpha') sub GROUP BY b ORDER BY b"); + + verifyDataRowsInOrder( + response, + rows("2026-01-01 00:00:00", 5), + rows("2026-01-01 01:00:00", 11), + rows("2026-01-01 03:00:00", 19)); + } + + @Test + public void numericHistogramBucketsByInterval() throws IOException { + JSONObject response = + executeQuery( + "SELECT b, COUNT(*) FROM (SELECT histogram('field'=value, 'interval'=20) AS b FROM " + + IDX + + ") sub GROUP BY b ORDER BY b"); + + // value runs 1..72, so the 20-wide buckets hold 19, 20, 20 and 13 documents. + verifyDataRowsInOrder(response, rows(0, 19), rows(20, 20), rows(40, 20), rows(60, 13)); + } + + /** + * V2 now matches these calls first, so declining with anything but SyntaxCheckException would cut + * off the legacy engine that has always answered the positional form. + */ + @Test + public void positionalCallStillReachesTheLegacyEngine() throws IOException { + JSONObject response = + executeQuery( + "SELECT COUNT(*) FROM " + IDX + " GROUP BY date_histogram(field='ts','interval'='1h')"); + + verifyDataRows(response, rows(12), rows(24), rows(17), rows(19)); + } + + @Test + public void positionalNumericHistogramStillReachesTheLegacyEngine() throws IOException { + JSONObject response = + executeQuery( + "SELECT COUNT(*) FROM " + IDX + " GROUP BY histogram(field='value','interval'='20')"); + + verifyDataRows(response, rows(19), rows(20), rows(20), rows(13)); + } +} diff --git a/integ-test/src/test/resources/date_histogram_test.json b/integ-test/src/test/resources/date_histogram_test.json new file mode 100644 index 00000000000..a46919462e6 --- /dev/null +++ b/integ-test/src/test/resources/date_histogram_test.json @@ -0,0 +1,144 @@ +{"index":{"_id":"1"}} +{"ts":"2026-01-01 00:00:00","category":"alpha","value":1} +{"index":{"_id":"2"}} +{"ts":"2026-01-01 00:00:00","category":"alpha","value":2} +{"index":{"_id":"3"}} +{"ts":"2026-01-01 00:00:00","category":"alpha","value":3} +{"index":{"_id":"4"}} +{"ts":"2026-01-01 00:00:00","category":"alpha","value":4} +{"index":{"_id":"5"}} +{"ts":"2026-01-01 00:00:00","category":"alpha","value":5} +{"index":{"_id":"6"}} +{"ts":"2026-01-01 00:30:00","category":"beta","value":6} +{"index":{"_id":"7"}} +{"ts":"2026-01-01 00:30:00","category":"beta","value":7} +{"index":{"_id":"8"}} +{"ts":"2026-01-01 00:30:00","category":"beta","value":8} +{"index":{"_id":"9"}} +{"ts":"2026-01-01 00:30:00","category":"beta","value":9} +{"index":{"_id":"10"}} +{"ts":"2026-01-01 00:30:00","category":"beta","value":10} +{"index":{"_id":"11"}} +{"ts":"2026-01-01 00:30:00","category":"beta","value":11} +{"index":{"_id":"12"}} +{"ts":"2026-01-01 00:30:00","category":"beta","value":12} +{"index":{"_id":"13"}} +{"ts":"2026-01-01 01:00:00","category":"alpha","value":13} +{"index":{"_id":"14"}} +{"ts":"2026-01-01 01:00:00","category":"alpha","value":14} +{"index":{"_id":"15"}} +{"ts":"2026-01-01 01:00:00","category":"alpha","value":15} +{"index":{"_id":"16"}} +{"ts":"2026-01-01 01:00:00","category":"alpha","value":16} +{"index":{"_id":"17"}} +{"ts":"2026-01-01 01:00:00","category":"alpha","value":17} +{"index":{"_id":"18"}} +{"ts":"2026-01-01 01:00:00","category":"alpha","value":18} +{"index":{"_id":"19"}} +{"ts":"2026-01-01 01:00:00","category":"alpha","value":19} +{"index":{"_id":"20"}} +{"ts":"2026-01-01 01:00:00","category":"alpha","value":20} +{"index":{"_id":"21"}} +{"ts":"2026-01-01 01:00:00","category":"alpha","value":21} +{"index":{"_id":"22"}} +{"ts":"2026-01-01 01:00:00","category":"alpha","value":22} +{"index":{"_id":"23"}} +{"ts":"2026-01-01 01:00:00","category":"alpha","value":23} +{"index":{"_id":"24"}} +{"ts":"2026-01-01 01:45:00","category":"gamma","value":24} +{"index":{"_id":"25"}} +{"ts":"2026-01-01 01:45:00","category":"gamma","value":25} +{"index":{"_id":"26"}} +{"ts":"2026-01-01 01:45:00","category":"gamma","value":26} +{"index":{"_id":"27"}} +{"ts":"2026-01-01 01:45:00","category":"gamma","value":27} +{"index":{"_id":"28"}} +{"ts":"2026-01-01 01:45:00","category":"gamma","value":28} +{"index":{"_id":"29"}} +{"ts":"2026-01-01 01:45:00","category":"gamma","value":29} +{"index":{"_id":"30"}} +{"ts":"2026-01-01 01:45:00","category":"gamma","value":30} +{"index":{"_id":"31"}} +{"ts":"2026-01-01 01:45:00","category":"gamma","value":31} +{"index":{"_id":"32"}} +{"ts":"2026-01-01 01:45:00","category":"gamma","value":32} +{"index":{"_id":"33"}} +{"ts":"2026-01-01 01:45:00","category":"gamma","value":33} +{"index":{"_id":"34"}} +{"ts":"2026-01-01 01:45:00","category":"gamma","value":34} +{"index":{"_id":"35"}} +{"ts":"2026-01-01 01:45:00","category":"gamma","value":35} +{"index":{"_id":"36"}} +{"ts":"2026-01-01 01:45:00","category":"gamma","value":36} +{"index":{"_id":"37"}} +{"ts":"2026-01-01 02:00:00","category":"beta","value":37} +{"index":{"_id":"38"}} +{"ts":"2026-01-01 02:00:00","category":"beta","value":38} +{"index":{"_id":"39"}} +{"ts":"2026-01-01 02:00:00","category":"beta","value":39} +{"index":{"_id":"40"}} +{"ts":"2026-01-01 02:00:00","category":"beta","value":40} +{"index":{"_id":"41"}} +{"ts":"2026-01-01 02:00:00","category":"beta","value":41} +{"index":{"_id":"42"}} +{"ts":"2026-01-01 02:00:00","category":"beta","value":42} +{"index":{"_id":"43"}} +{"ts":"2026-01-01 02:00:00","category":"beta","value":43} +{"index":{"_id":"44"}} +{"ts":"2026-01-01 02:00:00","category":"beta","value":44} +{"index":{"_id":"45"}} +{"ts":"2026-01-01 02:00:00","category":"beta","value":45} +{"index":{"_id":"46"}} +{"ts":"2026-01-01 02:00:00","category":"beta","value":46} +{"index":{"_id":"47"}} +{"ts":"2026-01-01 02:00:00","category":"beta","value":47} +{"index":{"_id":"48"}} +{"ts":"2026-01-01 02:00:00","category":"beta","value":48} +{"index":{"_id":"49"}} +{"ts":"2026-01-01 02:00:00","category":"beta","value":49} +{"index":{"_id":"50"}} +{"ts":"2026-01-01 02:00:00","category":"beta","value":50} +{"index":{"_id":"51"}} +{"ts":"2026-01-01 02:00:00","category":"beta","value":51} +{"index":{"_id":"52"}} +{"ts":"2026-01-01 02:00:00","category":"beta","value":52} +{"index":{"_id":"53"}} +{"ts":"2026-01-01 02:00:00","category":"beta","value":53} +{"index":{"_id":"54"}} +{"ts":"2026-01-01 03:00:00","category":"alpha","value":54} +{"index":{"_id":"55"}} +{"ts":"2026-01-01 03:00:00","category":"alpha","value":55} +{"index":{"_id":"56"}} +{"ts":"2026-01-01 03:00:00","category":"alpha","value":56} +{"index":{"_id":"57"}} +{"ts":"2026-01-01 03:00:00","category":"alpha","value":57} +{"index":{"_id":"58"}} +{"ts":"2026-01-01 03:00:00","category":"alpha","value":58} +{"index":{"_id":"59"}} +{"ts":"2026-01-01 03:00:00","category":"alpha","value":59} +{"index":{"_id":"60"}} +{"ts":"2026-01-01 03:00:00","category":"alpha","value":60} +{"index":{"_id":"61"}} +{"ts":"2026-01-01 03:00:00","category":"alpha","value":61} +{"index":{"_id":"62"}} +{"ts":"2026-01-01 03:00:00","category":"alpha","value":62} +{"index":{"_id":"63"}} +{"ts":"2026-01-01 03:00:00","category":"alpha","value":63} +{"index":{"_id":"64"}} +{"ts":"2026-01-01 03:00:00","category":"alpha","value":64} +{"index":{"_id":"65"}} +{"ts":"2026-01-01 03:00:00","category":"alpha","value":65} +{"index":{"_id":"66"}} +{"ts":"2026-01-01 03:00:00","category":"alpha","value":66} +{"index":{"_id":"67"}} +{"ts":"2026-01-01 03:00:00","category":"alpha","value":67} +{"index":{"_id":"68"}} +{"ts":"2026-01-01 03:00:00","category":"alpha","value":68} +{"index":{"_id":"69"}} +{"ts":"2026-01-01 03:00:00","category":"alpha","value":69} +{"index":{"_id":"70"}} +{"ts":"2026-01-01 03:00:00","category":"alpha","value":70} +{"index":{"_id":"71"}} +{"ts":"2026-01-01 03:00:00","category":"alpha","value":71} +{"index":{"_id":"72"}} +{"ts":"2026-01-01 03:00:00","category":"alpha","value":72} diff --git a/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 b/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 index 5f7361160b3..4a2ab35a89b 100644 --- a/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 +++ b/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 @@ -411,6 +411,12 @@ scalarFunctionName | flowControlFunctionName | systemFunctionName | nestedFunctionName + | bucketFunctionName + ; + +bucketFunctionName + : HISTOGRAM + | DATE_HISTOGRAM ; specificFunction diff --git a/sql/src/main/antlr/OpenSearchSQLParser.g4 b/sql/src/main/antlr/OpenSearchSQLParser.g4 index 5b52b9d3387..5029f081b1d 100644 --- a/sql/src/main/antlr/OpenSearchSQLParser.g4 +++ b/sql/src/main/antlr/OpenSearchSQLParser.g4 @@ -444,6 +444,12 @@ scalarFunctionName | flowControlFunctionName | systemFunctionName | nestedFunctionName + | bucketFunctionName + ; + +bucketFunctionName + : HISTOGRAM + | DATE_HISTOGRAM ; specificFunction diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java index e7510f31b7a..823e5731a56 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java @@ -100,6 +100,8 @@ import org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.OrExpressionContext; import org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.TableNameContext; import org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParserBaseVisitor; +import org.opensearch.sql.sql.parser.bucket.BucketFunctionExpander; +import org.opensearch.sql.sql.parser.bucket.BucketFunctionRegistry; /** Expression builder to parse text to expression in AST. */ public class AstExpressionBuilder extends OpenSearchSQLParserBaseVisitor { @@ -162,7 +164,18 @@ public UnresolvedExpression visitNestedAllFunctionCall(NestedAllFunctionCallCont @Override public UnresolvedExpression visitScalarFunctionCall(ScalarFunctionCallContext ctx) { - return buildFunction(ctx.scalarFunctionName().getText(), ctx.functionArgs().functionArg()); + String functionName = ctx.scalarFunctionName().getText(); + List args = + ctx.functionArgs().functionArg().stream() + .map(this::visitFunctionArg) + .collect(Collectors.toList()); + + Optional bucketExpander = BucketFunctionRegistry.lookup(functionName); + if (bucketExpander.isPresent()) { + return bucketExpander.get().expand(args); + } + + return new Function(functionName, args); } @Override diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionExpander.java b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionExpander.java new file mode 100644 index 00000000000..d6d2dc2283d --- /dev/null +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionExpander.java @@ -0,0 +1,22 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.sql.parser.bucket; + +import java.util.List; +import org.opensearch.sql.ast.expression.UnresolvedExpression; + +/** + * Parse-time expander for a bucket function call. Each implementation lowers calls to one bucket + * function (e.g. {@code histogram}) into standard SQL constructs the rest of the engine already + * understands. + * + *

Implementations are stateless and registered by name in {@link BucketFunctionRegistry}. + */ +public interface BucketFunctionExpander { + + /** Lowers a bucket function call into its bucket-key expression. */ + UnresolvedExpression expand(List args); +} diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionRegistry.java b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionRegistry.java new file mode 100644 index 00000000000..e1471597689 --- /dev/null +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionRegistry.java @@ -0,0 +1,32 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.sql.parser.bucket; + +import java.util.Locale; +import java.util.Map; +import java.util.Optional; + +/** Lookup table mapping bucket-function names to their {@link BucketFunctionExpander}. */ +public final class BucketFunctionRegistry { + + private static final Map EXPANDERS = + Map.of( + HistogramExpander.FUNCTION_NAME, new HistogramExpander(), + DateHistogramExpander.FUNCTION_NAME, new DateHistogramExpander()); + + private BucketFunctionRegistry() {} + + /** + * Returns the expander for {@code functionName} (case-insensitive), or empty if not a bucket + * function. + */ + public static Optional lookup(String functionName) { + if (functionName == null) { + return Optional.empty(); + } + return Optional.ofNullable(EXPANDERS.get(functionName.toUpperCase(Locale.ROOT))); + } +} diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionUtils.java b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionUtils.java new file mode 100644 index 00000000000..850d7ba92be --- /dev/null +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionUtils.java @@ -0,0 +1,44 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.sql.parser.bucket; + +import java.util.List; +import org.opensearch.sql.ast.dsl.AstDSL; +import org.opensearch.sql.ast.expression.DataType; +import org.opensearch.sql.ast.expression.Function; +import org.opensearch.sql.ast.expression.Literal; +import org.opensearch.sql.ast.expression.QualifiedName; +import org.opensearch.sql.ast.expression.UnresolvedExpression; + +/** + * Shared parameter helpers for bucket-function expanders. Operates on values pulled from a {@link + * NamedArguments} or from a positional argument list. + */ +final class BucketFunctionUtils { + + private BucketFunctionUtils() {} + + /** + * Named-argument form accepts string-literal field names ({@code 'field'='age'}). Coerce them to + * {@link QualifiedName} so downstream sees a column reference regardless of how the user spelled + * it. + */ + static UnresolvedExpression normalizeFieldRef(UnresolvedExpression expr) { + if (expr instanceof Literal lit && lit.getType() == DataType.STRING) { + return AstDSL.qualifiedName(lit.getValue().toString()); + } + return expr; + } + + /** If {@code missingOrNull} is non-null, wrap field with {@code COALESCE(field, missing)}. */ + static UnresolvedExpression applyMissing( + UnresolvedExpression field, UnresolvedExpression missingOrNull) { + if (missingOrNull == null) { + return field; + } + return new Function("coalesce", List.of(field, missingOrNull)); + } +} diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpander.java b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpander.java new file mode 100644 index 00000000000..82be57dc9f3 --- /dev/null +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpander.java @@ -0,0 +1,135 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.sql.parser.bucket; + +import static org.opensearch.sql.sql.parser.bucket.BucketFunctionUtils.applyMissing; +import static org.opensearch.sql.sql.parser.bucket.BucketFunctionUtils.normalizeFieldRef; + +import java.time.ZoneOffset; +import java.util.List; +import java.util.Objects; +import java.util.stream.Stream; +import org.opensearch.sql.ast.dsl.AstDSL; +import org.opensearch.sql.ast.expression.Function; +import org.opensearch.sql.ast.expression.Literal; +import org.opensearch.sql.ast.expression.Span; +import org.opensearch.sql.ast.expression.UnresolvedExpression; +import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.exception.SemanticCheckException; + +/** + * Lowers {@code date_histogram(...)} calls to a {@link Span} expression with the time unit inferred + * from the interval string. Optional parameters wrap the bucket key: + * + *

    + *
  • {@code missing} — wraps the field with {@code COALESCE(field, missing)} before bucketing. + *
  • {@code time_zone} — shifts the field with {@code TIMESTAMPADD(SECOND, offset, field)} + * before bucketing. Validated as a {@link java.time.ZoneOffset} at parse time. + *
  • {@code format} — wraps the bucket with {@code DATE_FORMAT(span, format)}. + *
+ * + *

{@code interval}, {@code fixed_interval}, and {@code calendar_interval} are accepted as + * mutually-exclusive syntactic synonyms; this lowering does not preserve the calendar-vs-fixed + * distinction across them. + * + *

TODO: V1 also accepts the following parameters; they are currently rejected: + * + *

    + *
  • {@code min_doc_count} — would lower to {@code HAVING COUNT(*) >= N}. Needs parser-side + * plumbing to inject a HAVING clause from inside a scalar function call. + *
  • {@code order} — would lower to {@code ORDER BY}. Same plumbing requirement as above. + *
  • {@code alias} — would set the surrounding SELECT-list alias. Needs reaching outside the + * function call to mutate the parent SELECT element. + *
  • {@code offset} — would shift bucket boundaries via {@code TIMESTAMPADD(SECOND, -offset, + * field)} before bucketing and {@code TIMESTAMPADD(SECOND, offset, span)} after. Needs a + * duration-string parser ({@code '1h'}, {@code '2d'}, etc.) distinct from {@code time_zone}'s + * {@code ZoneOffset} format. + *
+ */ +final class DateHistogramExpander implements BucketFunctionExpander { + + static final String FUNCTION_NAME = "DATE_HISTOGRAM"; + + @Override + public UnresolvedExpression expand(List args) { + if (!NamedArguments.isNamedArguments(args)) { + // SyntaxCheckException is the only type RestSQLQueryAction falls back on, so an + // unrecognized shape keeps reaching the legacy engine that has always served it. + throw new SyntaxCheckException( + "date_histogram requires named arguments: date_histogram('field'=," + + " 'interval'=)"); + } + NamedArguments named = NamedArguments.parse(args); + UnresolvedExpression field = named.require("field", FUNCTION_NAME); + Literal intervalLiteral = extractIntervalLiteral(named); + Literal formatLiteral = named.requireStringIfPresent("format"); + Literal timeZoneLiteral = named.requireStringIfPresent("time_zone"); + UnresolvedExpression missing = named.remove("missing"); + named.rejectRemaining(FUNCTION_NAME); + return buildBucket(field, intervalLiteral, formatLiteral, timeZoneLiteral, missing); + } + + /** + * Pulls the interval from the named arguments accepting any of {@code interval}, {@code + * fixed_interval}, {@code calendar_interval}. Exactly one must be present. + */ + private static Literal extractIntervalLiteral(NamedArguments named) { + Literal interval = named.requireStringIfPresent("interval"); + Literal fixedInterval = named.requireStringIfPresent("fixed_interval"); + Literal calendarInterval = named.requireStringIfPresent("calendar_interval"); + + List suppliedIntervals = + Stream.of(interval, fixedInterval, calendarInterval).filter(Objects::nonNull).toList(); + + if (suppliedIntervals.isEmpty()) { + throw new SemanticCheckException( + "date_histogram requires one of: interval, fixed_interval, calendar_interval"); + } + if (suppliedIntervals.size() > 1) { + throw new SemanticCheckException( + "date_histogram accepts only one of: interval, fixed_interval, calendar_interval"); + } + return suppliedIntervals.get(0); + } + + private static UnresolvedExpression buildBucket( + UnresolvedExpression field, + Literal intervalLiteral, + Literal formatLiteral, + Literal timeZoneLiteral, + UnresolvedExpression missingOrNull) { + UnresolvedExpression resolvedField = applyMissing(normalizeFieldRef(field), missingOrNull); + UnresolvedExpression shiftedField = + timeZoneLiteral != null + ? applyTimeZoneShift(resolvedField, timeZoneLiteral) + : resolvedField; + Span span = AstDSL.spanFromSpanLengthLiteral(shiftedField, intervalLiteral); + if (formatLiteral == null) { + return span; + } + return new Function("date_format", List.of(span, formatLiteral)); + } + + /** + * Wraps the field with a {@code TIMESTAMPADD(SECOND, offset, field)} shift derived from a + * timezone literal. Validates the literal at parse time as a {@link ZoneOffset} (e.g. {@code + * '+05:30'}, {@code 'Z'}); runtime arithmetic is plain second addition. + */ + private static UnresolvedExpression applyTimeZoneShift( + UnresolvedExpression field, Literal timeZoneLiteral) { + String tzString = timeZoneLiteral.getValue().toString(); + int offsetSeconds; + try { + offsetSeconds = ZoneOffset.of(tzString).getTotalSeconds(); + } catch (RuntimeException ex) { + throw new SemanticCheckException( + "time_zone must be a valid offset like '+05:30' or 'Z'; got '" + tzString + "'"); + } + return new Function( + "timestampadd", + List.of(AstDSL.stringLiteral("SECOND"), AstDSL.intLiteral(offsetSeconds), field)); + } +} diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/HistogramExpander.java b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/HistogramExpander.java new file mode 100644 index 00000000000..a5b1a6a71ca --- /dev/null +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/HistogramExpander.java @@ -0,0 +1,72 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.sql.parser.bucket; + +import static org.opensearch.sql.sql.parser.bucket.BucketFunctionUtils.applyMissing; +import static org.opensearch.sql.sql.parser.bucket.BucketFunctionUtils.normalizeFieldRef; + +import java.util.List; +import org.opensearch.sql.ast.dsl.AstDSL; +import org.opensearch.sql.ast.expression.Function; +import org.opensearch.sql.ast.expression.Span; +import org.opensearch.sql.ast.expression.SpanUnit; +import org.opensearch.sql.ast.expression.UnresolvedExpression; +import org.opensearch.sql.common.antlr.SyntaxCheckException; + +/** + * Lowers {@code histogram(...)} calls to a {@link Span} expression with {@code SpanUnit.NONE}. + * Optional parameters wrap the bucket key: + * + *
    + *
  • {@code missing} — wraps the field with {@code COALESCE(field, missing)} before bucketing. + *
  • {@code offset} — wraps as {@code +(Span(-(field, offset), interval, NONE), offset)} to + * preserve the standard {@code [k*interval+offset, (k+1)*interval+offset)} boundaries. + *
+ * + *

TODO: V1 also accepts the following parameters; they are currently rejected: + * + *

    + *
  • {@code min_doc_count} — would lower to {@code HAVING COUNT(*) >= N}. Needs parser-side + * plumbing to inject a HAVING clause from inside a scalar function call. + *
  • {@code order} — would lower to {@code ORDER BY}. Same plumbing requirement as above. + *
  • {@code alias} — would set the surrounding SELECT-list alias. Needs reaching outside the + * function call to mutate the parent SELECT element. + *
+ */ +final class HistogramExpander implements BucketFunctionExpander { + + static final String FUNCTION_NAME = "HISTOGRAM"; + + @Override + public UnresolvedExpression expand(List args) { + if (!NamedArguments.isNamedArguments(args)) { + // See DateHistogramExpander: this type is what allows the legacy fallback. + throw new SyntaxCheckException( + "histogram requires named arguments: histogram('field'=, 'interval'=)"); + } + NamedArguments named = NamedArguments.parse(args); + UnresolvedExpression field = named.require("field", FUNCTION_NAME); + UnresolvedExpression interval = named.require("interval", FUNCTION_NAME); + UnresolvedExpression offset = named.remove("offset"); + UnresolvedExpression missing = named.remove("missing"); + named.rejectRemaining(FUNCTION_NAME); + return buildBucket(field, interval, offset, missing); + } + + private static UnresolvedExpression buildBucket( + UnresolvedExpression field, + UnresolvedExpression interval, + UnresolvedExpression offsetOrNull, + UnresolvedExpression missingOrNull) { + UnresolvedExpression resolvedField = applyMissing(normalizeFieldRef(field), missingOrNull); + if (offsetOrNull == null) { + return AstDSL.span(resolvedField, interval, SpanUnit.NONE); + } + UnresolvedExpression shifted = new Function("-", List.of(resolvedField, offsetOrNull)); + Span bucket = (Span) AstDSL.span(shifted, interval, SpanUnit.NONE); + return new Function("+", List.of(bucket, offsetOrNull)); + } +} diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/NamedArguments.java b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/NamedArguments.java new file mode 100644 index 00000000000..5a1ced9a949 --- /dev/null +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/NamedArguments.java @@ -0,0 +1,142 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.sql.parser.bucket; + +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.Set; +import org.opensearch.sql.ast.expression.DataType; +import org.opensearch.sql.ast.expression.Function; +import org.opensearch.sql.ast.expression.Literal; +import org.opensearch.sql.ast.expression.UnresolvedExpression; +import org.opensearch.sql.exception.SemanticCheckException; + +/** + * Parses and validates named-argument style function arguments. The arg shape is {@code + * Function("=", [StringLiteral(key), value])} — what ANTLR produces for {@code 'key'=value}. Keys + * are lower-cased on parse; iteration order matches source order. + * + *

Drain semantics. Every extraction method ({@code require}, {@code remove}, {@code + * requireString}, {@code requireStringIfPresent}, {@code rejectIfPresent}, {@code consumeSilently}) + * removes its key from the collection. After the caller has extracted everything it recognizes, + * {@code rejectRemaining} sweeps what is left and treats those keys as unknown parameters — so + * extracted keys must drain out, otherwise they would be re-rejected. + */ +public final class NamedArguments { + + private final Map arguments; + + private NamedArguments(Map arguments) { + this.arguments = arguments; + } + + /** True iff every arg is a {@code 'key'=value} key-value pair. Empty list returns false. */ + public static boolean isNamedArguments(List args) { + if (args.isEmpty()) { + return false; + } + return args.stream().allMatch(NamedArguments::isKeyValuePair); + } + + private static boolean isKeyValuePair(UnresolvedExpression arg) { + if (!(arg instanceof Function fn) || !"=".equals(fn.getFuncName())) { + return false; + } + if (fn.getFuncArgs().size() != 2) { + return false; + } + return fn.getFuncArgs().get(0) instanceof Literal keyLiteral + && keyLiteral.getType() == DataType.STRING; + } + + /** + * Parses the given args into a {@code NamedArguments}. Each arg must match the {@code + * 'key'=value} shape — a non-matching arg raises {@link SemanticCheckException}. Duplicate keys + * also raise {@link SemanticCheckException}. + */ + public static NamedArguments parse(List args) { + Map arguments = new LinkedHashMap<>(); + for (UnresolvedExpression arg : args) { + if (!isKeyValuePair(arg)) { + throw new SemanticCheckException("Named arguments must be of form 'key'=value; got " + arg); + } + Function fn = (Function) arg; + Literal keyLiteral = (Literal) fn.getFuncArgs().get(0); + String key = keyLiteral.getValue().toString().toLowerCase(Locale.ROOT); + UnresolvedExpression value = fn.getFuncArgs().get(1); + if (arguments.put(key, value) != null) { + throw new SemanticCheckException("Duplicate parameter: " + key); + } + } + return new NamedArguments(arguments); + } + + /** Removes and returns the value for {@code key}, or {@code null} if not present. */ + public UnresolvedExpression remove(String key) { + return arguments.remove(key); + } + + /** Removes and returns the value for {@code key}; throws if absent. */ + public UnresolvedExpression require(String key, String funcName) { + UnresolvedExpression value = arguments.remove(key); + if (value == null) { + throw new SemanticCheckException( + funcName.toLowerCase(Locale.ROOT) + " requires " + key + " parameter"); + } + return value; + } + + /** As {@link #require}, additionally enforcing string-literal type. */ + public Literal requireString(String key, String funcName) { + return asStringLiteral(require(key, funcName), key); + } + + /** As {@link #remove}, additionally enforcing string-literal type when present. */ + public Literal requireStringIfPresent(String key) { + UnresolvedExpression value = arguments.remove(key); + return value == null ? null : asStringLiteral(value, key); + } + + private static Literal asStringLiteral(UnresolvedExpression expr, String paramName) { + if (!(expr instanceof Literal literal) || literal.getType() != DataType.STRING) { + throw new SemanticCheckException( + paramName + " must be a string literal (e.g. '1d', '15m'); got " + expr); + } + return literal; + } + + /** If {@code key} is present, throws with the supplied message; otherwise no-op. */ + public void rejectIfPresent(String key, String message) { + if (arguments.remove(key) != null) { + throw new SemanticCheckException(message); + } + } + + /** Drops the listed keys without inspecting their values. */ + public void consumeSilently(Set keys) { + for (String key : keys) { + arguments.remove(key); + } + } + + /** Treats any keys still remaining as unsupported parameters. Call last. */ + public void rejectRemaining(String funcName) { + if (arguments.isEmpty()) { + return; + } + String label = arguments.size() == 1 ? "parameter" : "parameters"; + String unsupported = String.join(", ", arguments.keySet()); + throw new SemanticCheckException( + funcName.toLowerCase(Locale.ROOT) + " does not accept " + label + ": " + unsupported); + } + + /** Number of unconsumed keys. Primarily for tests. */ + int size() { + return arguments.size(); + } +} diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionRegistryTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionRegistryTest.java new file mode 100644 index 00000000000..9dc5acf2572 --- /dev/null +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionRegistryTest.java @@ -0,0 +1,53 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.sql.parser.bucket; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.Optional; +import org.junit.jupiter.api.DisplayNameGeneration; +import org.junit.jupiter.api.DisplayNameGenerator; +import org.junit.jupiter.api.Test; + +@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) +class BucketFunctionRegistryTest { + + @Test + void lookup_returns_HistogramExpander_for_HISTOGRAM() { + Optional expander = BucketFunctionRegistry.lookup("HISTOGRAM"); + assertTrue(expander.isPresent()); + assertInstanceOf(HistogramExpander.class, expander.get()); + } + + @Test + void lookup_returns_DateHistogramExpander_for_DATE_HISTOGRAM() { + Optional expander = BucketFunctionRegistry.lookup("DATE_HISTOGRAM"); + assertTrue(expander.isPresent()); + assertInstanceOf(DateHistogramExpander.class, expander.get()); + } + + @Test + void lookup_is_case_insensitive() { + assertTrue(BucketFunctionRegistry.lookup("histogram").isPresent()); + assertTrue(BucketFunctionRegistry.lookup("Histogram").isPresent()); + assertTrue(BucketFunctionRegistry.lookup("date_histogram").isPresent()); + assertTrue(BucketFunctionRegistry.lookup("Date_Histogram").isPresent()); + } + + @Test + void lookup_returns_empty_for_unknown_function() { + assertFalse(BucketFunctionRegistry.lookup("range").isPresent()); + assertFalse(BucketFunctionRegistry.lookup("SUM").isPresent()); + assertFalse(BucketFunctionRegistry.lookup("FLOOR").isPresent()); + } + + @Test + void lookup_returns_empty_for_null() { + assertFalse(BucketFunctionRegistry.lookup(null).isPresent()); + } +} diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionUtilsTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionUtilsTest.java new file mode 100644 index 00000000000..b211a6362ad --- /dev/null +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionUtilsTest.java @@ -0,0 +1,56 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.sql.parser.bucket; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertSame; + +import java.util.List; +import org.junit.jupiter.api.DisplayNameGeneration; +import org.junit.jupiter.api.DisplayNameGenerator; +import org.junit.jupiter.api.Test; +import org.opensearch.sql.ast.dsl.AstDSL; +import org.opensearch.sql.ast.expression.Function; +import org.opensearch.sql.ast.expression.QualifiedName; +import org.opensearch.sql.ast.expression.UnresolvedExpression; + +@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) +class BucketFunctionUtilsTest { + + @Test + void normalizeFieldRef_string_literal_becomes_qualified_name() { + UnresolvedExpression result = + BucketFunctionUtils.normalizeFieldRef(AstDSL.stringLiteral("age")); + assertEquals(AstDSL.qualifiedName("age"), result); + } + + @Test + void normalizeFieldRef_qualified_name_passes_through_unchanged() { + QualifiedName input = AstDSL.qualifiedName("age"); + assertSame(input, BucketFunctionUtils.normalizeFieldRef(input)); + } + + @Test + void normalizeFieldRef_non_string_literal_passes_through_unchanged() { + UnresolvedExpression input = AstDSL.intLiteral(1); + assertSame(input, BucketFunctionUtils.normalizeFieldRef(input)); + } + + @Test + void applyMissing_null_returns_field_unchanged() { + QualifiedName field = AstDSL.qualifiedName("age"); + assertSame(field, BucketFunctionUtils.applyMissing(field, null)); + } + + @Test + void applyMissing_non_null_wraps_with_coalesce() { + QualifiedName field = AstDSL.qualifiedName("age"); + UnresolvedExpression missing = AstDSL.intLiteral(0); + assertEquals( + new Function("coalesce", List.of(field, missing)), + BucketFunctionUtils.applyMissing(field, missing)); + } +} diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpanderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpanderTest.java new file mode 100644 index 00000000000..f1de66628c0 --- /dev/null +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpanderTest.java @@ -0,0 +1,367 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.sql.parser.bucket; + +import static java.util.Collections.emptyList; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.google.common.collect.ImmutableList; +import java.util.List; +import org.junit.jupiter.api.DisplayNameGeneration; +import org.junit.jupiter.api.DisplayNameGenerator; +import org.junit.jupiter.api.Test; +import org.opensearch.sql.ast.dsl.AstDSL; +import org.opensearch.sql.ast.expression.AllFields; +import org.opensearch.sql.ast.expression.Function; +import org.opensearch.sql.ast.expression.QualifiedName; +import org.opensearch.sql.ast.expression.Span; +import org.opensearch.sql.ast.expression.SpanUnit; +import org.opensearch.sql.ast.expression.UnresolvedExpression; +import org.opensearch.sql.ast.tree.UnresolvedPlan; +import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.exception.SemanticCheckException; +import org.opensearch.sql.sql.parser.AstBuilderTestBase; + +@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) +class DateHistogramExpanderTest extends AstBuilderTestBase { + + private final DateHistogramExpander expander = new DateHistogramExpander(); + + @Test + void rejects_positional_invocation_with_clear_message() { + SyntaxCheckException ex = + assertThrows( + SyntaxCheckException.class, + () -> expander.expand(List.of(AstDSL.qualifiedName("ts"), AstDSL.stringLiteral("1d")))); + assertTrue(ex.getMessage().contains("named arguments")); + assertTrue(ex.getMessage().contains("date_histogram")); + } + + @Test + void property_bag_with_interval_param_lowers_to_span() { + UnresolvedExpression result = + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("ts")), + kv("interval", AstDSL.stringLiteral("1d")))); + + assertEquals(new Span(AstDSL.qualifiedName("ts"), AstDSL.intLiteral(1), SpanUnit.D), result); + } + + @Test + void property_bag_with_qualified_name_field_passes_through_unchanged() { + QualifiedName ts = AstDSL.qualifiedName("ts"); + UnresolvedExpression result = + expander.expand(List.of(kv("field", ts), kv("interval", AstDSL.stringLiteral("1d")))); + + assertEquals(new Span(ts, AstDSL.intLiteral(1), SpanUnit.D), result); + } + + @Test + void property_bag_with_fixed_interval_param_lowers_to_span() { + UnresolvedExpression result = + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("ts")), + kv("fixed_interval", AstDSL.stringLiteral("15m")))); + + assertEquals(new Span(AstDSL.qualifiedName("ts"), AstDSL.intLiteral(15), SpanUnit.m), result); + } + + @Test + void property_bag_with_calendar_interval_param_lowers_to_span() { + UnresolvedExpression result = + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("ts")), + kv("calendar_interval", AstDSL.stringLiteral("1d")))); + + assertEquals(new Span(AstDSL.qualifiedName("ts"), AstDSL.intLiteral(1), SpanUnit.D), result); + } + + /** + * The two types differ on purpose: an unrecognized shape falls back to the legacy engine, a bad + * parameter does not. Collapsing them would drop a working feature silently. + */ + @Test + void separates_an_unrecognized_call_shape_from_bad_parameters() { + assertThrows( + SyntaxCheckException.class, + () -> expander.expand(List.of(AstDSL.qualifiedName("ts"), AstDSL.stringLiteral("1d")))); + + assertThrows( + SemanticCheckException.class, + () -> expander.expand(List.of(kv("field", AstDSL.qualifiedName("ts"))))); + } + + @Test + void property_bag_rejects_both_interval_and_fixed_interval() { + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("ts")), + kv("interval", AstDSL.stringLiteral("1d")), + kv("fixed_interval", AstDSL.stringLiteral("15m"))))); + } + + @Test + void property_bag_format_wraps_with_date_format() { + UnresolvedExpression result = + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("ts")), + kv("interval", AstDSL.stringLiteral("1d")), + kv("format", AstDSL.stringLiteral("yyyy-MM-dd")))); + + Span innerSpan = new Span(AstDSL.qualifiedName("ts"), AstDSL.intLiteral(1), SpanUnit.D); + Function expected = + new Function("date_format", List.of(innerSpan, AstDSL.stringLiteral("yyyy-MM-dd"))); + assertEquals(expected, result); + } + + @Test + void property_bag_time_zone_wraps_field_with_timestampadd() { + UnresolvedExpression result = + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("ts")), + kv("interval", AstDSL.stringLiteral("1d")), + kv("time_zone", AstDSL.stringLiteral("+05:30")))); + + // +05:30 = 5*3600 + 30*60 = 19800 seconds + Function shiftedField = + new Function( + "timestampadd", + List.of( + AstDSL.stringLiteral("SECOND"), + AstDSL.intLiteral(19800), + AstDSL.qualifiedName("ts"))); + Span expected = new Span(shiftedField, AstDSL.intLiteral(1), SpanUnit.D); + assertEquals(expected, result); + } + + @Test + void property_bag_format_and_time_zone_compose() { + UnresolvedExpression result = + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("ts")), + kv("interval", AstDSL.stringLiteral("1d")), + kv("format", AstDSL.stringLiteral("yyyy")), + kv("time_zone", AstDSL.stringLiteral("Z")))); + + // Z = 0 offset + Function shiftedField = + new Function( + "timestampadd", + List.of( + AstDSL.stringLiteral("SECOND"), AstDSL.intLiteral(0), AstDSL.qualifiedName("ts"))); + Span innerSpan = new Span(shiftedField, AstDSL.intLiteral(1), SpanUnit.D); + Function expected = + new Function("date_format", List.of(innerSpan, AstDSL.stringLiteral("yyyy"))); + assertEquals(expected, result); + } + + @Test + void property_bag_rejects_invalid_time_zone() { + SemanticCheckException ex = + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("ts")), + kv("interval", AstDSL.stringLiteral("1d")), + kv("time_zone", AstDSL.stringLiteral("not-a-tz"))))); + assertTrue(ex.getMessage().contains("time_zone")); + } + + @Test + void property_bag_rejects_alias() { + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("ts")), + kv("interval", AstDSL.stringLiteral("1d")), + kv("alias", AstDSL.stringLiteral("my_label"))))); + } + + @Test + void property_bag_rejects_nested() { + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("ts")), + kv("interval", AstDSL.stringLiteral("1d")), + kv("nested", AstDSL.stringLiteral("path"))))); + } + + @Test + void property_bag_rejects_reverse_nested() { + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("ts")), + kv("interval", AstDSL.stringLiteral("1d")), + kv("reverse_nested", AstDSL.stringLiteral("path"))))); + } + + @Test + void property_bag_rejects_children() { + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("ts")), + kv("interval", AstDSL.stringLiteral("1d")), + kv("children", AstDSL.stringLiteral("ignored"))))); + } + + @Test + void property_bag_missing_wraps_field_with_coalesce() { + UnresolvedExpression result = + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("ts")), + kv("interval", AstDSL.stringLiteral("1d")), + kv("missing", AstDSL.stringLiteral("2024-01-01")))); + + Function coalesced = + new Function( + "coalesce", List.of(AstDSL.qualifiedName("ts"), AstDSL.stringLiteral("2024-01-01"))); + assertEquals(new Span(coalesced, AstDSL.intLiteral(1), SpanUnit.D), result); + } + + @Test + void property_bag_rejects_offset() { + SemanticCheckException ex = + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("ts")), + kv("interval", AstDSL.stringLiteral("1d")), + kv("offset", AstDSL.stringLiteral("1h"))))); + assertTrue(ex.getMessage().contains("offset")); + assertTrue(ex.getMessage().contains("does not accept")); + } + + @Test + void property_bag_rejects_min_doc_count() { + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("ts")), + kv("interval", AstDSL.stringLiteral("1d")), + kv("min_doc_count", AstDSL.intLiteral(5))))); + } + + @Test + void property_bag_rejects_extended_bounds() { + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("ts")), + kv("interval", AstDSL.stringLiteral("1d")), + kv("extended_bounds", AstDSL.stringLiteral("a:b"))))); + } + + @Test + void property_bag_rejects_unknown_param() { + SemanticCheckException ex = + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("ts")), + kv("interval", AstDSL.stringLiteral("1d")), + kv("missing_param", AstDSL.stringLiteral("foo"))))); + assertTrue(ex.getMessage().contains("missing_param")); + } + + @Test + void property_bag_rejects_duplicate_keys() { + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("ts")), + kv("field", AstDSL.stringLiteral("created_at")), + kv("interval", AstDSL.stringLiteral("1d"))))); + } + + @Test + void property_bag_rejects_missing_field() { + assertThrows( + SemanticCheckException.class, + () -> expander.expand(List.of(kv("interval", AstDSL.stringLiteral("1d"))))); + } + + @Test + void property_bag_rejects_when_no_interval_synonym_provided() { + SemanticCheckException ex = + assertThrows( + SemanticCheckException.class, + () -> expander.expand(List.of(kv("field", AstDSL.stringLiteral("ts"))))); + assertTrue(ex.getMessage().contains("requires one of")); + } + + @Test + void via_sql_with_interval_param_lowers_to_span() { + QualifiedName ts = AstDSL.qualifiedName("ts"); + Span bucket = AstDSL.span(ts, AstDSL.intLiteral(1), SpanUnit.D); + + UnresolvedPlan result = + buildAST( + "SELECT date_histogram('field'='ts', 'interval'='1d'), COUNT(*) FROM events " + + "GROUP BY date_histogram('field'='ts', 'interval'='1d')"); + + assertEquals( + AstDSL.project( + AstDSL.agg( + AstDSL.relation("events"), + ImmutableList.of( + AstDSL.alias("COUNT(*)", AstDSL.aggregate("COUNT", AllFields.of()))), + emptyList(), + ImmutableList.of(AstDSL.alias(bucket.toString(), bucket)), + emptyList()), + AstDSL.alias("date_histogram('field'='ts', 'interval'='1d')", bucket), + AstDSL.alias("COUNT(*)", AstDSL.aggregate("COUNT", AllFields.of()))), + result); + } + + @Test + void via_sql_rejects_positional_invocation() { + assertThrows( + SyntaxCheckException.class, + () -> + buildAST( + "SELECT date_histogram(ts, '1d') FROM events GROUP BY date_histogram(ts, '1d')")); + } + + /** Builds a Function("=", [stringLiteral(key), value]) — same shape ANTLR produces for 'k'=v. */ + private static UnresolvedExpression kv(String key, UnresolvedExpression value) { + return new Function("=", List.of(AstDSL.stringLiteral(key), value)); + } +} diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/HistogramExpanderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/HistogramExpanderTest.java new file mode 100644 index 00000000000..67c801e3392 --- /dev/null +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/HistogramExpanderTest.java @@ -0,0 +1,296 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.sql.parser.bucket; + +import static java.util.Collections.emptyList; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.google.common.collect.ImmutableList; +import java.util.List; +import org.junit.jupiter.api.DisplayNameGeneration; +import org.junit.jupiter.api.DisplayNameGenerator; +import org.junit.jupiter.api.Test; +import org.opensearch.sql.ast.dsl.AstDSL; +import org.opensearch.sql.ast.expression.AllFields; +import org.opensearch.sql.ast.expression.Function; +import org.opensearch.sql.ast.expression.QualifiedName; +import org.opensearch.sql.ast.expression.Span; +import org.opensearch.sql.ast.expression.SpanUnit; +import org.opensearch.sql.ast.expression.UnresolvedExpression; +import org.opensearch.sql.ast.tree.UnresolvedPlan; +import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.exception.SemanticCheckException; +import org.opensearch.sql.sql.parser.AstBuilderTestBase; + +@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) +class HistogramExpanderTest extends AstBuilderTestBase { + + private final HistogramExpander expander = new HistogramExpander(); + + @Test + void rejects_positional_invocation_with_clear_message() { + SyntaxCheckException ex = + assertThrows( + SyntaxCheckException.class, + () -> expander.expand(List.of(AstDSL.qualifiedName("price"), AstDSL.intLiteral(100)))); + assertTrue(ex.getMessage().contains("named arguments")); + assertTrue(ex.getMessage().contains("histogram")); + } + + @Test + void property_bag_with_string_field_coerces_to_qualified_name() { + UnresolvedExpression result = + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("age")), kv("interval", AstDSL.intLiteral(10)))); + + assertEquals( + new Span(AstDSL.qualifiedName("age"), AstDSL.intLiteral(10), SpanUnit.NONE), result); + } + + @Test + void property_bag_with_qualified_name_field_passes_through_unchanged() { + QualifiedName age = AstDSL.qualifiedName("age"); + UnresolvedExpression result = + expander.expand(List.of(kv("field", age), kv("interval", AstDSL.intLiteral(10)))); + + assertEquals(new Span(age, AstDSL.intLiteral(10), SpanUnit.NONE), result); + } + + @Test + void property_bag_rejects_alias() { + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("age")), + kv("interval", AstDSL.intLiteral(10)), + kv("alias", AstDSL.stringLiteral("my_label"))))); + } + + @Test + void property_bag_rejects_nested() { + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("age")), + kv("interval", AstDSL.intLiteral(10)), + kv("nested", AstDSL.stringLiteral("path"))))); + } + + @Test + void property_bag_rejects_reverse_nested() { + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("age")), + kv("interval", AstDSL.intLiteral(10)), + kv("reverse_nested", AstDSL.stringLiteral("path"))))); + } + + @Test + void property_bag_rejects_children() { + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("age")), + kv("interval", AstDSL.intLiteral(10)), + kv("children", AstDSL.stringLiteral("ignored"))))); + } + + @Test + void property_bag_rejects_format() { + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("age")), + kv("interval", AstDSL.intLiteral(10)), + kv("format", AstDSL.stringLiteral("yyyy"))))); + } + + @Test + void property_bag_rejects_time_zone() { + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("age")), + kv("interval", AstDSL.intLiteral(10)), + kv("time_zone", AstDSL.stringLiteral("+05:30"))))); + } + + @Test + void property_bag_rejects_min_doc_count() { + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("age")), + kv("interval", AstDSL.intLiteral(10)), + kv("min_doc_count", AstDSL.intLiteral(5))))); + } + + @Test + void property_bag_rejects_order() { + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("age")), + kv("interval", AstDSL.intLiteral(10)), + kv("order", AstDSL.stringLiteral("count_desc"))))); + } + + @Test + void property_bag_rejects_extended_bounds() { + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("age")), + kv("interval", AstDSL.intLiteral(10)), + kv("extended_bounds", AstDSL.stringLiteral("0:100"))))); + } + + @Test + void property_bag_rejects_unknown_param() { + SemanticCheckException ex = + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("age")), + kv("interval", AstDSL.intLiteral(10)), + kv("missing_param", AstDSL.stringLiteral("foo"))))); + assertTrue(ex.getMessage().contains("missing_param")); + } + + @Test + void property_bag_rejects_duplicate_keys() { + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("age")), + kv("field", AstDSL.stringLiteral("size")), + kv("interval", AstDSL.intLiteral(10))))); + } + + @Test + void property_bag_rejects_missing_field() { + assertThrows( + SemanticCheckException.class, + () -> expander.expand(List.of(kv("interval", AstDSL.intLiteral(10))))); + } + + @Test + void property_bag_rejects_missing_interval() { + assertThrows( + SemanticCheckException.class, + () -> expander.expand(List.of(kv("field", AstDSL.stringLiteral("age"))))); + } + + @Test + void property_bag_offset_shifts_bucket_boundaries() { + UnresolvedExpression result = + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("age")), + kv("interval", AstDSL.intLiteral(10)), + kv("offset", AstDSL.intLiteral(3)))); + + QualifiedName age = AstDSL.qualifiedName("age"); + Function shiftedField = new Function("-", List.of(age, AstDSL.intLiteral(3))); + Span bucket = new Span(shiftedField, AstDSL.intLiteral(10), SpanUnit.NONE); + Function expected = new Function("+", List.of(bucket, AstDSL.intLiteral(3))); + assertEquals(expected, result); + } + + @Test + void property_bag_missing_wraps_field_with_coalesce() { + UnresolvedExpression result = + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("age")), + kv("interval", AstDSL.intLiteral(10)), + kv("missing", AstDSL.intLiteral(0)))); + + Function coalesced = + new Function("coalesce", List.of(AstDSL.qualifiedName("age"), AstDSL.intLiteral(0))); + assertEquals(new Span(coalesced, AstDSL.intLiteral(10), SpanUnit.NONE), result); + } + + @Test + void property_bag_offset_and_missing_compose_in_correct_order() { + UnresolvedExpression result = + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("age")), + kv("interval", AstDSL.intLiteral(10)), + kv("offset", AstDSL.intLiteral(3)), + kv("missing", AstDSL.intLiteral(0)))); + + QualifiedName age = AstDSL.qualifiedName("age"); + Function coalesced = new Function("coalesce", List.of(age, AstDSL.intLiteral(0))); + Function shifted = new Function("-", List.of(coalesced, AstDSL.intLiteral(3))); + Span bucket = new Span(shifted, AstDSL.intLiteral(10), SpanUnit.NONE); + Function expected = new Function("+", List.of(bucket, AstDSL.intLiteral(3))); + assertEquals(expected, result); + } + + @Test + void via_sql_lowers_to_span() { + QualifiedName age = AstDSL.qualifiedName("age"); + Span bucket = AstDSL.span(age, AstDSL.intLiteral(10), SpanUnit.NONE); + + UnresolvedPlan result = + buildAST( + "SELECT histogram('field'='age', 'interval'=10), COUNT(*) FROM accounts " + + "GROUP BY histogram('field'='age', 'interval'=10)"); + + assertEquals( + AstDSL.project( + AstDSL.agg( + AstDSL.relation("accounts"), + ImmutableList.of( + AstDSL.alias("COUNT(*)", AstDSL.aggregate("COUNT", AllFields.of()))), + emptyList(), + ImmutableList.of(AstDSL.alias(bucket.toString(), bucket)), + emptyList()), + AstDSL.alias("histogram('field'='age', 'interval'=10)", bucket), + AstDSL.alias("COUNT(*)", AstDSL.aggregate("COUNT", AllFields.of()))), + result); + } + + @Test + void via_sql_rejects_positional_invocation() { + assertThrows( + SyntaxCheckException.class, + () -> buildAST("SELECT histogram(price, 100) FROM orders GROUP BY histogram(price, 100)")); + } + + /** Builds a Function("=", [stringLiteral(key), value]) — same shape ANTLR produces for 'k'=v. */ + private static UnresolvedExpression kv(String key, UnresolvedExpression value) { + return new Function("=", List.of(AstDSL.stringLiteral(key), value)); + } +} diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/NamedArgumentsTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/NamedArgumentsTest.java new file mode 100644 index 00000000000..267ddc06165 --- /dev/null +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/NamedArgumentsTest.java @@ -0,0 +1,249 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.sql.parser.bucket; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.List; +import java.util.Set; +import org.junit.jupiter.api.DisplayNameGeneration; +import org.junit.jupiter.api.DisplayNameGenerator; +import org.junit.jupiter.api.Test; +import org.opensearch.sql.ast.dsl.AstDSL; +import org.opensearch.sql.ast.expression.Function; +import org.opensearch.sql.ast.expression.Literal; +import org.opensearch.sql.ast.expression.UnresolvedExpression; +import org.opensearch.sql.exception.SemanticCheckException; + +@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) +class NamedArgumentsTest { + + @Test + void empty_arg_list_is_not_named_arguments() { + assertFalse(NamedArguments.isNamedArguments(List.of())); + } + + @Test + void single_kv_pair_is_named_arguments() { + assertTrue(NamedArguments.isNamedArguments(List.of(kv("k", AstDSL.intLiteral(1))))); + } + + @Test + void plain_function_call_is_not_named_arguments() { + UnresolvedExpression nonKv = AstDSL.qualifiedName("col"); + assertFalse(NamedArguments.isNamedArguments(List.of(nonKv))); + } + + @Test + void mixed_args_are_not_named_arguments() { + assertFalse( + NamedArguments.isNamedArguments( + List.of(kv("k", AstDSL.intLiteral(1)), AstDSL.qualifiedName("col")))); + } + + @Test + void non_equals_function_is_not_named_arguments() { + UnresolvedExpression notEq = + new Function("+", List.of(AstDSL.stringLiteral("a"), AstDSL.intLiteral(1))); + assertFalse(NamedArguments.isNamedArguments(List.of(notEq))); + } + + @Test + void equals_with_non_string_left_is_not_named_arguments() { + UnresolvedExpression intEqInt = + new Function("=", List.of(AstDSL.intLiteral(1), AstDSL.intLiteral(2))); + assertFalse(NamedArguments.isNamedArguments(List.of(intEqInt))); + } + + /** + * The legacy spelling, {@code date_histogram(field='ts', ...)}, arrives here as an equality whose + * left side is a column reference rather than a string literal. Reading it as named arguments + * would take the call away from the legacy engine that has always served it. + */ + @Test + void equals_with_a_column_reference_on_the_left_is_not_named_arguments() { + UnresolvedExpression fieldEqValue = + new Function("=", List.of(AstDSL.qualifiedName("field"), AstDSL.stringLiteral("ts"))); + assertFalse(NamedArguments.isNamedArguments(List.of(fieldEqValue))); + } + + @Test + void equals_with_other_than_two_operands_is_not_named_arguments() { + UnresolvedExpression threeOperands = + new Function( + "=", List.of(AstDSL.stringLiteral("k"), AstDSL.intLiteral(1), AstDSL.intLiteral(2))); + assertFalse(NamedArguments.isNamedArguments(List.of(threeOperands))); + } + + @Test + void a_string_parameter_given_a_column_reference_is_rejected() { + NamedArguments bag = + NamedArguments.parse(List.of(kv("interval", AstDSL.qualifiedName("not_a_literal")))); + + assertThrows(SemanticCheckException.class, () -> bag.requireStringIfPresent("interval")); + } + + @Test + void parse_keeps_keys_in_source_order_and_lower_cases_them() { + NamedArguments bag = + NamedArguments.parse( + List.of( + kv("Field", AstDSL.stringLiteral("ts")), + kv("INTERVAL", AstDSL.stringLiteral("1d")))); + + assertEquals(AstDSL.stringLiteral("ts"), bag.remove("field")); + assertEquals(AstDSL.stringLiteral("1d"), bag.remove("interval")); + assertEquals(0, bag.size()); + } + + @Test + void parse_rejects_duplicate_keys() { + SemanticCheckException ex = + assertThrows( + SemanticCheckException.class, + () -> + NamedArguments.parse( + List.of( + kv("field", AstDSL.stringLiteral("a")), + kv("field", AstDSL.stringLiteral("b"))))); + assertTrue(ex.getMessage().contains("field")); + } + + @Test + void parse_rejects_non_key_value_arg_with_clear_message() { + UnresolvedExpression bareColumn = AstDSL.qualifiedName("age"); + SemanticCheckException ex = + assertThrows( + SemanticCheckException.class, + () -> + NamedArguments.parse(List.of(kv("field", AstDSL.stringLiteral("a")), bareColumn))); + assertTrue(ex.getMessage().contains("'key'=value")); + } + + @Test + void remove_returns_value_when_present_and_null_when_absent() { + NamedArguments bag = NamedArguments.parse(List.of(kv("field", AstDSL.stringLiteral("ts")))); + assertEquals(AstDSL.stringLiteral("ts"), bag.remove("field")); + assertNull(bag.remove("field")); + assertNull(bag.remove("never_inserted")); + assertEquals(0, bag.size()); + } + + @Test + void require_returns_value_and_removes_it() { + NamedArguments bag = NamedArguments.parse(List.of(kv("field", AstDSL.stringLiteral("ts")))); + assertEquals(AstDSL.stringLiteral("ts"), bag.require("field", "histogram")); + assertEquals(0, bag.size()); + } + + @Test + void require_throws_when_missing_with_function_name_in_message() { + NamedArguments bag = NamedArguments.parse(List.of(kv("other", AstDSL.intLiteral(1)))); + SemanticCheckException ex = + assertThrows(SemanticCheckException.class, () -> bag.require("field", "HISTOGRAM")); + assertTrue(ex.getMessage().contains("histogram")); + assertTrue(ex.getMessage().contains("field")); + } + + @Test + void requireString_returns_string_literal() { + NamedArguments bag = NamedArguments.parse(List.of(kv("interval", AstDSL.stringLiteral("1d")))); + Literal interval = bag.requireString("interval", "date_histogram"); + assertEquals(AstDSL.stringLiteral("1d"), interval); + } + + @Test + void requireString_rejects_non_string_value() { + NamedArguments bag = NamedArguments.parse(List.of(kv("interval", AstDSL.intLiteral(100)))); + assertThrows( + SemanticCheckException.class, () -> bag.requireString("interval", "date_histogram")); + } + + @Test + void requireStringIfPresent_returns_null_when_absent() { + NamedArguments bag = NamedArguments.parse(List.of(kv("field", AstDSL.stringLiteral("ts")))); + assertNull(bag.requireStringIfPresent("format")); + } + + @Test + void requireStringIfPresent_returns_value_when_present() { + NamedArguments bag = NamedArguments.parse(List.of(kv("format", AstDSL.stringLiteral("yyyy")))); + assertEquals(AstDSL.stringLiteral("yyyy"), bag.requireStringIfPresent("format")); + } + + @Test + void requireStringIfPresent_rejects_non_string_value_when_present() { + NamedArguments bag = NamedArguments.parse(List.of(kv("format", AstDSL.intLiteral(2024)))); + assertThrows(SemanticCheckException.class, () -> bag.requireStringIfPresent("format")); + } + + @Test + void rejectIfPresent_throws_when_key_present() { + NamedArguments bag = NamedArguments.parse(List.of(kv("script", AstDSL.stringLiteral("x")))); + SemanticCheckException ex = + assertThrows(SemanticCheckException.class, () -> bag.rejectIfPresent("script", "no!")); + assertTrue(ex.getMessage().contains("no!")); + } + + @Test + void rejectIfPresent_no_op_when_key_absent() { + NamedArguments bag = NamedArguments.parse(List.of(kv("field", AstDSL.stringLiteral("ts")))); + bag.rejectIfPresent("script", "no!"); + assertEquals(1, bag.size()); + } + + @Test + void consumeSilently_drops_listed_keys() { + NamedArguments bag = + NamedArguments.parse( + List.of( + kv("alias", AstDSL.stringLiteral("x")), + kv("nested", AstDSL.stringLiteral("p")), + kv("interval", AstDSL.intLiteral(10)))); + bag.consumeSilently(Set.of("alias", "nested")); + assertEquals(1, bag.size()); + } + + @Test + void rejectRemaining_single_key_uses_parameter_label() { + NamedArguments bag = NamedArguments.parse(List.of(kv("mystery", AstDSL.intLiteral(5)))); + SemanticCheckException ex = + assertThrows(SemanticCheckException.class, () -> bag.rejectRemaining("HISTOGRAM")); + assertTrue(ex.getMessage().contains("histogram")); + assertTrue(ex.getMessage().contains("does not accept parameter:")); + assertTrue(ex.getMessage().contains("mystery")); + } + + @Test + void rejectRemaining_multiple_keys_listed_in_source_order_with_plural_label() { + NamedArguments bag = + NamedArguments.parse( + List.of( + kv("foo", AstDSL.intLiteral(1)), + kv("bar", AstDSL.intLiteral(2)), + kv("baz", AstDSL.intLiteral(3)))); + SemanticCheckException ex = + assertThrows(SemanticCheckException.class, () -> bag.rejectRemaining("HISTOGRAM")); + assertTrue(ex.getMessage().contains("does not accept parameters:")); + assertTrue(ex.getMessage().contains("foo, bar, baz")); + } + + @Test + void rejectRemaining_no_op_when_bag_empty() { + NamedArguments bag = NamedArguments.parse(List.of(kv("alias", AstDSL.stringLiteral("x")))); + bag.consumeSilently(Set.of("alias")); + bag.rejectRemaining("histogram"); // does not throw + } + + /** Builds a Function("=", [stringLiteral(key), value]) — same shape ANTLR produces for 'k'=v. */ + private static UnresolvedExpression kv(String key, UnresolvedExpression value) { + return new Function("=", List.of(AstDSL.stringLiteral(key), value)); + } +} From cc420ab7a10e2f40324a7e99242efff2f0e5cd24 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Mon, 17 Aug 2026 14:21:33 -0700 Subject: [PATCH 02/18] Defer every unlowerable bucket call to the legacy engine CsvFormatResponseIT.dateHistogramTest has been asserting this query for years: SELECT COUNT(*) FROM GROUP BY date_histogram('field'='insert_time','fixed_interval'='4d','alias'='days') It broke once these names entered the V2 grammar. The keys are quoted, so V2 reads it as named arguments and takes over, then rejects `alias` -- a parameter the legacy engine implements and this expander does not. The earlier fix assumed the quoted-key form belongs to V2, so a bad parameter there is the caller's error. That is wrong: legacy uses the same spelling and accepts parameters V2 has no lowering for, so "unsupported here" cannot be treated as "invalid". Every rejection in the bucket package now raises SyntaxCheckException, which means anything this expander cannot lower reaches the legacy engine exactly as it did before the grammar change -- answered if legacy understands it, and refused with legacy's own message if not. The cost is that a genuine typo in the V2 form gets legacy's error rather than ours; that is worth far less than a query that used to work. Adds coverage for the `alias` case at both levels, since the positional form alone did not catch it. Signed-off-by: Jialiang Liang --- .../sql/DateHistogramBucketFunctionIT.java | 16 ++++++ .../parser/bucket/DateHistogramExpander.java | 7 ++- .../sql/sql/parser/bucket/NamedArguments.java | 18 +++---- .../bucket/DateHistogramExpanderTest.java | 54 +++++++++++-------- .../parser/bucket/HistogramExpanderTest.java | 29 +++++----- .../sql/parser/bucket/NamedArgumentsTest.java | 33 ++++++------ 6 files changed, 90 insertions(+), 67 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java index cef009a68cc..e6ee293937a 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java @@ -182,6 +182,22 @@ public void positionalCallStillReachesTheLegacyEngine() throws IOException { verifyDataRows(response, rows(12), rows(24), rows(17), rows(19)); } + /** + * The quoted-key form is not V2-only — legacy uses it too, with parameters V2 does not implement. + * `alias` is one, and CsvFormatResponseIT.dateHistogramTest has relied on it for years, so an + * unsupported parameter has to defer rather than fail. + */ + @Test + public void unsupportedParameterStillReachesTheLegacyEngine() throws IOException { + JSONObject response = + executeQuery( + "SELECT COUNT(*) FROM " + + IDX + + " GROUP BY date_histogram('field'='ts','fixed_interval'='1h','alias'='hours')"); + + verifyDataRows(response, rows(12), rows(24), rows(17), rows(19)); + } + @Test public void positionalNumericHistogramStillReachesTheLegacyEngine() throws IOException { JSONObject response = diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpander.java b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpander.java index 82be57dc9f3..0153ef82527 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpander.java +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpander.java @@ -18,7 +18,6 @@ import org.opensearch.sql.ast.expression.Span; import org.opensearch.sql.ast.expression.UnresolvedExpression; import org.opensearch.sql.common.antlr.SyntaxCheckException; -import org.opensearch.sql.exception.SemanticCheckException; /** * Lowers {@code date_histogram(...)} calls to a {@link Span} expression with the time unit inferred @@ -85,11 +84,11 @@ private static Literal extractIntervalLiteral(NamedArguments named) { Stream.of(interval, fixedInterval, calendarInterval).filter(Objects::nonNull).toList(); if (suppliedIntervals.isEmpty()) { - throw new SemanticCheckException( + throw new SyntaxCheckException( "date_histogram requires one of: interval, fixed_interval, calendar_interval"); } if (suppliedIntervals.size() > 1) { - throw new SemanticCheckException( + throw new SyntaxCheckException( "date_histogram accepts only one of: interval, fixed_interval, calendar_interval"); } return suppliedIntervals.get(0); @@ -125,7 +124,7 @@ private static UnresolvedExpression applyTimeZoneShift( try { offsetSeconds = ZoneOffset.of(tzString).getTotalSeconds(); } catch (RuntimeException ex) { - throw new SemanticCheckException( + throw new SyntaxCheckException( "time_zone must be a valid offset like '+05:30' or 'Z'; got '" + tzString + "'"); } return new Function( diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/NamedArguments.java b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/NamedArguments.java index 5a1ced9a949..fd1d47b2e60 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/NamedArguments.java +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/NamedArguments.java @@ -14,7 +14,7 @@ import org.opensearch.sql.ast.expression.Function; import org.opensearch.sql.ast.expression.Literal; import org.opensearch.sql.ast.expression.UnresolvedExpression; -import org.opensearch.sql.exception.SemanticCheckException; +import org.opensearch.sql.common.antlr.SyntaxCheckException; /** * Parses and validates named-argument style function arguments. The arg shape is {@code @@ -56,21 +56,21 @@ private static boolean isKeyValuePair(UnresolvedExpression arg) { /** * Parses the given args into a {@code NamedArguments}. Each arg must match the {@code - * 'key'=value} shape — a non-matching arg raises {@link SemanticCheckException}. Duplicate keys - * also raise {@link SemanticCheckException}. + * 'key'=value} shape — a non-matching arg raises {@link SyntaxCheckException}. Duplicate keys + * also raise {@link SyntaxCheckException}. */ public static NamedArguments parse(List args) { Map arguments = new LinkedHashMap<>(); for (UnresolvedExpression arg : args) { if (!isKeyValuePair(arg)) { - throw new SemanticCheckException("Named arguments must be of form 'key'=value; got " + arg); + throw new SyntaxCheckException("Named arguments must be of form 'key'=value; got " + arg); } Function fn = (Function) arg; Literal keyLiteral = (Literal) fn.getFuncArgs().get(0); String key = keyLiteral.getValue().toString().toLowerCase(Locale.ROOT); UnresolvedExpression value = fn.getFuncArgs().get(1); if (arguments.put(key, value) != null) { - throw new SemanticCheckException("Duplicate parameter: " + key); + throw new SyntaxCheckException("Duplicate parameter: " + key); } } return new NamedArguments(arguments); @@ -85,7 +85,7 @@ public UnresolvedExpression remove(String key) { public UnresolvedExpression require(String key, String funcName) { UnresolvedExpression value = arguments.remove(key); if (value == null) { - throw new SemanticCheckException( + throw new SyntaxCheckException( funcName.toLowerCase(Locale.ROOT) + " requires " + key + " parameter"); } return value; @@ -104,7 +104,7 @@ public Literal requireStringIfPresent(String key) { private static Literal asStringLiteral(UnresolvedExpression expr, String paramName) { if (!(expr instanceof Literal literal) || literal.getType() != DataType.STRING) { - throw new SemanticCheckException( + throw new SyntaxCheckException( paramName + " must be a string literal (e.g. '1d', '15m'); got " + expr); } return literal; @@ -113,7 +113,7 @@ private static Literal asStringLiteral(UnresolvedExpression expr, String paramNa /** If {@code key} is present, throws with the supplied message; otherwise no-op. */ public void rejectIfPresent(String key, String message) { if (arguments.remove(key) != null) { - throw new SemanticCheckException(message); + throw new SyntaxCheckException(message); } } @@ -131,7 +131,7 @@ public void rejectRemaining(String funcName) { } String label = arguments.size() == 1 ? "parameter" : "parameters"; String unsupported = String.join(", ", arguments.keySet()); - throw new SemanticCheckException( + throw new SyntaxCheckException( funcName.toLowerCase(Locale.ROOT) + " does not accept " + label + ": " + unsupported); } diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpanderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpanderTest.java index f1de66628c0..be657f92e79 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpanderTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpanderTest.java @@ -24,7 +24,6 @@ import org.opensearch.sql.ast.expression.UnresolvedExpression; import org.opensearch.sql.ast.tree.UnresolvedPlan; import org.opensearch.sql.common.antlr.SyntaxCheckException; -import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.sql.parser.AstBuilderTestBase; @DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) @@ -85,24 +84,35 @@ void property_bag_with_calendar_interval_param_lowers_to_span() { } /** - * The two types differ on purpose: an unrecognized shape falls back to the legacy engine, a bad - * parameter does not. Collapsing them would drop a working feature silently. + * Every rejection is a SyntaxCheckException, the one type RestSQLQueryAction falls back on, so + * anything this expander cannot lower goes to the legacy engine exactly as it did before these + * names entered the V2 grammar. `alias` is the case that matters: legacy accepts it, V2 does not, + * and it arrives in the same quoted-key form V2 uses. */ @Test - void separates_an_unrecognized_call_shape_from_bad_parameters() { + void every_rejection_defers_to_the_legacy_engine() { assertThrows( SyntaxCheckException.class, () -> expander.expand(List.of(AstDSL.qualifiedName("ts"), AstDSL.stringLiteral("1d")))); assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand(List.of(kv("field", AstDSL.qualifiedName("ts"))))); + + assertThrows( + SyntaxCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("ts")), + kv("fixed_interval", AstDSL.stringLiteral("4d")), + kv("alias", AstDSL.stringLiteral("days"))))); } @Test void property_bag_rejects_both_interval_and_fixed_interval() { assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -171,9 +181,9 @@ void property_bag_format_and_time_zone_compose() { @Test void property_bag_rejects_invalid_time_zone() { - SemanticCheckException ex = + SyntaxCheckException ex = assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -186,7 +196,7 @@ void property_bag_rejects_invalid_time_zone() { @Test void property_bag_rejects_alias() { assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -198,7 +208,7 @@ void property_bag_rejects_alias() { @Test void property_bag_rejects_nested() { assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -210,7 +220,7 @@ void property_bag_rejects_nested() { @Test void property_bag_rejects_reverse_nested() { assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -222,7 +232,7 @@ void property_bag_rejects_reverse_nested() { @Test void property_bag_rejects_children() { assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -248,9 +258,9 @@ void property_bag_missing_wraps_field_with_coalesce() { @Test void property_bag_rejects_offset() { - SemanticCheckException ex = + SyntaxCheckException ex = assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -264,7 +274,7 @@ void property_bag_rejects_offset() { @Test void property_bag_rejects_min_doc_count() { assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -276,7 +286,7 @@ void property_bag_rejects_min_doc_count() { @Test void property_bag_rejects_extended_bounds() { assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -287,9 +297,9 @@ void property_bag_rejects_extended_bounds() { @Test void property_bag_rejects_unknown_param() { - SemanticCheckException ex = + SyntaxCheckException ex = assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -302,7 +312,7 @@ void property_bag_rejects_unknown_param() { @Test void property_bag_rejects_duplicate_keys() { assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -314,15 +324,15 @@ void property_bag_rejects_duplicate_keys() { @Test void property_bag_rejects_missing_field() { assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand(List.of(kv("interval", AstDSL.stringLiteral("1d"))))); } @Test void property_bag_rejects_when_no_interval_synonym_provided() { - SemanticCheckException ex = + SyntaxCheckException ex = assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand(List.of(kv("field", AstDSL.stringLiteral("ts"))))); assertTrue(ex.getMessage().contains("requires one of")); } diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/HistogramExpanderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/HistogramExpanderTest.java index 67c801e3392..7fe6d5e66f0 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/HistogramExpanderTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/HistogramExpanderTest.java @@ -24,7 +24,6 @@ import org.opensearch.sql.ast.expression.UnresolvedExpression; import org.opensearch.sql.ast.tree.UnresolvedPlan; import org.opensearch.sql.common.antlr.SyntaxCheckException; -import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.sql.parser.AstBuilderTestBase; @DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) @@ -65,7 +64,7 @@ void property_bag_with_qualified_name_field_passes_through_unchanged() { @Test void property_bag_rejects_alias() { assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -77,7 +76,7 @@ void property_bag_rejects_alias() { @Test void property_bag_rejects_nested() { assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -89,7 +88,7 @@ void property_bag_rejects_nested() { @Test void property_bag_rejects_reverse_nested() { assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -101,7 +100,7 @@ void property_bag_rejects_reverse_nested() { @Test void property_bag_rejects_children() { assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -113,7 +112,7 @@ void property_bag_rejects_children() { @Test void property_bag_rejects_format() { assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -125,7 +124,7 @@ void property_bag_rejects_format() { @Test void property_bag_rejects_time_zone() { assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -137,7 +136,7 @@ void property_bag_rejects_time_zone() { @Test void property_bag_rejects_min_doc_count() { assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -149,7 +148,7 @@ void property_bag_rejects_min_doc_count() { @Test void property_bag_rejects_order() { assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -161,7 +160,7 @@ void property_bag_rejects_order() { @Test void property_bag_rejects_extended_bounds() { assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -172,9 +171,9 @@ void property_bag_rejects_extended_bounds() { @Test void property_bag_rejects_unknown_param() { - SemanticCheckException ex = + SyntaxCheckException ex = assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -187,7 +186,7 @@ void property_bag_rejects_unknown_param() { @Test void property_bag_rejects_duplicate_keys() { assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand( List.of( @@ -199,14 +198,14 @@ void property_bag_rejects_duplicate_keys() { @Test void property_bag_rejects_missing_field() { assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand(List.of(kv("interval", AstDSL.intLiteral(10))))); } @Test void property_bag_rejects_missing_interval() { assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> expander.expand(List.of(kv("field", AstDSL.stringLiteral("age"))))); } diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/NamedArgumentsTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/NamedArgumentsTest.java index 267ddc06165..1d7edb6af5c 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/NamedArgumentsTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/NamedArgumentsTest.java @@ -20,7 +20,7 @@ import org.opensearch.sql.ast.expression.Function; import org.opensearch.sql.ast.expression.Literal; import org.opensearch.sql.ast.expression.UnresolvedExpression; -import org.opensearch.sql.exception.SemanticCheckException; +import org.opensearch.sql.common.antlr.SyntaxCheckException; @DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) class NamedArgumentsTest { @@ -87,7 +87,7 @@ void a_string_parameter_given_a_column_reference_is_rejected() { NamedArguments bag = NamedArguments.parse(List.of(kv("interval", AstDSL.qualifiedName("not_a_literal")))); - assertThrows(SemanticCheckException.class, () -> bag.requireStringIfPresent("interval")); + assertThrows(SyntaxCheckException.class, () -> bag.requireStringIfPresent("interval")); } @Test @@ -105,9 +105,9 @@ void parse_keeps_keys_in_source_order_and_lower_cases_them() { @Test void parse_rejects_duplicate_keys() { - SemanticCheckException ex = + SyntaxCheckException ex = assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> NamedArguments.parse( List.of( @@ -119,9 +119,9 @@ void parse_rejects_duplicate_keys() { @Test void parse_rejects_non_key_value_arg_with_clear_message() { UnresolvedExpression bareColumn = AstDSL.qualifiedName("age"); - SemanticCheckException ex = + SyntaxCheckException ex = assertThrows( - SemanticCheckException.class, + SyntaxCheckException.class, () -> NamedArguments.parse(List.of(kv("field", AstDSL.stringLiteral("a")), bareColumn))); assertTrue(ex.getMessage().contains("'key'=value")); @@ -146,8 +146,8 @@ void require_returns_value_and_removes_it() { @Test void require_throws_when_missing_with_function_name_in_message() { NamedArguments bag = NamedArguments.parse(List.of(kv("other", AstDSL.intLiteral(1)))); - SemanticCheckException ex = - assertThrows(SemanticCheckException.class, () -> bag.require("field", "HISTOGRAM")); + SyntaxCheckException ex = + assertThrows(SyntaxCheckException.class, () -> bag.require("field", "HISTOGRAM")); assertTrue(ex.getMessage().contains("histogram")); assertTrue(ex.getMessage().contains("field")); } @@ -162,8 +162,7 @@ void requireString_returns_string_literal() { @Test void requireString_rejects_non_string_value() { NamedArguments bag = NamedArguments.parse(List.of(kv("interval", AstDSL.intLiteral(100)))); - assertThrows( - SemanticCheckException.class, () -> bag.requireString("interval", "date_histogram")); + assertThrows(SyntaxCheckException.class, () -> bag.requireString("interval", "date_histogram")); } @Test @@ -181,14 +180,14 @@ void requireStringIfPresent_returns_value_when_present() { @Test void requireStringIfPresent_rejects_non_string_value_when_present() { NamedArguments bag = NamedArguments.parse(List.of(kv("format", AstDSL.intLiteral(2024)))); - assertThrows(SemanticCheckException.class, () -> bag.requireStringIfPresent("format")); + assertThrows(SyntaxCheckException.class, () -> bag.requireStringIfPresent("format")); } @Test void rejectIfPresent_throws_when_key_present() { NamedArguments bag = NamedArguments.parse(List.of(kv("script", AstDSL.stringLiteral("x")))); - SemanticCheckException ex = - assertThrows(SemanticCheckException.class, () -> bag.rejectIfPresent("script", "no!")); + SyntaxCheckException ex = + assertThrows(SyntaxCheckException.class, () -> bag.rejectIfPresent("script", "no!")); assertTrue(ex.getMessage().contains("no!")); } @@ -214,8 +213,8 @@ void consumeSilently_drops_listed_keys() { @Test void rejectRemaining_single_key_uses_parameter_label() { NamedArguments bag = NamedArguments.parse(List.of(kv("mystery", AstDSL.intLiteral(5)))); - SemanticCheckException ex = - assertThrows(SemanticCheckException.class, () -> bag.rejectRemaining("HISTOGRAM")); + SyntaxCheckException ex = + assertThrows(SyntaxCheckException.class, () -> bag.rejectRemaining("HISTOGRAM")); assertTrue(ex.getMessage().contains("histogram")); assertTrue(ex.getMessage().contains("does not accept parameter:")); assertTrue(ex.getMessage().contains("mystery")); @@ -229,8 +228,8 @@ void rejectRemaining_multiple_keys_listed_in_source_order_with_plural_label() { kv("foo", AstDSL.intLiteral(1)), kv("bar", AstDSL.intLiteral(2)), kv("baz", AstDSL.intLiteral(3)))); - SemanticCheckException ex = - assertThrows(SemanticCheckException.class, () -> bag.rejectRemaining("HISTOGRAM")); + SyntaxCheckException ex = + assertThrows(SyntaxCheckException.class, () -> bag.rejectRemaining("HISTOGRAM")); assertTrue(ex.getMessage().contains("does not accept parameters:")); assertTrue(ex.getMessage().contains("foo, bar, baz")); } From 7072d03c3e8c414f4727b8dc3600bd39071f8c6a Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Tue, 18 Aug 2026 11:15:53 -0700 Subject: [PATCH 03/18] Make the bucket-function ITs behave the same with and without analytics engine Verified against a local analytics-engine sandbox (9 plugins, every index parquet-backed so all data queries route to DataFusion). Three problems showed up, none of them visible on the default route. The dataset could not load at all. Parquet-backed indices are append-only and reject a custom document id, so all 72 bulk items failed and every assertion saw an empty index. The ids were never read by any test; dropping them lets the same dataset load on both routes. Three tests asserted results that only the legacy engine can produce. The old `date_histogram(field=, ...)` spelling, and the `alias` parameter, are understood only by the legacy V1 engine, and that engine is reachable only through RestSQLQueryAction -- the analytics route enters through RestUnifiedQueryAction, which has no fallback to it. Those queries have never worked on the analytics route, before or after this change, so tests asserting their results can only ever pass on one of the two. Removed. The behaviour they guarded is still covered where it belongs: CsvFormatResponseIT.dateHistogramTest has asserted the `alias` shape for years and is what caught the regression in CI, and the expander unit tests assert the exception type directly, without needing an engine at all. One test asserted a failure -- that a second grouping key over a bare table scan leaves the span's field typed UNDEFINED. That is a V2 execution defect, not a property of these functions, and the analytics route resolves the same query correctly. Pinning it made the suite demand an engine bug stay unfixed and fail wherever it was already fixed. Removed; the constraint is noted on the test that uses the derived-table form. Seven tests remain, all asserting what a query returns rather than which engine answered it. They pass identically on both routes. Signed-off-by: Jialiang Liang --- .../sql/DateHistogramBucketFunctionIT.java | 68 +-------- .../test/resources/date_histogram_test.json | 144 +++++++++--------- 2 files changed, 76 insertions(+), 136 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java index e6ee293937a..69183f5f8e9 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java @@ -5,9 +5,6 @@ package org.opensearch.sql.sql; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertThrows; -import static org.junit.Assert.assertTrue; import static org.opensearch.sql.util.MatcherUtils.rows; import static org.opensearch.sql.util.MatcherUtils.verifyDataRows; import static org.opensearch.sql.util.MatcherUtils.verifyDataRowsInOrder; @@ -15,7 +12,6 @@ import java.io.IOException; import org.json.JSONObject; import org.junit.Test; -import org.opensearch.client.ResponseException; import org.opensearch.sql.legacy.SQLIntegTestCase; /** @@ -101,7 +97,10 @@ public void intervalSynonymsProduceTheSameBuckets() throws IOException { } } - /** Only resolves with the scan in its own derived table — see the test below. */ + /** + * The scan sits in its own derived table because the V2 engine cannot resolve the span's field + * otherwise when a second grouping key is present. + */ @Test public void bucketsCombineWithAnAdditionalGroupingKey() throws IOException { JSONObject response = @@ -121,27 +120,6 @@ public void bucketsCombineWithAnAdditionalGroupingKey() throws IOException { rows("2026-01-01 03:00:00", "alpha", 19)); } - /** - * A limitation, not a guarantee: two grouping keys over a bare table scan leave the span's field - * typed UNDEFINED. One key is fine, and a derived table resolves it. If this starts passing, the - * engine was fixed — relax the test. - */ - @Test - public void bucketWithASecondKeyNeedsTheScanInItsOwnDerivedTable() { - ResponseException error = - assertThrows( - ResponseException.class, - () -> - executeQuery( - "SELECT b, c, COUNT(*) FROM (SELECT date_histogram('field'=ts," - + " 'interval'='1h') AS b, category AS c FROM " - + IDX - + ") sub GROUP BY b, c ORDER BY b, c")); - - assertEquals(400, error.getResponse().getStatusLine().getStatusCode()); - assertTrue(error.getMessage().contains("UNDEFINED")); - } - @Test public void bucketsRespectAWhereClause() throws IOException { JSONObject response = @@ -168,42 +146,4 @@ public void numericHistogramBucketsByInterval() throws IOException { // value runs 1..72, so the 20-wide buckets hold 19, 20, 20 and 13 documents. verifyDataRowsInOrder(response, rows(0, 19), rows(20, 20), rows(40, 20), rows(60, 13)); } - - /** - * V2 now matches these calls first, so declining with anything but SyntaxCheckException would cut - * off the legacy engine that has always answered the positional form. - */ - @Test - public void positionalCallStillReachesTheLegacyEngine() throws IOException { - JSONObject response = - executeQuery( - "SELECT COUNT(*) FROM " + IDX + " GROUP BY date_histogram(field='ts','interval'='1h')"); - - verifyDataRows(response, rows(12), rows(24), rows(17), rows(19)); - } - - /** - * The quoted-key form is not V2-only — legacy uses it too, with parameters V2 does not implement. - * `alias` is one, and CsvFormatResponseIT.dateHistogramTest has relied on it for years, so an - * unsupported parameter has to defer rather than fail. - */ - @Test - public void unsupportedParameterStillReachesTheLegacyEngine() throws IOException { - JSONObject response = - executeQuery( - "SELECT COUNT(*) FROM " - + IDX - + " GROUP BY date_histogram('field'='ts','fixed_interval'='1h','alias'='hours')"); - - verifyDataRows(response, rows(12), rows(24), rows(17), rows(19)); - } - - @Test - public void positionalNumericHistogramStillReachesTheLegacyEngine() throws IOException { - JSONObject response = - executeQuery( - "SELECT COUNT(*) FROM " + IDX + " GROUP BY histogram(field='value','interval'='20')"); - - verifyDataRows(response, rows(19), rows(20), rows(20), rows(13)); - } } diff --git a/integ-test/src/test/resources/date_histogram_test.json b/integ-test/src/test/resources/date_histogram_test.json index a46919462e6..2d43eca9da3 100644 --- a/integ-test/src/test/resources/date_histogram_test.json +++ b/integ-test/src/test/resources/date_histogram_test.json @@ -1,144 +1,144 @@ -{"index":{"_id":"1"}} +{"index":{}} {"ts":"2026-01-01 00:00:00","category":"alpha","value":1} -{"index":{"_id":"2"}} +{"index":{}} {"ts":"2026-01-01 00:00:00","category":"alpha","value":2} -{"index":{"_id":"3"}} +{"index":{}} {"ts":"2026-01-01 00:00:00","category":"alpha","value":3} -{"index":{"_id":"4"}} +{"index":{}} {"ts":"2026-01-01 00:00:00","category":"alpha","value":4} -{"index":{"_id":"5"}} +{"index":{}} {"ts":"2026-01-01 00:00:00","category":"alpha","value":5} -{"index":{"_id":"6"}} +{"index":{}} {"ts":"2026-01-01 00:30:00","category":"beta","value":6} -{"index":{"_id":"7"}} +{"index":{}} {"ts":"2026-01-01 00:30:00","category":"beta","value":7} -{"index":{"_id":"8"}} +{"index":{}} {"ts":"2026-01-01 00:30:00","category":"beta","value":8} -{"index":{"_id":"9"}} +{"index":{}} {"ts":"2026-01-01 00:30:00","category":"beta","value":9} -{"index":{"_id":"10"}} +{"index":{}} {"ts":"2026-01-01 00:30:00","category":"beta","value":10} -{"index":{"_id":"11"}} +{"index":{}} {"ts":"2026-01-01 00:30:00","category":"beta","value":11} -{"index":{"_id":"12"}} +{"index":{}} {"ts":"2026-01-01 00:30:00","category":"beta","value":12} -{"index":{"_id":"13"}} +{"index":{}} {"ts":"2026-01-01 01:00:00","category":"alpha","value":13} -{"index":{"_id":"14"}} +{"index":{}} {"ts":"2026-01-01 01:00:00","category":"alpha","value":14} -{"index":{"_id":"15"}} +{"index":{}} {"ts":"2026-01-01 01:00:00","category":"alpha","value":15} -{"index":{"_id":"16"}} +{"index":{}} {"ts":"2026-01-01 01:00:00","category":"alpha","value":16} -{"index":{"_id":"17"}} +{"index":{}} {"ts":"2026-01-01 01:00:00","category":"alpha","value":17} -{"index":{"_id":"18"}} +{"index":{}} {"ts":"2026-01-01 01:00:00","category":"alpha","value":18} -{"index":{"_id":"19"}} +{"index":{}} {"ts":"2026-01-01 01:00:00","category":"alpha","value":19} -{"index":{"_id":"20"}} +{"index":{}} {"ts":"2026-01-01 01:00:00","category":"alpha","value":20} -{"index":{"_id":"21"}} +{"index":{}} {"ts":"2026-01-01 01:00:00","category":"alpha","value":21} -{"index":{"_id":"22"}} +{"index":{}} {"ts":"2026-01-01 01:00:00","category":"alpha","value":22} -{"index":{"_id":"23"}} +{"index":{}} {"ts":"2026-01-01 01:00:00","category":"alpha","value":23} -{"index":{"_id":"24"}} +{"index":{}} {"ts":"2026-01-01 01:45:00","category":"gamma","value":24} -{"index":{"_id":"25"}} +{"index":{}} {"ts":"2026-01-01 01:45:00","category":"gamma","value":25} -{"index":{"_id":"26"}} +{"index":{}} {"ts":"2026-01-01 01:45:00","category":"gamma","value":26} -{"index":{"_id":"27"}} +{"index":{}} {"ts":"2026-01-01 01:45:00","category":"gamma","value":27} -{"index":{"_id":"28"}} +{"index":{}} {"ts":"2026-01-01 01:45:00","category":"gamma","value":28} -{"index":{"_id":"29"}} +{"index":{}} {"ts":"2026-01-01 01:45:00","category":"gamma","value":29} -{"index":{"_id":"30"}} +{"index":{}} {"ts":"2026-01-01 01:45:00","category":"gamma","value":30} -{"index":{"_id":"31"}} +{"index":{}} {"ts":"2026-01-01 01:45:00","category":"gamma","value":31} -{"index":{"_id":"32"}} +{"index":{}} {"ts":"2026-01-01 01:45:00","category":"gamma","value":32} -{"index":{"_id":"33"}} +{"index":{}} {"ts":"2026-01-01 01:45:00","category":"gamma","value":33} -{"index":{"_id":"34"}} +{"index":{}} {"ts":"2026-01-01 01:45:00","category":"gamma","value":34} -{"index":{"_id":"35"}} +{"index":{}} {"ts":"2026-01-01 01:45:00","category":"gamma","value":35} -{"index":{"_id":"36"}} +{"index":{}} {"ts":"2026-01-01 01:45:00","category":"gamma","value":36} -{"index":{"_id":"37"}} +{"index":{}} {"ts":"2026-01-01 02:00:00","category":"beta","value":37} -{"index":{"_id":"38"}} +{"index":{}} {"ts":"2026-01-01 02:00:00","category":"beta","value":38} -{"index":{"_id":"39"}} +{"index":{}} {"ts":"2026-01-01 02:00:00","category":"beta","value":39} -{"index":{"_id":"40"}} +{"index":{}} {"ts":"2026-01-01 02:00:00","category":"beta","value":40} -{"index":{"_id":"41"}} +{"index":{}} {"ts":"2026-01-01 02:00:00","category":"beta","value":41} -{"index":{"_id":"42"}} +{"index":{}} {"ts":"2026-01-01 02:00:00","category":"beta","value":42} -{"index":{"_id":"43"}} +{"index":{}} {"ts":"2026-01-01 02:00:00","category":"beta","value":43} -{"index":{"_id":"44"}} +{"index":{}} {"ts":"2026-01-01 02:00:00","category":"beta","value":44} -{"index":{"_id":"45"}} +{"index":{}} {"ts":"2026-01-01 02:00:00","category":"beta","value":45} -{"index":{"_id":"46"}} +{"index":{}} {"ts":"2026-01-01 02:00:00","category":"beta","value":46} -{"index":{"_id":"47"}} +{"index":{}} {"ts":"2026-01-01 02:00:00","category":"beta","value":47} -{"index":{"_id":"48"}} +{"index":{}} {"ts":"2026-01-01 02:00:00","category":"beta","value":48} -{"index":{"_id":"49"}} +{"index":{}} {"ts":"2026-01-01 02:00:00","category":"beta","value":49} -{"index":{"_id":"50"}} +{"index":{}} {"ts":"2026-01-01 02:00:00","category":"beta","value":50} -{"index":{"_id":"51"}} +{"index":{}} {"ts":"2026-01-01 02:00:00","category":"beta","value":51} -{"index":{"_id":"52"}} +{"index":{}} {"ts":"2026-01-01 02:00:00","category":"beta","value":52} -{"index":{"_id":"53"}} +{"index":{}} {"ts":"2026-01-01 02:00:00","category":"beta","value":53} -{"index":{"_id":"54"}} +{"index":{}} {"ts":"2026-01-01 03:00:00","category":"alpha","value":54} -{"index":{"_id":"55"}} +{"index":{}} {"ts":"2026-01-01 03:00:00","category":"alpha","value":55} -{"index":{"_id":"56"}} +{"index":{}} {"ts":"2026-01-01 03:00:00","category":"alpha","value":56} -{"index":{"_id":"57"}} +{"index":{}} {"ts":"2026-01-01 03:00:00","category":"alpha","value":57} -{"index":{"_id":"58"}} +{"index":{}} {"ts":"2026-01-01 03:00:00","category":"alpha","value":58} -{"index":{"_id":"59"}} +{"index":{}} {"ts":"2026-01-01 03:00:00","category":"alpha","value":59} -{"index":{"_id":"60"}} +{"index":{}} {"ts":"2026-01-01 03:00:00","category":"alpha","value":60} -{"index":{"_id":"61"}} +{"index":{}} {"ts":"2026-01-01 03:00:00","category":"alpha","value":61} -{"index":{"_id":"62"}} +{"index":{}} {"ts":"2026-01-01 03:00:00","category":"alpha","value":62} -{"index":{"_id":"63"}} +{"index":{}} {"ts":"2026-01-01 03:00:00","category":"alpha","value":63} -{"index":{"_id":"64"}} +{"index":{}} {"ts":"2026-01-01 03:00:00","category":"alpha","value":64} -{"index":{"_id":"65"}} +{"index":{}} {"ts":"2026-01-01 03:00:00","category":"alpha","value":65} -{"index":{"_id":"66"}} +{"index":{}} {"ts":"2026-01-01 03:00:00","category":"alpha","value":66} -{"index":{"_id":"67"}} +{"index":{}} {"ts":"2026-01-01 03:00:00","category":"alpha","value":67} -{"index":{"_id":"68"}} +{"index":{}} {"ts":"2026-01-01 03:00:00","category":"alpha","value":68} -{"index":{"_id":"69"}} +{"index":{}} {"ts":"2026-01-01 03:00:00","category":"alpha","value":69} -{"index":{"_id":"70"}} +{"index":{}} {"ts":"2026-01-01 03:00:00","category":"alpha","value":70} -{"index":{"_id":"71"}} +{"index":{}} {"ts":"2026-01-01 03:00:00","category":"alpha","value":71} -{"index":{"_id":"72"}} +{"index":{}} {"ts":"2026-01-01 03:00:00","category":"alpha","value":72} From 510eaaf44680f3d1832883e4d5e4170c4ee182c8 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Tue, 18 Aug 2026 11:28:35 -0700 Subject: [PATCH 04/18] Gate the legacy-only bucket tests behind a capability instead of dropping them Three of these tests assert results only the legacy V1 engine can produce: the positional `date_histogram(field=, ...)` spelling and the `alias` parameter. That engine is reachable only through RestSQLQueryAction's SyntaxCheckException fallback, and the analytics-engine route enters through RestUnifiedQueryAction, which has no such fallback -- so those queries have never worked there. They were removed in the previous commit to keep the suite green on both routes. Restoring them behind @RequiresCapability keeps the guard where it matters and still leaves both routes green, which is what the existing capability mechanism is for: the default route runs all ten, the analytics route skips these three with the reason printed. The guard is worth keeping -- these are the shapes a V2 grammar addition can silently take away from the legacy engine, which is exactly the regression CI caught here. LEGACY_ENGINE_FALLBACK is worded after LEGACY_METHOD_QUERY, which covers the same situation for method-query syntax. Signed-off-by: Jialiang Liang --- .../sql/DateHistogramBucketFunctionIT.java | 42 +++++++++++++++++++ .../org/opensearch/sql/util/Capability.java | 11 +++++ 2 files changed, 53 insertions(+) diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java index 69183f5f8e9..76e154cf2e7 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java @@ -5,6 +5,7 @@ package org.opensearch.sql.sql; +import static org.opensearch.sql.util.Capability.LEGACY_ENGINE_FALLBACK; import static org.opensearch.sql.util.MatcherUtils.rows; import static org.opensearch.sql.util.MatcherUtils.verifyDataRows; import static org.opensearch.sql.util.MatcherUtils.verifyDataRowsInOrder; @@ -13,6 +14,7 @@ import org.json.JSONObject; import org.junit.Test; import org.opensearch.sql.legacy.SQLIntegTestCase; +import org.opensearch.sql.util.RequiresCapability; /** * Execution coverage for {@code date_histogram} and {@code histogram}. The expander unit tests @@ -146,4 +148,44 @@ public void numericHistogramBucketsByInterval() throws IOException { // value runs 1..72, so the 20-wide buckets hold 19, 20, 20 and 13 documents. verifyDataRowsInOrder(response, rows(0, 19), rows(20, 20), rows(40, 20), rows(60, 13)); } + + /** + * Only the legacy V1 engine understands this spelling, and it answered before these names entered + * the V2 grammar. The expander has to keep declining with SyntaxCheckException so it still does. + */ + @Test + @RequiresCapability(LEGACY_ENGINE_FALLBACK) + public void positionalCallReturnsHourlyBuckets() throws IOException { + JSONObject response = + executeQuery( + "SELECT COUNT(*) FROM " + IDX + " GROUP BY date_histogram(field='ts','interval'='1h')"); + + verifyDataRows(response, rows(12), rows(24), rows(17), rows(19)); + } + + /** + * `alias` has no lowering here but the legacy engine implements it, so the query still has to + * answer. CsvFormatResponseIT.dateHistogramTest has asserted this shape for years. + */ + @Test + @RequiresCapability(LEGACY_ENGINE_FALLBACK) + public void callWithAliasParameterReturnsHourlyBuckets() throws IOException { + JSONObject response = + executeQuery( + "SELECT COUNT(*) FROM " + + IDX + + " GROUP BY date_histogram('field'='ts','fixed_interval'='1h','alias'='hours')"); + + verifyDataRows(response, rows(12), rows(24), rows(17), rows(19)); + } + + @Test + @RequiresCapability(LEGACY_ENGINE_FALLBACK) + public void positionalNumericHistogramReturnsBuckets() throws IOException { + JSONObject response = + executeQuery( + "SELECT COUNT(*) FROM " + IDX + " GROUP BY histogram(field='value','interval'='20')"); + + verifyDataRows(response, rows(19), rows(20), rows(20), rows(13)); + } } diff --git a/integ-test/src/test/java/org/opensearch/sql/util/Capability.java b/integ-test/src/test/java/org/opensearch/sql/util/Capability.java index 1d4067f1414..f5a5b784e38 100644 --- a/integ-test/src/test/java/org/opensearch/sql/util/Capability.java +++ b/integ-test/src/test/java/org/opensearch/sql/util/Capability.java @@ -541,6 +541,17 @@ public enum Capability { * FRONTEND: legacy method-query syntax (regexp_query/wildcard_query) is not in the Calcite * grammar. */ + /** + * FRONTEND: the legacy V1 engine answers call shapes the V2 grammar declines, but only on the + * default route. Requests reach it when RestSQLQueryAction catches a SyntaxCheckException; the + * analytics-engine route enters through RestUnifiedQueryAction, which has no such fallback. + */ + LEGACY_ENGINE_FALLBACK( + "A call shape only the legacy V1 engine understands (e.g. positional" + + " date_histogram(field=, ...), or an `alias` parameter) can't be answered on the" + + " analytics-engine route: reaching that engine depends on RestSQLQueryAction's" + + " SyntaxCheckException fallback, and the analytics route does not go through it."), + LEGACY_METHOD_QUERY( "Legacy method-query syntax (regexp_query/wildcard_query/query/matchquery) is not in the" + " Calcite grammar used by the analytics-engine route."), From b2cdea121ded3b6a195b583a7db849777179ede0 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Tue, 18 Aug 2026 19:08:31 -0700 Subject: [PATCH 05/18] Report a bad argument instead of deferring to the legacy engine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback: making every rejection a SyntaxCheckException caused an unexpected fallback. A misspelled `time_zone`, two interval synonyms at once, a missing required parameter — all of those were being handed to the legacy engine, which answers with an opaque parser error about a query the user never wrote, hiding the message that would have told them what was wrong. AstBuilder.visitTableFunctionRelation already makes this call for table functions, in a comment that says as much: "Use SemanticCheckException (not SyntaxCheckException) so the request does not fall back to the legacy SQL engine, whose opaque parser error would mask this message." Same split here. SyntaxCheckException is now reserved for the two cases that mean "this call shape is not mine": arguments that are not the named form at all, and named arguments carrying a parameter this expander has no lowering for, such as `alias`, which the legacy engine does implement. Those still have to reach it. Everything else — missing field, missing or duplicated interval, a non-string where a string literal is required, an invalid time zone — is the caller's mistake inside a shape this expander owns, and now says so directly. The boundary test asserts both halves so neither can be collapsed into the other without failing. Signed-off-by: Jialiang Liang --- .../parser/bucket/DateHistogramExpander.java | 7 +-- .../sql/sql/parser/bucket/NamedArguments.java | 9 ++-- .../bucket/DateHistogramExpanderTest.java | 44 ++++++++++++------- .../parser/bucket/HistogramExpanderTest.java | 7 +-- .../sql/parser/bucket/NamedArgumentsTest.java | 20 +++++---- 5 files changed, 52 insertions(+), 35 deletions(-) diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpander.java b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpander.java index 0153ef82527..82be57dc9f3 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpander.java +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpander.java @@ -18,6 +18,7 @@ import org.opensearch.sql.ast.expression.Span; import org.opensearch.sql.ast.expression.UnresolvedExpression; import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.exception.SemanticCheckException; /** * Lowers {@code date_histogram(...)} calls to a {@link Span} expression with the time unit inferred @@ -84,11 +85,11 @@ private static Literal extractIntervalLiteral(NamedArguments named) { Stream.of(interval, fixedInterval, calendarInterval).filter(Objects::nonNull).toList(); if (suppliedIntervals.isEmpty()) { - throw new SyntaxCheckException( + throw new SemanticCheckException( "date_histogram requires one of: interval, fixed_interval, calendar_interval"); } if (suppliedIntervals.size() > 1) { - throw new SyntaxCheckException( + throw new SemanticCheckException( "date_histogram accepts only one of: interval, fixed_interval, calendar_interval"); } return suppliedIntervals.get(0); @@ -124,7 +125,7 @@ private static UnresolvedExpression applyTimeZoneShift( try { offsetSeconds = ZoneOffset.of(tzString).getTotalSeconds(); } catch (RuntimeException ex) { - throw new SyntaxCheckException( + throw new SemanticCheckException( "time_zone must be a valid offset like '+05:30' or 'Z'; got '" + tzString + "'"); } return new Function( diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/NamedArguments.java b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/NamedArguments.java index fd1d47b2e60..4f10a68f8a8 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/NamedArguments.java +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/NamedArguments.java @@ -15,6 +15,7 @@ import org.opensearch.sql.ast.expression.Literal; import org.opensearch.sql.ast.expression.UnresolvedExpression; import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.exception.SemanticCheckException; /** * Parses and validates named-argument style function arguments. The arg shape is {@code @@ -63,14 +64,14 @@ public static NamedArguments parse(List args) { Map arguments = new LinkedHashMap<>(); for (UnresolvedExpression arg : args) { if (!isKeyValuePair(arg)) { - throw new SyntaxCheckException("Named arguments must be of form 'key'=value; got " + arg); + throw new SemanticCheckException("Named arguments must be of form 'key'=value; got " + arg); } Function fn = (Function) arg; Literal keyLiteral = (Literal) fn.getFuncArgs().get(0); String key = keyLiteral.getValue().toString().toLowerCase(Locale.ROOT); UnresolvedExpression value = fn.getFuncArgs().get(1); if (arguments.put(key, value) != null) { - throw new SyntaxCheckException("Duplicate parameter: " + key); + throw new SemanticCheckException("Duplicate parameter: " + key); } } return new NamedArguments(arguments); @@ -85,7 +86,7 @@ public UnresolvedExpression remove(String key) { public UnresolvedExpression require(String key, String funcName) { UnresolvedExpression value = arguments.remove(key); if (value == null) { - throw new SyntaxCheckException( + throw new SemanticCheckException( funcName.toLowerCase(Locale.ROOT) + " requires " + key + " parameter"); } return value; @@ -104,7 +105,7 @@ public Literal requireStringIfPresent(String key) { private static Literal asStringLiteral(UnresolvedExpression expr, String paramName) { if (!(expr instanceof Literal literal) || literal.getType() != DataType.STRING) { - throw new SyntaxCheckException( + throw new SemanticCheckException( paramName + " must be a string literal (e.g. '1d', '15m'); got " + expr); } return literal; diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpanderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpanderTest.java index be657f92e79..9c1753da07e 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpanderTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpanderTest.java @@ -24,6 +24,7 @@ import org.opensearch.sql.ast.expression.UnresolvedExpression; import org.opensearch.sql.ast.tree.UnresolvedPlan; import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.sql.parser.AstBuilderTestBase; @DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) @@ -84,21 +85,19 @@ void property_bag_with_calendar_interval_param_lowers_to_span() { } /** - * Every rejection is a SyntaxCheckException, the one type RestSQLQueryAction falls back on, so - * anything this expander cannot lower goes to the legacy engine exactly as it did before these - * names entered the V2 grammar. `alias` is the case that matters: legacy accepts it, V2 does not, - * and it arrives in the same quoted-key form V2 uses. + * The split matters. A call shape this expander does not own has to raise SyntaxCheckException, + * the one type RestSQLQueryAction falls back on, so the legacy engine keeps answering the + * positional form and parameters like `alias` that it implements and this one does not. A bad + * argument inside a shape we do own raises SemanticCheckException instead, so the caller gets + * this message rather than an opaque legacy parser error -- the same choice + * AstBuilder.visitTableFunctionRelation makes. */ @Test - void every_rejection_defers_to_the_legacy_engine() { + void unowned_shapes_defer_but_bad_arguments_do_not() { assertThrows( SyntaxCheckException.class, () -> expander.expand(List.of(AstDSL.qualifiedName("ts"), AstDSL.stringLiteral("1d")))); - assertThrows( - SyntaxCheckException.class, - () -> expander.expand(List.of(kv("field", AstDSL.qualifiedName("ts"))))); - assertThrows( SyntaxCheckException.class, () -> @@ -107,12 +106,25 @@ void every_rejection_defers_to_the_legacy_engine() { kv("field", AstDSL.stringLiteral("ts")), kv("fixed_interval", AstDSL.stringLiteral("4d")), kv("alias", AstDSL.stringLiteral("days"))))); + + assertThrows( + SemanticCheckException.class, + () -> expander.expand(List.of(kv("field", AstDSL.stringLiteral("ts"))))); + + assertThrows( + SemanticCheckException.class, + () -> + expander.expand( + List.of( + kv("field", AstDSL.stringLiteral("ts")), + kv("interval", AstDSL.stringLiteral("1d")), + kv("time_zone", AstDSL.stringLiteral("not-an-offset"))))); } @Test void property_bag_rejects_both_interval_and_fixed_interval() { assertThrows( - SyntaxCheckException.class, + SemanticCheckException.class, () -> expander.expand( List.of( @@ -181,9 +193,9 @@ void property_bag_format_and_time_zone_compose() { @Test void property_bag_rejects_invalid_time_zone() { - SyntaxCheckException ex = + SemanticCheckException ex = assertThrows( - SyntaxCheckException.class, + SemanticCheckException.class, () -> expander.expand( List.of( @@ -312,7 +324,7 @@ void property_bag_rejects_unknown_param() { @Test void property_bag_rejects_duplicate_keys() { assertThrows( - SyntaxCheckException.class, + SemanticCheckException.class, () -> expander.expand( List.of( @@ -324,15 +336,15 @@ void property_bag_rejects_duplicate_keys() { @Test void property_bag_rejects_missing_field() { assertThrows( - SyntaxCheckException.class, + SemanticCheckException.class, () -> expander.expand(List.of(kv("interval", AstDSL.stringLiteral("1d"))))); } @Test void property_bag_rejects_when_no_interval_synonym_provided() { - SyntaxCheckException ex = + SemanticCheckException ex = assertThrows( - SyntaxCheckException.class, + SemanticCheckException.class, () -> expander.expand(List.of(kv("field", AstDSL.stringLiteral("ts"))))); assertTrue(ex.getMessage().contains("requires one of")); } diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/HistogramExpanderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/HistogramExpanderTest.java index 7fe6d5e66f0..019318a0dec 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/HistogramExpanderTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/HistogramExpanderTest.java @@ -24,6 +24,7 @@ import org.opensearch.sql.ast.expression.UnresolvedExpression; import org.opensearch.sql.ast.tree.UnresolvedPlan; import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.sql.parser.AstBuilderTestBase; @DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) @@ -186,7 +187,7 @@ void property_bag_rejects_unknown_param() { @Test void property_bag_rejects_duplicate_keys() { assertThrows( - SyntaxCheckException.class, + SemanticCheckException.class, () -> expander.expand( List.of( @@ -198,14 +199,14 @@ void property_bag_rejects_duplicate_keys() { @Test void property_bag_rejects_missing_field() { assertThrows( - SyntaxCheckException.class, + SemanticCheckException.class, () -> expander.expand(List.of(kv("interval", AstDSL.intLiteral(10))))); } @Test void property_bag_rejects_missing_interval() { assertThrows( - SyntaxCheckException.class, + SemanticCheckException.class, () -> expander.expand(List.of(kv("field", AstDSL.stringLiteral("age"))))); } diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/NamedArgumentsTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/NamedArgumentsTest.java index 1d7edb6af5c..1eed9b55bc9 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/NamedArgumentsTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/NamedArgumentsTest.java @@ -21,6 +21,7 @@ import org.opensearch.sql.ast.expression.Literal; import org.opensearch.sql.ast.expression.UnresolvedExpression; import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.exception.SemanticCheckException; @DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) class NamedArgumentsTest { @@ -87,7 +88,7 @@ void a_string_parameter_given_a_column_reference_is_rejected() { NamedArguments bag = NamedArguments.parse(List.of(kv("interval", AstDSL.qualifiedName("not_a_literal")))); - assertThrows(SyntaxCheckException.class, () -> bag.requireStringIfPresent("interval")); + assertThrows(SemanticCheckException.class, () -> bag.requireStringIfPresent("interval")); } @Test @@ -105,9 +106,9 @@ void parse_keeps_keys_in_source_order_and_lower_cases_them() { @Test void parse_rejects_duplicate_keys() { - SyntaxCheckException ex = + SemanticCheckException ex = assertThrows( - SyntaxCheckException.class, + SemanticCheckException.class, () -> NamedArguments.parse( List.of( @@ -119,9 +120,9 @@ void parse_rejects_duplicate_keys() { @Test void parse_rejects_non_key_value_arg_with_clear_message() { UnresolvedExpression bareColumn = AstDSL.qualifiedName("age"); - SyntaxCheckException ex = + SemanticCheckException ex = assertThrows( - SyntaxCheckException.class, + SemanticCheckException.class, () -> NamedArguments.parse(List.of(kv("field", AstDSL.stringLiteral("a")), bareColumn))); assertTrue(ex.getMessage().contains("'key'=value")); @@ -146,8 +147,8 @@ void require_returns_value_and_removes_it() { @Test void require_throws_when_missing_with_function_name_in_message() { NamedArguments bag = NamedArguments.parse(List.of(kv("other", AstDSL.intLiteral(1)))); - SyntaxCheckException ex = - assertThrows(SyntaxCheckException.class, () -> bag.require("field", "HISTOGRAM")); + SemanticCheckException ex = + assertThrows(SemanticCheckException.class, () -> bag.require("field", "HISTOGRAM")); assertTrue(ex.getMessage().contains("histogram")); assertTrue(ex.getMessage().contains("field")); } @@ -162,7 +163,8 @@ void requireString_returns_string_literal() { @Test void requireString_rejects_non_string_value() { NamedArguments bag = NamedArguments.parse(List.of(kv("interval", AstDSL.intLiteral(100)))); - assertThrows(SyntaxCheckException.class, () -> bag.requireString("interval", "date_histogram")); + assertThrows( + SemanticCheckException.class, () -> bag.requireString("interval", "date_histogram")); } @Test @@ -180,7 +182,7 @@ void requireStringIfPresent_returns_value_when_present() { @Test void requireStringIfPresent_rejects_non_string_value_when_present() { NamedArguments bag = NamedArguments.parse(List.of(kv("format", AstDSL.intLiteral(2024)))); - assertThrows(SyntaxCheckException.class, () -> bag.requireStringIfPresent("format")); + assertThrows(SemanticCheckException.class, () -> bag.requireStringIfPresent("format")); } @Test From b954e107e01f788d51504802026249a1a8556144 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Tue, 18 Aug 2026 19:57:58 -0700 Subject: [PATCH 06/18] Handle bucket functions the way this parser handles its other functions Review feedback: the bucket package was a second function and argument resolution path alongside the one already here. It is gone. `histogram` and `date_histogram` now get a `visitBucketFunctionCall` method next to `visitHighlightFunctionCall` and `visitPercentileApproxFunctionCall`, and the grammar carries the argument shape the way `highlightFunction` does: bucketFunction : bucketFunctionName LR_BRACKET bucketArg (COMMA bucketArg)* RR_BRACKET bucketArg : bucketArgName EQUAL_SYMBOL bucketArgValue That deletes NamedArguments outright. It existed to work out which half of a `Function("=", ...)` was the key, which was only necessary because the call went through the generic functionArgs rule; with a rule of its own the parser answers that, and the visitor reads names and values directly. The registry and the expander interface went with it -- one mapped two names, the other had two implementations. The exception split now falls out of the grammar rather than being asserted in code. The positional spelling the legacy engine has always answered no longer matches `bucketArg`, so it stays an unrecognized scalar function and RestSQLQueryAction hands it back, without this code deciding anything. Only parameters that parse but have no lowering here -- alias, min_doc_count, order, which legacy implements -- still need an explicit SyntaxCheckException. Tests moved into AstExpressionBuilderTest alongside the other function-building tests. Net 1228 lines removed. `:sql:build` green including the coverage gate, DateHistogramBucketFunctionIT 10/10, CsvFormatResponseIT 25/25, and the bucket values are unchanged on a live cluster: hourly 12/24/17/19, half-hourly 5/7/11/13/17/19, numeric 19/20/20/13. Signed-off-by: Jialiang Liang --- .../src/main/antlr4/OpenSearchSQLParser.g4 | 19 +- sql/src/main/antlr/OpenSearchSQLParser.g4 | 19 +- .../sql/sql/parser/AstExpressionBuilder.java | 138 ++++++- .../parser/bucket/BucketFunctionExpander.java | 22 - .../parser/bucket/BucketFunctionRegistry.java | 32 -- .../parser/bucket/BucketFunctionUtils.java | 44 -- .../parser/bucket/DateHistogramExpander.java | 135 ------ .../sql/parser/bucket/HistogramExpander.java | 72 ---- .../sql/sql/parser/bucket/NamedArguments.java | 143 ------- .../sql/parser/AstExpressionBuilderTest.java | 112 +++++ .../bucket/BucketFunctionRegistryTest.java | 53 --- .../bucket/BucketFunctionUtilsTest.java | 56 --- .../bucket/DateHistogramExpanderTest.java | 389 ------------------ .../parser/bucket/HistogramExpanderTest.java | 296 ------------- .../sql/parser/bucket/NamedArgumentsTest.java | 250 ----------- 15 files changed, 276 insertions(+), 1504 deletions(-) delete mode 100644 sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionExpander.java delete mode 100644 sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionRegistry.java delete mode 100644 sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionUtils.java delete mode 100644 sql/src/main/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpander.java delete mode 100644 sql/src/main/java/org/opensearch/sql/sql/parser/bucket/HistogramExpander.java delete mode 100644 sql/src/main/java/org/opensearch/sql/sql/parser/bucket/NamedArguments.java delete mode 100644 sql/src/test/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionRegistryTest.java delete mode 100644 sql/src/test/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionUtilsTest.java delete mode 100644 sql/src/test/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpanderTest.java delete mode 100644 sql/src/test/java/org/opensearch/sql/sql/parser/bucket/HistogramExpanderTest.java delete mode 100644 sql/src/test/java/org/opensearch/sql/sql/parser/bucket/NamedArgumentsTest.java diff --git a/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 b/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 index 4a2ab35a89b..5162e6d1e78 100644 --- a/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 +++ b/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 @@ -335,6 +335,7 @@ functionCall | extractFunction # extractFunctionCall | getFormatFunction # getFormatFunctionCall | timestampFunction # timestampFunctionCall + | bucketFunction # bucketFunctionCall ; timestampFunction @@ -396,6 +397,14 @@ highlightFunction : HIGHLIGHT LR_BRACKET relevanceField (COMMA highlightArg)* RR_BRACKET ; +bucketFunction + : bucketFunctionName LR_BRACKET bucketArg (COMMA bucketArg)* RR_BRACKET + ; + +bucketArg + : bucketArgName EQUAL_SYMBOL bucketArgValue + ; + positionFunction : POSITION LR_BRACKET functionArg IN functionArg RR_BRACKET ; @@ -411,7 +420,6 @@ scalarFunctionName | flowControlFunctionName | systemFunctionName | nestedFunctionName - | bucketFunctionName ; bucketFunctionName @@ -762,6 +770,10 @@ highlightArgName | HIGHLIGHT_PRE_TAGS ; +bucketArgName + : stringLiteral + ; + relevanceFieldAndWeight : field = relevanceField | field = relevanceField weight = relevanceFieldWeight @@ -786,6 +798,11 @@ relevanceArgValue | constant ; +bucketArgValue + : constant + | qualifiedName + ; + highlightArgValue : stringLiteral ; diff --git a/sql/src/main/antlr/OpenSearchSQLParser.g4 b/sql/src/main/antlr/OpenSearchSQLParser.g4 index 5029f081b1d..fa0b5b91ea9 100644 --- a/sql/src/main/antlr/OpenSearchSQLParser.g4 +++ b/sql/src/main/antlr/OpenSearchSQLParser.g4 @@ -368,6 +368,7 @@ functionCall | extractFunction # extractFunctionCall | getFormatFunction # getFormatFunctionCall | timestampFunction # timestampFunctionCall + | bucketFunction # bucketFunctionCall ; timestampFunction @@ -429,6 +430,14 @@ highlightFunction : HIGHLIGHT LR_BRACKET relevanceField (COMMA highlightArg)* RR_BRACKET ; +bucketFunction + : bucketFunctionName LR_BRACKET bucketArg (COMMA bucketArg)* RR_BRACKET + ; + +bucketArg + : bucketArgName EQUAL_SYMBOL bucketArgValue + ; + positionFunction : POSITION LR_BRACKET functionArg IN functionArg RR_BRACKET ; @@ -444,7 +453,6 @@ scalarFunctionName | flowControlFunctionName | systemFunctionName | nestedFunctionName - | bucketFunctionName ; bucketFunctionName @@ -795,6 +803,10 @@ highlightArgName | HIGHLIGHT_PRE_TAGS ; +bucketArgName + : stringLiteral + ; + relevanceFieldAndWeight : field = relevanceField | field = relevanceField weight = relevanceFieldWeight @@ -819,6 +831,11 @@ relevanceArgValue | constant ; +bucketArgValue + : constant + | qualifiedName + ; + highlightArgValue : stringLiteral ; diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java index 823e5731a56..c65352335d4 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java @@ -20,6 +20,8 @@ import static org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.BetweenPredicateContext; import static org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.BinaryComparisonPredicateContext; import static org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.BooleanContext; +import static org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.BucketArgContext; +import static org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.BucketFunctionCallContext; import static org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.CaseFuncAlternativeContext; import static org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.CaseFunctionCallContext; import static org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.ColumnFilterContext; @@ -70,14 +72,18 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import java.time.ZoneOffset; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.stream.Collectors; +import java.util.stream.Stream; import org.antlr.v4.runtime.RuleContext; import org.antlr.v4.runtime.tree.ParseTree; import org.antlr.v4.runtime.tree.RuleNode; @@ -89,6 +95,7 @@ import org.opensearch.sql.common.antlr.AstBuildGuard; import org.opensearch.sql.common.antlr.SyntaxCheckException; import org.opensearch.sql.common.utils.StringUtils; +import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.expression.function.BuiltinFunctionName; import org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser; import org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.AlternateMultiMatchQueryContext; @@ -100,8 +107,6 @@ import org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.OrExpressionContext; import org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.TableNameContext; import org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParserBaseVisitor; -import org.opensearch.sql.sql.parser.bucket.BucketFunctionExpander; -import org.opensearch.sql.sql.parser.bucket.BucketFunctionRegistry; /** Expression builder to parse text to expression in AST. */ public class AstExpressionBuilder extends OpenSearchSQLParserBaseVisitor { @@ -164,18 +169,131 @@ public UnresolvedExpression visitNestedAllFunctionCall(NestedAllFunctionCallCont @Override public UnresolvedExpression visitScalarFunctionCall(ScalarFunctionCallContext ctx) { - String functionName = ctx.scalarFunctionName().getText(); - List args = - ctx.functionArgs().functionArg().stream() - .map(this::visitFunctionArg) + return buildFunction(ctx.scalarFunctionName().getText(), ctx.functionArgs().functionArg()); + } + + /** + * Lowers {@code histogram} and {@code date_histogram} to a {@link Span} over the bucketed field. + * The grammar admits only the {@code 'name'=value} form, so the positional spelling the legacy + * engine has always answered never reaches here -- it stays an unknown scalar function, and + * RestSQLQueryAction hands it back to that engine. + */ + @Override + public UnresolvedExpression visitBucketFunctionCall(BucketFunctionCallContext ctx) { + String functionName = + ctx.bucketFunction().bucketFunctionName().getText().toLowerCase(Locale.ROOT); + Map 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) { + throw new SemanticCheckException("Duplicate parameter: " + name); + } + } + + UnresolvedExpression field = requireArg(args, "field", functionName); + UnresolvedExpression missing = args.remove("missing"); + Literal interval = intervalOf(args, functionName); + Literal format = stringArg(args, "format"); + Literal timeZone = stringArg(args, "time_zone"); + + // Anything left is a parameter with no lowering here. Some of them -- alias, min_doc_count, + // order -- are implemented by the legacy engine, so decline in the one way RestSQLQueryAction + // falls back on rather than failing the request outright. + if (!args.isEmpty()) { + throw new SyntaxCheckException( + functionName + " does not accept parameter: " + String.join(", ", args.keySet())); + } + + UnresolvedExpression bucketed = coalesceMissing(normalizeField(field), missing); + if (timeZone != null) { + bucketed = shiftByTimeZone(bucketed, timeZone); + } + Span span = AstDSL.spanFromSpanLengthLiteral(bucketed, interval); + return format == null ? span : new Function("date_format", List.of(span, format)); + } + + private static UnresolvedExpression requireArg( + Map args, String name, String functionName) { + UnresolvedExpression value = args.remove(name); + if (value == null) { + throw new SemanticCheckException(functionName + " requires " + name + " parameter"); + } + return value; + } + + /** + * {@code interval}, {@code fixed_interval} and {@code calendar_interval} are synonyms; exactly + * one must be present. The distinction between calendar and fixed intervals is not preserved. + */ + private static Literal intervalOf(Map args, String functionName) { + List supplied = + Stream.of("interval", "fixed_interval", "calendar_interval") + .map(key -> stringOrNumericArg(args, key)) + .filter(Objects::nonNull) .collect(Collectors.toList()); + if (supplied.isEmpty()) { + throw new SemanticCheckException( + functionName + " requires one of: interval, fixed_interval, calendar_interval"); + } + if (supplied.size() > 1) { + throw new SemanticCheckException( + functionName + " accepts only one of: interval, fixed_interval, calendar_interval"); + } + return supplied.get(0); + } + + private static Literal stringArg(Map 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); + } + return literal; + } + + private static Literal stringOrNumericArg(Map 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); + } + return literal; + } - Optional bucketExpander = BucketFunctionRegistry.lookup(functionName); - if (bucketExpander.isPresent()) { - return bucketExpander.get().expand(args); + /** A string literal naming a column is coerced so downstream sees a column reference. */ + private static UnresolvedExpression normalizeField(UnresolvedExpression field) { + if (field instanceof Literal literal && literal.getType() == DataType.STRING) { + return AstDSL.qualifiedName(literal.getValue().toString()); } + return field; + } + + private static UnresolvedExpression coalesceMissing( + UnresolvedExpression field, UnresolvedExpression missing) { + return missing == null ? field : new Function("coalesce", List.of(field, missing)); + } - return new Function(functionName, args); + /** + * Shifts the field by a {@link ZoneOffset} before bucketing. Validated here so an invalid offset + * is reported rather than surfacing as an arithmetic failure at execution. + */ + private static UnresolvedExpression shiftByTimeZone( + UnresolvedExpression field, Literal timeZone) { + String offset = timeZone.getValue().toString(); + int seconds; + try { + seconds = ZoneOffset.of(offset).getTotalSeconds(); + } catch (RuntimeException e) { + throw new SemanticCheckException( + "time_zone must be a valid offset like '+05:30' or 'Z'; got '" + offset + "'"); + } + return new Function( + "timestampadd", List.of(AstDSL.stringLiteral("SECOND"), AstDSL.intLiteral(seconds), field)); } @Override diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionExpander.java b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionExpander.java deleted file mode 100644 index d6d2dc2283d..00000000000 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionExpander.java +++ /dev/null @@ -1,22 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.sql.parser.bucket; - -import java.util.List; -import org.opensearch.sql.ast.expression.UnresolvedExpression; - -/** - * Parse-time expander for a bucket function call. Each implementation lowers calls to one bucket - * function (e.g. {@code histogram}) into standard SQL constructs the rest of the engine already - * understands. - * - *

Implementations are stateless and registered by name in {@link BucketFunctionRegistry}. - */ -public interface BucketFunctionExpander { - - /** Lowers a bucket function call into its bucket-key expression. */ - UnresolvedExpression expand(List args); -} diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionRegistry.java b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionRegistry.java deleted file mode 100644 index e1471597689..00000000000 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionRegistry.java +++ /dev/null @@ -1,32 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.sql.parser.bucket; - -import java.util.Locale; -import java.util.Map; -import java.util.Optional; - -/** Lookup table mapping bucket-function names to their {@link BucketFunctionExpander}. */ -public final class BucketFunctionRegistry { - - private static final Map EXPANDERS = - Map.of( - HistogramExpander.FUNCTION_NAME, new HistogramExpander(), - DateHistogramExpander.FUNCTION_NAME, new DateHistogramExpander()); - - private BucketFunctionRegistry() {} - - /** - * Returns the expander for {@code functionName} (case-insensitive), or empty if not a bucket - * function. - */ - public static Optional lookup(String functionName) { - if (functionName == null) { - return Optional.empty(); - } - return Optional.ofNullable(EXPANDERS.get(functionName.toUpperCase(Locale.ROOT))); - } -} diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionUtils.java b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionUtils.java deleted file mode 100644 index 850d7ba92be..00000000000 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionUtils.java +++ /dev/null @@ -1,44 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.sql.parser.bucket; - -import java.util.List; -import org.opensearch.sql.ast.dsl.AstDSL; -import org.opensearch.sql.ast.expression.DataType; -import org.opensearch.sql.ast.expression.Function; -import org.opensearch.sql.ast.expression.Literal; -import org.opensearch.sql.ast.expression.QualifiedName; -import org.opensearch.sql.ast.expression.UnresolvedExpression; - -/** - * Shared parameter helpers for bucket-function expanders. Operates on values pulled from a {@link - * NamedArguments} or from a positional argument list. - */ -final class BucketFunctionUtils { - - private BucketFunctionUtils() {} - - /** - * Named-argument form accepts string-literal field names ({@code 'field'='age'}). Coerce them to - * {@link QualifiedName} so downstream sees a column reference regardless of how the user spelled - * it. - */ - static UnresolvedExpression normalizeFieldRef(UnresolvedExpression expr) { - if (expr instanceof Literal lit && lit.getType() == DataType.STRING) { - return AstDSL.qualifiedName(lit.getValue().toString()); - } - return expr; - } - - /** If {@code missingOrNull} is non-null, wrap field with {@code COALESCE(field, missing)}. */ - static UnresolvedExpression applyMissing( - UnresolvedExpression field, UnresolvedExpression missingOrNull) { - if (missingOrNull == null) { - return field; - } - return new Function("coalesce", List.of(field, missingOrNull)); - } -} diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpander.java b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpander.java deleted file mode 100644 index 82be57dc9f3..00000000000 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpander.java +++ /dev/null @@ -1,135 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.sql.parser.bucket; - -import static org.opensearch.sql.sql.parser.bucket.BucketFunctionUtils.applyMissing; -import static org.opensearch.sql.sql.parser.bucket.BucketFunctionUtils.normalizeFieldRef; - -import java.time.ZoneOffset; -import java.util.List; -import java.util.Objects; -import java.util.stream.Stream; -import org.opensearch.sql.ast.dsl.AstDSL; -import org.opensearch.sql.ast.expression.Function; -import org.opensearch.sql.ast.expression.Literal; -import org.opensearch.sql.ast.expression.Span; -import org.opensearch.sql.ast.expression.UnresolvedExpression; -import org.opensearch.sql.common.antlr.SyntaxCheckException; -import org.opensearch.sql.exception.SemanticCheckException; - -/** - * Lowers {@code date_histogram(...)} calls to a {@link Span} expression with the time unit inferred - * from the interval string. Optional parameters wrap the bucket key: - * - *

    - *
  • {@code missing} — wraps the field with {@code COALESCE(field, missing)} before bucketing. - *
  • {@code time_zone} — shifts the field with {@code TIMESTAMPADD(SECOND, offset, field)} - * before bucketing. Validated as a {@link java.time.ZoneOffset} at parse time. - *
  • {@code format} — wraps the bucket with {@code DATE_FORMAT(span, format)}. - *
- * - *

{@code interval}, {@code fixed_interval}, and {@code calendar_interval} are accepted as - * mutually-exclusive syntactic synonyms; this lowering does not preserve the calendar-vs-fixed - * distinction across them. - * - *

TODO: V1 also accepts the following parameters; they are currently rejected: - * - *

    - *
  • {@code min_doc_count} — would lower to {@code HAVING COUNT(*) >= N}. Needs parser-side - * plumbing to inject a HAVING clause from inside a scalar function call. - *
  • {@code order} — would lower to {@code ORDER BY}. Same plumbing requirement as above. - *
  • {@code alias} — would set the surrounding SELECT-list alias. Needs reaching outside the - * function call to mutate the parent SELECT element. - *
  • {@code offset} — would shift bucket boundaries via {@code TIMESTAMPADD(SECOND, -offset, - * field)} before bucketing and {@code TIMESTAMPADD(SECOND, offset, span)} after. Needs a - * duration-string parser ({@code '1h'}, {@code '2d'}, etc.) distinct from {@code time_zone}'s - * {@code ZoneOffset} format. - *
- */ -final class DateHistogramExpander implements BucketFunctionExpander { - - static final String FUNCTION_NAME = "DATE_HISTOGRAM"; - - @Override - public UnresolvedExpression expand(List args) { - if (!NamedArguments.isNamedArguments(args)) { - // SyntaxCheckException is the only type RestSQLQueryAction falls back on, so an - // unrecognized shape keeps reaching the legacy engine that has always served it. - throw new SyntaxCheckException( - "date_histogram requires named arguments: date_histogram('field'=," - + " 'interval'=)"); - } - NamedArguments named = NamedArguments.parse(args); - UnresolvedExpression field = named.require("field", FUNCTION_NAME); - Literal intervalLiteral = extractIntervalLiteral(named); - Literal formatLiteral = named.requireStringIfPresent("format"); - Literal timeZoneLiteral = named.requireStringIfPresent("time_zone"); - UnresolvedExpression missing = named.remove("missing"); - named.rejectRemaining(FUNCTION_NAME); - return buildBucket(field, intervalLiteral, formatLiteral, timeZoneLiteral, missing); - } - - /** - * Pulls the interval from the named arguments accepting any of {@code interval}, {@code - * fixed_interval}, {@code calendar_interval}. Exactly one must be present. - */ - private static Literal extractIntervalLiteral(NamedArguments named) { - Literal interval = named.requireStringIfPresent("interval"); - Literal fixedInterval = named.requireStringIfPresent("fixed_interval"); - Literal calendarInterval = named.requireStringIfPresent("calendar_interval"); - - List suppliedIntervals = - Stream.of(interval, fixedInterval, calendarInterval).filter(Objects::nonNull).toList(); - - if (suppliedIntervals.isEmpty()) { - throw new SemanticCheckException( - "date_histogram requires one of: interval, fixed_interval, calendar_interval"); - } - if (suppliedIntervals.size() > 1) { - throw new SemanticCheckException( - "date_histogram accepts only one of: interval, fixed_interval, calendar_interval"); - } - return suppliedIntervals.get(0); - } - - private static UnresolvedExpression buildBucket( - UnresolvedExpression field, - Literal intervalLiteral, - Literal formatLiteral, - Literal timeZoneLiteral, - UnresolvedExpression missingOrNull) { - UnresolvedExpression resolvedField = applyMissing(normalizeFieldRef(field), missingOrNull); - UnresolvedExpression shiftedField = - timeZoneLiteral != null - ? applyTimeZoneShift(resolvedField, timeZoneLiteral) - : resolvedField; - Span span = AstDSL.spanFromSpanLengthLiteral(shiftedField, intervalLiteral); - if (formatLiteral == null) { - return span; - } - return new Function("date_format", List.of(span, formatLiteral)); - } - - /** - * Wraps the field with a {@code TIMESTAMPADD(SECOND, offset, field)} shift derived from a - * timezone literal. Validates the literal at parse time as a {@link ZoneOffset} (e.g. {@code - * '+05:30'}, {@code 'Z'}); runtime arithmetic is plain second addition. - */ - private static UnresolvedExpression applyTimeZoneShift( - UnresolvedExpression field, Literal timeZoneLiteral) { - String tzString = timeZoneLiteral.getValue().toString(); - int offsetSeconds; - try { - offsetSeconds = ZoneOffset.of(tzString).getTotalSeconds(); - } catch (RuntimeException ex) { - throw new SemanticCheckException( - "time_zone must be a valid offset like '+05:30' or 'Z'; got '" + tzString + "'"); - } - return new Function( - "timestampadd", - List.of(AstDSL.stringLiteral("SECOND"), AstDSL.intLiteral(offsetSeconds), field)); - } -} diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/HistogramExpander.java b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/HistogramExpander.java deleted file mode 100644 index a5b1a6a71ca..00000000000 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/HistogramExpander.java +++ /dev/null @@ -1,72 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.sql.parser.bucket; - -import static org.opensearch.sql.sql.parser.bucket.BucketFunctionUtils.applyMissing; -import static org.opensearch.sql.sql.parser.bucket.BucketFunctionUtils.normalizeFieldRef; - -import java.util.List; -import org.opensearch.sql.ast.dsl.AstDSL; -import org.opensearch.sql.ast.expression.Function; -import org.opensearch.sql.ast.expression.Span; -import org.opensearch.sql.ast.expression.SpanUnit; -import org.opensearch.sql.ast.expression.UnresolvedExpression; -import org.opensearch.sql.common.antlr.SyntaxCheckException; - -/** - * Lowers {@code histogram(...)} calls to a {@link Span} expression with {@code SpanUnit.NONE}. - * Optional parameters wrap the bucket key: - * - *
    - *
  • {@code missing} — wraps the field with {@code COALESCE(field, missing)} before bucketing. - *
  • {@code offset} — wraps as {@code +(Span(-(field, offset), interval, NONE), offset)} to - * preserve the standard {@code [k*interval+offset, (k+1)*interval+offset)} boundaries. - *
- * - *

TODO: V1 also accepts the following parameters; they are currently rejected: - * - *

    - *
  • {@code min_doc_count} — would lower to {@code HAVING COUNT(*) >= N}. Needs parser-side - * plumbing to inject a HAVING clause from inside a scalar function call. - *
  • {@code order} — would lower to {@code ORDER BY}. Same plumbing requirement as above. - *
  • {@code alias} — would set the surrounding SELECT-list alias. Needs reaching outside the - * function call to mutate the parent SELECT element. - *
- */ -final class HistogramExpander implements BucketFunctionExpander { - - static final String FUNCTION_NAME = "HISTOGRAM"; - - @Override - public UnresolvedExpression expand(List args) { - if (!NamedArguments.isNamedArguments(args)) { - // See DateHistogramExpander: this type is what allows the legacy fallback. - throw new SyntaxCheckException( - "histogram requires named arguments: histogram('field'=, 'interval'=)"); - } - NamedArguments named = NamedArguments.parse(args); - UnresolvedExpression field = named.require("field", FUNCTION_NAME); - UnresolvedExpression interval = named.require("interval", FUNCTION_NAME); - UnresolvedExpression offset = named.remove("offset"); - UnresolvedExpression missing = named.remove("missing"); - named.rejectRemaining(FUNCTION_NAME); - return buildBucket(field, interval, offset, missing); - } - - private static UnresolvedExpression buildBucket( - UnresolvedExpression field, - UnresolvedExpression interval, - UnresolvedExpression offsetOrNull, - UnresolvedExpression missingOrNull) { - UnresolvedExpression resolvedField = applyMissing(normalizeFieldRef(field), missingOrNull); - if (offsetOrNull == null) { - return AstDSL.span(resolvedField, interval, SpanUnit.NONE); - } - UnresolvedExpression shifted = new Function("-", List.of(resolvedField, offsetOrNull)); - Span bucket = (Span) AstDSL.span(shifted, interval, SpanUnit.NONE); - return new Function("+", List.of(bucket, offsetOrNull)); - } -} diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/NamedArguments.java b/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/NamedArguments.java deleted file mode 100644 index 4f10a68f8a8..00000000000 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/bucket/NamedArguments.java +++ /dev/null @@ -1,143 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.sql.parser.bucket; - -import java.util.LinkedHashMap; -import java.util.List; -import java.util.Locale; -import java.util.Map; -import java.util.Set; -import org.opensearch.sql.ast.expression.DataType; -import org.opensearch.sql.ast.expression.Function; -import org.opensearch.sql.ast.expression.Literal; -import org.opensearch.sql.ast.expression.UnresolvedExpression; -import org.opensearch.sql.common.antlr.SyntaxCheckException; -import org.opensearch.sql.exception.SemanticCheckException; - -/** - * Parses and validates named-argument style function arguments. The arg shape is {@code - * Function("=", [StringLiteral(key), value])} — what ANTLR produces for {@code 'key'=value}. Keys - * are lower-cased on parse; iteration order matches source order. - * - *

Drain semantics. Every extraction method ({@code require}, {@code remove}, {@code - * requireString}, {@code requireStringIfPresent}, {@code rejectIfPresent}, {@code consumeSilently}) - * removes its key from the collection. After the caller has extracted everything it recognizes, - * {@code rejectRemaining} sweeps what is left and treats those keys as unknown parameters — so - * extracted keys must drain out, otherwise they would be re-rejected. - */ -public final class NamedArguments { - - private final Map arguments; - - private NamedArguments(Map arguments) { - this.arguments = arguments; - } - - /** True iff every arg is a {@code 'key'=value} key-value pair. Empty list returns false. */ - public static boolean isNamedArguments(List args) { - if (args.isEmpty()) { - return false; - } - return args.stream().allMatch(NamedArguments::isKeyValuePair); - } - - private static boolean isKeyValuePair(UnresolvedExpression arg) { - if (!(arg instanceof Function fn) || !"=".equals(fn.getFuncName())) { - return false; - } - if (fn.getFuncArgs().size() != 2) { - return false; - } - return fn.getFuncArgs().get(0) instanceof Literal keyLiteral - && keyLiteral.getType() == DataType.STRING; - } - - /** - * Parses the given args into a {@code NamedArguments}. Each arg must match the {@code - * 'key'=value} shape — a non-matching arg raises {@link SyntaxCheckException}. Duplicate keys - * also raise {@link SyntaxCheckException}. - */ - public static NamedArguments parse(List args) { - Map arguments = new LinkedHashMap<>(); - for (UnresolvedExpression arg : args) { - if (!isKeyValuePair(arg)) { - throw new SemanticCheckException("Named arguments must be of form 'key'=value; got " + arg); - } - Function fn = (Function) arg; - Literal keyLiteral = (Literal) fn.getFuncArgs().get(0); - String key = keyLiteral.getValue().toString().toLowerCase(Locale.ROOT); - UnresolvedExpression value = fn.getFuncArgs().get(1); - if (arguments.put(key, value) != null) { - throw new SemanticCheckException("Duplicate parameter: " + key); - } - } - return new NamedArguments(arguments); - } - - /** Removes and returns the value for {@code key}, or {@code null} if not present. */ - public UnresolvedExpression remove(String key) { - return arguments.remove(key); - } - - /** Removes and returns the value for {@code key}; throws if absent. */ - public UnresolvedExpression require(String key, String funcName) { - UnresolvedExpression value = arguments.remove(key); - if (value == null) { - throw new SemanticCheckException( - funcName.toLowerCase(Locale.ROOT) + " requires " + key + " parameter"); - } - return value; - } - - /** As {@link #require}, additionally enforcing string-literal type. */ - public Literal requireString(String key, String funcName) { - return asStringLiteral(require(key, funcName), key); - } - - /** As {@link #remove}, additionally enforcing string-literal type when present. */ - public Literal requireStringIfPresent(String key) { - UnresolvedExpression value = arguments.remove(key); - return value == null ? null : asStringLiteral(value, key); - } - - private static Literal asStringLiteral(UnresolvedExpression expr, String paramName) { - if (!(expr instanceof Literal literal) || literal.getType() != DataType.STRING) { - throw new SemanticCheckException( - paramName + " must be a string literal (e.g. '1d', '15m'); got " + expr); - } - return literal; - } - - /** If {@code key} is present, throws with the supplied message; otherwise no-op. */ - public void rejectIfPresent(String key, String message) { - if (arguments.remove(key) != null) { - throw new SyntaxCheckException(message); - } - } - - /** Drops the listed keys without inspecting their values. */ - public void consumeSilently(Set keys) { - for (String key : keys) { - arguments.remove(key); - } - } - - /** Treats any keys still remaining as unsupported parameters. Call last. */ - public void rejectRemaining(String funcName) { - if (arguments.isEmpty()) { - return; - } - String label = arguments.size() == 1 ? "parameter" : "parameters"; - String unsupported = String.join(", ", arguments.keySet()); - throw new SyntaxCheckException( - funcName.toLowerCase(Locale.ROOT) + " does not accept " + label + ": " + unsupported); - } - - /** Number of unconsumed keys. Primarily for tests. */ - int size() { - return arguments.size(); - } -} diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java index aba8023b07e..51f919001a7 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java @@ -47,12 +47,16 @@ import org.opensearch.sql.ast.expression.DataType; import org.opensearch.sql.ast.expression.Literal; import org.opensearch.sql.ast.expression.RelevanceFieldList; +import org.opensearch.sql.ast.expression.Span; +import org.opensearch.sql.ast.expression.SpanUnit; import org.opensearch.sql.ast.expression.WindowFrame; import org.opensearch.sql.ast.expression.WindowFunction; import org.opensearch.sql.ast.tree.Sort.SortOption; import org.opensearch.sql.common.antlr.AstBuildGuard; import org.opensearch.sql.common.antlr.CaseInsensitiveCharStream; import org.opensearch.sql.common.antlr.SyntaxAnalysisErrorListener; +import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.sql.antlr.parser.OpenSearchSQLLexer; import org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser; @@ -860,6 +864,114 @@ private static String nest(int depth, String base, UnaryOperator wrap) { return expr; } + @Test + public void canBuildDateHistogramAsSpan() { + assertEquals( + new Span(qualifiedName("ts"), intLiteral(1), SpanUnit.H), + buildExprAst("date_histogram('field'=ts, 'interval'='1h')")); + } + + @Test + public void canBuildDateHistogramWithIntervalSynonyms() { + Span expected = new Span(qualifiedName("ts"), intLiteral(1), SpanUnit.D); + assertEquals(expected, buildExprAst("date_histogram('field'=ts, 'fixed_interval'='1d')")); + assertEquals(expected, buildExprAst("date_histogram('field'=ts, 'calendar_interval'='1d')")); + } + + @Test + public void canBuildDateHistogramWithStringFieldName() { + assertEquals( + new Span(qualifiedName("ts"), intLiteral(30), SpanUnit.m), + buildExprAst("date_histogram('field'='ts', 'interval'='30m')")); + } + + /** A numeric literal field is left alone rather than coerced to a column reference. */ + @Test + public void bucketFieldGivenNonStringLiteralIsPassedThrough() { + assertEquals( + new Span(intLiteral(1), intLiteral(10), SpanUnit.NONE), + buildExprAst("histogram('field'=1, 'interval'=10)")); + } + + @Test + public void canBuildNumericHistogramAsSpan() { + assertEquals( + new Span(qualifiedName("age"), intLiteral(10), SpanUnit.NONE), + buildExprAst("histogram('field'=age, 'interval'=10)")); + } + + @Test + public void canBuildDateHistogramWithMissing() { + assertEquals( + new Span( + function("coalesce", qualifiedName("ts"), stringLiteral("1970-01-01")), + intLiteral(1), + SpanUnit.H), + buildExprAst("date_histogram('field'=ts, 'interval'='1h', 'missing'='1970-01-01')")); + } + + @Test + public void canBuildDateHistogramWithTimeZoneShift() { + assertEquals( + new Span( + function( + "timestampadd", stringLiteral("SECOND"), intLiteral(19800), qualifiedName("ts")), + intLiteral(1), + SpanUnit.H), + buildExprAst("date_histogram('field'=ts, 'interval'='1h', 'time_zone'='+05:30')")); + } + + @Test + public void canBuildDateHistogramWithFormat() { + assertEquals( + function( + "date_format", + new Span(qualifiedName("ts"), intLiteral(1), SpanUnit.D), + stringLiteral("yyyy-MM-dd")), + buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'format'='yyyy-MM-dd')")); + } + + /** + * A parameter with no lowering here has to raise SyntaxCheckException -- the one type + * RestSQLQueryAction falls back on -- because the legacy engine implements alias, min_doc_count + * and order, and has answered queries using them for years. + */ + @Test + public void unsupportedBucketParameterDefersToLegacyEngine() { + assertThrows( + SyntaxCheckException.class, + () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'alias'='days')")); + assertThrows( + SyntaxCheckException.class, + () -> buildExprAst("histogram('field'=age, 'interval'=10, 'min_doc_count'=1)")); + } + + /** A bad argument inside a shape we own must not fall back, so the caller sees this message. */ + @Test + public void badBucketArgumentIsReportedRatherThanDeferred() { + assertThrows( + SemanticCheckException.class, () -> buildExprAst("date_histogram('interval'='1d')")); + assertThrows(SemanticCheckException.class, () -> buildExprAst("date_histogram('field'=ts)")); + assertThrows( + SemanticCheckException.class, + () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'fixed_interval'='2d')")); + assertThrows( + SemanticCheckException.class, + () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'time_zone'='nope')")); + assertThrows( + SemanticCheckException.class, + () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'format'=7)")); + assertThrows( + SemanticCheckException.class, + () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'format'=other)")); + assertThrows( + SemanticCheckException.class, + () -> buildExprAst("date_histogram('field'=ts, 'interval'=ts)")); + assertThrows( + SemanticCheckException.class, + () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'interval'='2d')")); + } + private Node buildExprAst(String expr) { return buildExprAst(expr, astExprBuilder); } diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionRegistryTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionRegistryTest.java deleted file mode 100644 index 9dc5acf2572..00000000000 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionRegistryTest.java +++ /dev/null @@ -1,53 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.sql.parser.bucket; - -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertInstanceOf; -import static org.junit.jupiter.api.Assertions.assertTrue; - -import java.util.Optional; -import org.junit.jupiter.api.DisplayNameGeneration; -import org.junit.jupiter.api.DisplayNameGenerator; -import org.junit.jupiter.api.Test; - -@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) -class BucketFunctionRegistryTest { - - @Test - void lookup_returns_HistogramExpander_for_HISTOGRAM() { - Optional expander = BucketFunctionRegistry.lookup("HISTOGRAM"); - assertTrue(expander.isPresent()); - assertInstanceOf(HistogramExpander.class, expander.get()); - } - - @Test - void lookup_returns_DateHistogramExpander_for_DATE_HISTOGRAM() { - Optional expander = BucketFunctionRegistry.lookup("DATE_HISTOGRAM"); - assertTrue(expander.isPresent()); - assertInstanceOf(DateHistogramExpander.class, expander.get()); - } - - @Test - void lookup_is_case_insensitive() { - assertTrue(BucketFunctionRegistry.lookup("histogram").isPresent()); - assertTrue(BucketFunctionRegistry.lookup("Histogram").isPresent()); - assertTrue(BucketFunctionRegistry.lookup("date_histogram").isPresent()); - assertTrue(BucketFunctionRegistry.lookup("Date_Histogram").isPresent()); - } - - @Test - void lookup_returns_empty_for_unknown_function() { - assertFalse(BucketFunctionRegistry.lookup("range").isPresent()); - assertFalse(BucketFunctionRegistry.lookup("SUM").isPresent()); - assertFalse(BucketFunctionRegistry.lookup("FLOOR").isPresent()); - } - - @Test - void lookup_returns_empty_for_null() { - assertFalse(BucketFunctionRegistry.lookup(null).isPresent()); - } -} diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionUtilsTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionUtilsTest.java deleted file mode 100644 index b211a6362ad..00000000000 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/BucketFunctionUtilsTest.java +++ /dev/null @@ -1,56 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.sql.parser.bucket; - -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertSame; - -import java.util.List; -import org.junit.jupiter.api.DisplayNameGeneration; -import org.junit.jupiter.api.DisplayNameGenerator; -import org.junit.jupiter.api.Test; -import org.opensearch.sql.ast.dsl.AstDSL; -import org.opensearch.sql.ast.expression.Function; -import org.opensearch.sql.ast.expression.QualifiedName; -import org.opensearch.sql.ast.expression.UnresolvedExpression; - -@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) -class BucketFunctionUtilsTest { - - @Test - void normalizeFieldRef_string_literal_becomes_qualified_name() { - UnresolvedExpression result = - BucketFunctionUtils.normalizeFieldRef(AstDSL.stringLiteral("age")); - assertEquals(AstDSL.qualifiedName("age"), result); - } - - @Test - void normalizeFieldRef_qualified_name_passes_through_unchanged() { - QualifiedName input = AstDSL.qualifiedName("age"); - assertSame(input, BucketFunctionUtils.normalizeFieldRef(input)); - } - - @Test - void normalizeFieldRef_non_string_literal_passes_through_unchanged() { - UnresolvedExpression input = AstDSL.intLiteral(1); - assertSame(input, BucketFunctionUtils.normalizeFieldRef(input)); - } - - @Test - void applyMissing_null_returns_field_unchanged() { - QualifiedName field = AstDSL.qualifiedName("age"); - assertSame(field, BucketFunctionUtils.applyMissing(field, null)); - } - - @Test - void applyMissing_non_null_wraps_with_coalesce() { - QualifiedName field = AstDSL.qualifiedName("age"); - UnresolvedExpression missing = AstDSL.intLiteral(0); - assertEquals( - new Function("coalesce", List.of(field, missing)), - BucketFunctionUtils.applyMissing(field, missing)); - } -} diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpanderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpanderTest.java deleted file mode 100644 index 9c1753da07e..00000000000 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/DateHistogramExpanderTest.java +++ /dev/null @@ -1,389 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.sql.parser.bucket; - -import static java.util.Collections.emptyList; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; - -import com.google.common.collect.ImmutableList; -import java.util.List; -import org.junit.jupiter.api.DisplayNameGeneration; -import org.junit.jupiter.api.DisplayNameGenerator; -import org.junit.jupiter.api.Test; -import org.opensearch.sql.ast.dsl.AstDSL; -import org.opensearch.sql.ast.expression.AllFields; -import org.opensearch.sql.ast.expression.Function; -import org.opensearch.sql.ast.expression.QualifiedName; -import org.opensearch.sql.ast.expression.Span; -import org.opensearch.sql.ast.expression.SpanUnit; -import org.opensearch.sql.ast.expression.UnresolvedExpression; -import org.opensearch.sql.ast.tree.UnresolvedPlan; -import org.opensearch.sql.common.antlr.SyntaxCheckException; -import org.opensearch.sql.exception.SemanticCheckException; -import org.opensearch.sql.sql.parser.AstBuilderTestBase; - -@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) -class DateHistogramExpanderTest extends AstBuilderTestBase { - - private final DateHistogramExpander expander = new DateHistogramExpander(); - - @Test - void rejects_positional_invocation_with_clear_message() { - SyntaxCheckException ex = - assertThrows( - SyntaxCheckException.class, - () -> expander.expand(List.of(AstDSL.qualifiedName("ts"), AstDSL.stringLiteral("1d")))); - assertTrue(ex.getMessage().contains("named arguments")); - assertTrue(ex.getMessage().contains("date_histogram")); - } - - @Test - void property_bag_with_interval_param_lowers_to_span() { - UnresolvedExpression result = - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("ts")), - kv("interval", AstDSL.stringLiteral("1d")))); - - assertEquals(new Span(AstDSL.qualifiedName("ts"), AstDSL.intLiteral(1), SpanUnit.D), result); - } - - @Test - void property_bag_with_qualified_name_field_passes_through_unchanged() { - QualifiedName ts = AstDSL.qualifiedName("ts"); - UnresolvedExpression result = - expander.expand(List.of(kv("field", ts), kv("interval", AstDSL.stringLiteral("1d")))); - - assertEquals(new Span(ts, AstDSL.intLiteral(1), SpanUnit.D), result); - } - - @Test - void property_bag_with_fixed_interval_param_lowers_to_span() { - UnresolvedExpression result = - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("ts")), - kv("fixed_interval", AstDSL.stringLiteral("15m")))); - - assertEquals(new Span(AstDSL.qualifiedName("ts"), AstDSL.intLiteral(15), SpanUnit.m), result); - } - - @Test - void property_bag_with_calendar_interval_param_lowers_to_span() { - UnresolvedExpression result = - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("ts")), - kv("calendar_interval", AstDSL.stringLiteral("1d")))); - - assertEquals(new Span(AstDSL.qualifiedName("ts"), AstDSL.intLiteral(1), SpanUnit.D), result); - } - - /** - * The split matters. A call shape this expander does not own has to raise SyntaxCheckException, - * the one type RestSQLQueryAction falls back on, so the legacy engine keeps answering the - * positional form and parameters like `alias` that it implements and this one does not. A bad - * argument inside a shape we do own raises SemanticCheckException instead, so the caller gets - * this message rather than an opaque legacy parser error -- the same choice - * AstBuilder.visitTableFunctionRelation makes. - */ - @Test - void unowned_shapes_defer_but_bad_arguments_do_not() { - assertThrows( - SyntaxCheckException.class, - () -> expander.expand(List.of(AstDSL.qualifiedName("ts"), AstDSL.stringLiteral("1d")))); - - assertThrows( - SyntaxCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("ts")), - kv("fixed_interval", AstDSL.stringLiteral("4d")), - kv("alias", AstDSL.stringLiteral("days"))))); - - assertThrows( - SemanticCheckException.class, - () -> expander.expand(List.of(kv("field", AstDSL.stringLiteral("ts"))))); - - assertThrows( - SemanticCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("ts")), - kv("interval", AstDSL.stringLiteral("1d")), - kv("time_zone", AstDSL.stringLiteral("not-an-offset"))))); - } - - @Test - void property_bag_rejects_both_interval_and_fixed_interval() { - assertThrows( - SemanticCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("ts")), - kv("interval", AstDSL.stringLiteral("1d")), - kv("fixed_interval", AstDSL.stringLiteral("15m"))))); - } - - @Test - void property_bag_format_wraps_with_date_format() { - UnresolvedExpression result = - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("ts")), - kv("interval", AstDSL.stringLiteral("1d")), - kv("format", AstDSL.stringLiteral("yyyy-MM-dd")))); - - Span innerSpan = new Span(AstDSL.qualifiedName("ts"), AstDSL.intLiteral(1), SpanUnit.D); - Function expected = - new Function("date_format", List.of(innerSpan, AstDSL.stringLiteral("yyyy-MM-dd"))); - assertEquals(expected, result); - } - - @Test - void property_bag_time_zone_wraps_field_with_timestampadd() { - UnresolvedExpression result = - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("ts")), - kv("interval", AstDSL.stringLiteral("1d")), - kv("time_zone", AstDSL.stringLiteral("+05:30")))); - - // +05:30 = 5*3600 + 30*60 = 19800 seconds - Function shiftedField = - new Function( - "timestampadd", - List.of( - AstDSL.stringLiteral("SECOND"), - AstDSL.intLiteral(19800), - AstDSL.qualifiedName("ts"))); - Span expected = new Span(shiftedField, AstDSL.intLiteral(1), SpanUnit.D); - assertEquals(expected, result); - } - - @Test - void property_bag_format_and_time_zone_compose() { - UnresolvedExpression result = - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("ts")), - kv("interval", AstDSL.stringLiteral("1d")), - kv("format", AstDSL.stringLiteral("yyyy")), - kv("time_zone", AstDSL.stringLiteral("Z")))); - - // Z = 0 offset - Function shiftedField = - new Function( - "timestampadd", - List.of( - AstDSL.stringLiteral("SECOND"), AstDSL.intLiteral(0), AstDSL.qualifiedName("ts"))); - Span innerSpan = new Span(shiftedField, AstDSL.intLiteral(1), SpanUnit.D); - Function expected = - new Function("date_format", List.of(innerSpan, AstDSL.stringLiteral("yyyy"))); - assertEquals(expected, result); - } - - @Test - void property_bag_rejects_invalid_time_zone() { - SemanticCheckException ex = - assertThrows( - SemanticCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("ts")), - kv("interval", AstDSL.stringLiteral("1d")), - kv("time_zone", AstDSL.stringLiteral("not-a-tz"))))); - assertTrue(ex.getMessage().contains("time_zone")); - } - - @Test - void property_bag_rejects_alias() { - assertThrows( - SyntaxCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("ts")), - kv("interval", AstDSL.stringLiteral("1d")), - kv("alias", AstDSL.stringLiteral("my_label"))))); - } - - @Test - void property_bag_rejects_nested() { - assertThrows( - SyntaxCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("ts")), - kv("interval", AstDSL.stringLiteral("1d")), - kv("nested", AstDSL.stringLiteral("path"))))); - } - - @Test - void property_bag_rejects_reverse_nested() { - assertThrows( - SyntaxCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("ts")), - kv("interval", AstDSL.stringLiteral("1d")), - kv("reverse_nested", AstDSL.stringLiteral("path"))))); - } - - @Test - void property_bag_rejects_children() { - assertThrows( - SyntaxCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("ts")), - kv("interval", AstDSL.stringLiteral("1d")), - kv("children", AstDSL.stringLiteral("ignored"))))); - } - - @Test - void property_bag_missing_wraps_field_with_coalesce() { - UnresolvedExpression result = - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("ts")), - kv("interval", AstDSL.stringLiteral("1d")), - kv("missing", AstDSL.stringLiteral("2024-01-01")))); - - Function coalesced = - new Function( - "coalesce", List.of(AstDSL.qualifiedName("ts"), AstDSL.stringLiteral("2024-01-01"))); - assertEquals(new Span(coalesced, AstDSL.intLiteral(1), SpanUnit.D), result); - } - - @Test - void property_bag_rejects_offset() { - SyntaxCheckException ex = - assertThrows( - SyntaxCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("ts")), - kv("interval", AstDSL.stringLiteral("1d")), - kv("offset", AstDSL.stringLiteral("1h"))))); - assertTrue(ex.getMessage().contains("offset")); - assertTrue(ex.getMessage().contains("does not accept")); - } - - @Test - void property_bag_rejects_min_doc_count() { - assertThrows( - SyntaxCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("ts")), - kv("interval", AstDSL.stringLiteral("1d")), - kv("min_doc_count", AstDSL.intLiteral(5))))); - } - - @Test - void property_bag_rejects_extended_bounds() { - assertThrows( - SyntaxCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("ts")), - kv("interval", AstDSL.stringLiteral("1d")), - kv("extended_bounds", AstDSL.stringLiteral("a:b"))))); - } - - @Test - void property_bag_rejects_unknown_param() { - SyntaxCheckException ex = - assertThrows( - SyntaxCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("ts")), - kv("interval", AstDSL.stringLiteral("1d")), - kv("missing_param", AstDSL.stringLiteral("foo"))))); - assertTrue(ex.getMessage().contains("missing_param")); - } - - @Test - void property_bag_rejects_duplicate_keys() { - assertThrows( - SemanticCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("ts")), - kv("field", AstDSL.stringLiteral("created_at")), - kv("interval", AstDSL.stringLiteral("1d"))))); - } - - @Test - void property_bag_rejects_missing_field() { - assertThrows( - SemanticCheckException.class, - () -> expander.expand(List.of(kv("interval", AstDSL.stringLiteral("1d"))))); - } - - @Test - void property_bag_rejects_when_no_interval_synonym_provided() { - SemanticCheckException ex = - assertThrows( - SemanticCheckException.class, - () -> expander.expand(List.of(kv("field", AstDSL.stringLiteral("ts"))))); - assertTrue(ex.getMessage().contains("requires one of")); - } - - @Test - void via_sql_with_interval_param_lowers_to_span() { - QualifiedName ts = AstDSL.qualifiedName("ts"); - Span bucket = AstDSL.span(ts, AstDSL.intLiteral(1), SpanUnit.D); - - UnresolvedPlan result = - buildAST( - "SELECT date_histogram('field'='ts', 'interval'='1d'), COUNT(*) FROM events " - + "GROUP BY date_histogram('field'='ts', 'interval'='1d')"); - - assertEquals( - AstDSL.project( - AstDSL.agg( - AstDSL.relation("events"), - ImmutableList.of( - AstDSL.alias("COUNT(*)", AstDSL.aggregate("COUNT", AllFields.of()))), - emptyList(), - ImmutableList.of(AstDSL.alias(bucket.toString(), bucket)), - emptyList()), - AstDSL.alias("date_histogram('field'='ts', 'interval'='1d')", bucket), - AstDSL.alias("COUNT(*)", AstDSL.aggregate("COUNT", AllFields.of()))), - result); - } - - @Test - void via_sql_rejects_positional_invocation() { - assertThrows( - SyntaxCheckException.class, - () -> - buildAST( - "SELECT date_histogram(ts, '1d') FROM events GROUP BY date_histogram(ts, '1d')")); - } - - /** Builds a Function("=", [stringLiteral(key), value]) — same shape ANTLR produces for 'k'=v. */ - private static UnresolvedExpression kv(String key, UnresolvedExpression value) { - return new Function("=", List.of(AstDSL.stringLiteral(key), value)); - } -} diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/HistogramExpanderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/HistogramExpanderTest.java deleted file mode 100644 index 019318a0dec..00000000000 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/HistogramExpanderTest.java +++ /dev/null @@ -1,296 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.sql.parser.bucket; - -import static java.util.Collections.emptyList; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; - -import com.google.common.collect.ImmutableList; -import java.util.List; -import org.junit.jupiter.api.DisplayNameGeneration; -import org.junit.jupiter.api.DisplayNameGenerator; -import org.junit.jupiter.api.Test; -import org.opensearch.sql.ast.dsl.AstDSL; -import org.opensearch.sql.ast.expression.AllFields; -import org.opensearch.sql.ast.expression.Function; -import org.opensearch.sql.ast.expression.QualifiedName; -import org.opensearch.sql.ast.expression.Span; -import org.opensearch.sql.ast.expression.SpanUnit; -import org.opensearch.sql.ast.expression.UnresolvedExpression; -import org.opensearch.sql.ast.tree.UnresolvedPlan; -import org.opensearch.sql.common.antlr.SyntaxCheckException; -import org.opensearch.sql.exception.SemanticCheckException; -import org.opensearch.sql.sql.parser.AstBuilderTestBase; - -@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) -class HistogramExpanderTest extends AstBuilderTestBase { - - private final HistogramExpander expander = new HistogramExpander(); - - @Test - void rejects_positional_invocation_with_clear_message() { - SyntaxCheckException ex = - assertThrows( - SyntaxCheckException.class, - () -> expander.expand(List.of(AstDSL.qualifiedName("price"), AstDSL.intLiteral(100)))); - assertTrue(ex.getMessage().contains("named arguments")); - assertTrue(ex.getMessage().contains("histogram")); - } - - @Test - void property_bag_with_string_field_coerces_to_qualified_name() { - UnresolvedExpression result = - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("age")), kv("interval", AstDSL.intLiteral(10)))); - - assertEquals( - new Span(AstDSL.qualifiedName("age"), AstDSL.intLiteral(10), SpanUnit.NONE), result); - } - - @Test - void property_bag_with_qualified_name_field_passes_through_unchanged() { - QualifiedName age = AstDSL.qualifiedName("age"); - UnresolvedExpression result = - expander.expand(List.of(kv("field", age), kv("interval", AstDSL.intLiteral(10)))); - - assertEquals(new Span(age, AstDSL.intLiteral(10), SpanUnit.NONE), result); - } - - @Test - void property_bag_rejects_alias() { - assertThrows( - SyntaxCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("age")), - kv("interval", AstDSL.intLiteral(10)), - kv("alias", AstDSL.stringLiteral("my_label"))))); - } - - @Test - void property_bag_rejects_nested() { - assertThrows( - SyntaxCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("age")), - kv("interval", AstDSL.intLiteral(10)), - kv("nested", AstDSL.stringLiteral("path"))))); - } - - @Test - void property_bag_rejects_reverse_nested() { - assertThrows( - SyntaxCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("age")), - kv("interval", AstDSL.intLiteral(10)), - kv("reverse_nested", AstDSL.stringLiteral("path"))))); - } - - @Test - void property_bag_rejects_children() { - assertThrows( - SyntaxCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("age")), - kv("interval", AstDSL.intLiteral(10)), - kv("children", AstDSL.stringLiteral("ignored"))))); - } - - @Test - void property_bag_rejects_format() { - assertThrows( - SyntaxCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("age")), - kv("interval", AstDSL.intLiteral(10)), - kv("format", AstDSL.stringLiteral("yyyy"))))); - } - - @Test - void property_bag_rejects_time_zone() { - assertThrows( - SyntaxCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("age")), - kv("interval", AstDSL.intLiteral(10)), - kv("time_zone", AstDSL.stringLiteral("+05:30"))))); - } - - @Test - void property_bag_rejects_min_doc_count() { - assertThrows( - SyntaxCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("age")), - kv("interval", AstDSL.intLiteral(10)), - kv("min_doc_count", AstDSL.intLiteral(5))))); - } - - @Test - void property_bag_rejects_order() { - assertThrows( - SyntaxCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("age")), - kv("interval", AstDSL.intLiteral(10)), - kv("order", AstDSL.stringLiteral("count_desc"))))); - } - - @Test - void property_bag_rejects_extended_bounds() { - assertThrows( - SyntaxCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("age")), - kv("interval", AstDSL.intLiteral(10)), - kv("extended_bounds", AstDSL.stringLiteral("0:100"))))); - } - - @Test - void property_bag_rejects_unknown_param() { - SyntaxCheckException ex = - assertThrows( - SyntaxCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("age")), - kv("interval", AstDSL.intLiteral(10)), - kv("missing_param", AstDSL.stringLiteral("foo"))))); - assertTrue(ex.getMessage().contains("missing_param")); - } - - @Test - void property_bag_rejects_duplicate_keys() { - assertThrows( - SemanticCheckException.class, - () -> - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("age")), - kv("field", AstDSL.stringLiteral("size")), - kv("interval", AstDSL.intLiteral(10))))); - } - - @Test - void property_bag_rejects_missing_field() { - assertThrows( - SemanticCheckException.class, - () -> expander.expand(List.of(kv("interval", AstDSL.intLiteral(10))))); - } - - @Test - void property_bag_rejects_missing_interval() { - assertThrows( - SemanticCheckException.class, - () -> expander.expand(List.of(kv("field", AstDSL.stringLiteral("age"))))); - } - - @Test - void property_bag_offset_shifts_bucket_boundaries() { - UnresolvedExpression result = - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("age")), - kv("interval", AstDSL.intLiteral(10)), - kv("offset", AstDSL.intLiteral(3)))); - - QualifiedName age = AstDSL.qualifiedName("age"); - Function shiftedField = new Function("-", List.of(age, AstDSL.intLiteral(3))); - Span bucket = new Span(shiftedField, AstDSL.intLiteral(10), SpanUnit.NONE); - Function expected = new Function("+", List.of(bucket, AstDSL.intLiteral(3))); - assertEquals(expected, result); - } - - @Test - void property_bag_missing_wraps_field_with_coalesce() { - UnresolvedExpression result = - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("age")), - kv("interval", AstDSL.intLiteral(10)), - kv("missing", AstDSL.intLiteral(0)))); - - Function coalesced = - new Function("coalesce", List.of(AstDSL.qualifiedName("age"), AstDSL.intLiteral(0))); - assertEquals(new Span(coalesced, AstDSL.intLiteral(10), SpanUnit.NONE), result); - } - - @Test - void property_bag_offset_and_missing_compose_in_correct_order() { - UnresolvedExpression result = - expander.expand( - List.of( - kv("field", AstDSL.stringLiteral("age")), - kv("interval", AstDSL.intLiteral(10)), - kv("offset", AstDSL.intLiteral(3)), - kv("missing", AstDSL.intLiteral(0)))); - - QualifiedName age = AstDSL.qualifiedName("age"); - Function coalesced = new Function("coalesce", List.of(age, AstDSL.intLiteral(0))); - Function shifted = new Function("-", List.of(coalesced, AstDSL.intLiteral(3))); - Span bucket = new Span(shifted, AstDSL.intLiteral(10), SpanUnit.NONE); - Function expected = new Function("+", List.of(bucket, AstDSL.intLiteral(3))); - assertEquals(expected, result); - } - - @Test - void via_sql_lowers_to_span() { - QualifiedName age = AstDSL.qualifiedName("age"); - Span bucket = AstDSL.span(age, AstDSL.intLiteral(10), SpanUnit.NONE); - - UnresolvedPlan result = - buildAST( - "SELECT histogram('field'='age', 'interval'=10), COUNT(*) FROM accounts " - + "GROUP BY histogram('field'='age', 'interval'=10)"); - - assertEquals( - AstDSL.project( - AstDSL.agg( - AstDSL.relation("accounts"), - ImmutableList.of( - AstDSL.alias("COUNT(*)", AstDSL.aggregate("COUNT", AllFields.of()))), - emptyList(), - ImmutableList.of(AstDSL.alias(bucket.toString(), bucket)), - emptyList()), - AstDSL.alias("histogram('field'='age', 'interval'=10)", bucket), - AstDSL.alias("COUNT(*)", AstDSL.aggregate("COUNT", AllFields.of()))), - result); - } - - @Test - void via_sql_rejects_positional_invocation() { - assertThrows( - SyntaxCheckException.class, - () -> buildAST("SELECT histogram(price, 100) FROM orders GROUP BY histogram(price, 100)")); - } - - /** Builds a Function("=", [stringLiteral(key), value]) — same shape ANTLR produces for 'k'=v. */ - private static UnresolvedExpression kv(String key, UnresolvedExpression value) { - return new Function("=", List.of(AstDSL.stringLiteral(key), value)); - } -} diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/NamedArgumentsTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/NamedArgumentsTest.java deleted file mode 100644 index 1eed9b55bc9..00000000000 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/bucket/NamedArgumentsTest.java +++ /dev/null @@ -1,250 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.sql.parser.bucket; - -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNull; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; - -import java.util.List; -import java.util.Set; -import org.junit.jupiter.api.DisplayNameGeneration; -import org.junit.jupiter.api.DisplayNameGenerator; -import org.junit.jupiter.api.Test; -import org.opensearch.sql.ast.dsl.AstDSL; -import org.opensearch.sql.ast.expression.Function; -import org.opensearch.sql.ast.expression.Literal; -import org.opensearch.sql.ast.expression.UnresolvedExpression; -import org.opensearch.sql.common.antlr.SyntaxCheckException; -import org.opensearch.sql.exception.SemanticCheckException; - -@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) -class NamedArgumentsTest { - - @Test - void empty_arg_list_is_not_named_arguments() { - assertFalse(NamedArguments.isNamedArguments(List.of())); - } - - @Test - void single_kv_pair_is_named_arguments() { - assertTrue(NamedArguments.isNamedArguments(List.of(kv("k", AstDSL.intLiteral(1))))); - } - - @Test - void plain_function_call_is_not_named_arguments() { - UnresolvedExpression nonKv = AstDSL.qualifiedName("col"); - assertFalse(NamedArguments.isNamedArguments(List.of(nonKv))); - } - - @Test - void mixed_args_are_not_named_arguments() { - assertFalse( - NamedArguments.isNamedArguments( - List.of(kv("k", AstDSL.intLiteral(1)), AstDSL.qualifiedName("col")))); - } - - @Test - void non_equals_function_is_not_named_arguments() { - UnresolvedExpression notEq = - new Function("+", List.of(AstDSL.stringLiteral("a"), AstDSL.intLiteral(1))); - assertFalse(NamedArguments.isNamedArguments(List.of(notEq))); - } - - @Test - void equals_with_non_string_left_is_not_named_arguments() { - UnresolvedExpression intEqInt = - new Function("=", List.of(AstDSL.intLiteral(1), AstDSL.intLiteral(2))); - assertFalse(NamedArguments.isNamedArguments(List.of(intEqInt))); - } - - /** - * The legacy spelling, {@code date_histogram(field='ts', ...)}, arrives here as an equality whose - * left side is a column reference rather than a string literal. Reading it as named arguments - * would take the call away from the legacy engine that has always served it. - */ - @Test - void equals_with_a_column_reference_on_the_left_is_not_named_arguments() { - UnresolvedExpression fieldEqValue = - new Function("=", List.of(AstDSL.qualifiedName("field"), AstDSL.stringLiteral("ts"))); - assertFalse(NamedArguments.isNamedArguments(List.of(fieldEqValue))); - } - - @Test - void equals_with_other_than_two_operands_is_not_named_arguments() { - UnresolvedExpression threeOperands = - new Function( - "=", List.of(AstDSL.stringLiteral("k"), AstDSL.intLiteral(1), AstDSL.intLiteral(2))); - assertFalse(NamedArguments.isNamedArguments(List.of(threeOperands))); - } - - @Test - void a_string_parameter_given_a_column_reference_is_rejected() { - NamedArguments bag = - NamedArguments.parse(List.of(kv("interval", AstDSL.qualifiedName("not_a_literal")))); - - assertThrows(SemanticCheckException.class, () -> bag.requireStringIfPresent("interval")); - } - - @Test - void parse_keeps_keys_in_source_order_and_lower_cases_them() { - NamedArguments bag = - NamedArguments.parse( - List.of( - kv("Field", AstDSL.stringLiteral("ts")), - kv("INTERVAL", AstDSL.stringLiteral("1d")))); - - assertEquals(AstDSL.stringLiteral("ts"), bag.remove("field")); - assertEquals(AstDSL.stringLiteral("1d"), bag.remove("interval")); - assertEquals(0, bag.size()); - } - - @Test - void parse_rejects_duplicate_keys() { - SemanticCheckException ex = - assertThrows( - SemanticCheckException.class, - () -> - NamedArguments.parse( - List.of( - kv("field", AstDSL.stringLiteral("a")), - kv("field", AstDSL.stringLiteral("b"))))); - assertTrue(ex.getMessage().contains("field")); - } - - @Test - void parse_rejects_non_key_value_arg_with_clear_message() { - UnresolvedExpression bareColumn = AstDSL.qualifiedName("age"); - SemanticCheckException ex = - assertThrows( - SemanticCheckException.class, - () -> - NamedArguments.parse(List.of(kv("field", AstDSL.stringLiteral("a")), bareColumn))); - assertTrue(ex.getMessage().contains("'key'=value")); - } - - @Test - void remove_returns_value_when_present_and_null_when_absent() { - NamedArguments bag = NamedArguments.parse(List.of(kv("field", AstDSL.stringLiteral("ts")))); - assertEquals(AstDSL.stringLiteral("ts"), bag.remove("field")); - assertNull(bag.remove("field")); - assertNull(bag.remove("never_inserted")); - assertEquals(0, bag.size()); - } - - @Test - void require_returns_value_and_removes_it() { - NamedArguments bag = NamedArguments.parse(List.of(kv("field", AstDSL.stringLiteral("ts")))); - assertEquals(AstDSL.stringLiteral("ts"), bag.require("field", "histogram")); - assertEquals(0, bag.size()); - } - - @Test - void require_throws_when_missing_with_function_name_in_message() { - NamedArguments bag = NamedArguments.parse(List.of(kv("other", AstDSL.intLiteral(1)))); - SemanticCheckException ex = - assertThrows(SemanticCheckException.class, () -> bag.require("field", "HISTOGRAM")); - assertTrue(ex.getMessage().contains("histogram")); - assertTrue(ex.getMessage().contains("field")); - } - - @Test - void requireString_returns_string_literal() { - NamedArguments bag = NamedArguments.parse(List.of(kv("interval", AstDSL.stringLiteral("1d")))); - Literal interval = bag.requireString("interval", "date_histogram"); - assertEquals(AstDSL.stringLiteral("1d"), interval); - } - - @Test - void requireString_rejects_non_string_value() { - NamedArguments bag = NamedArguments.parse(List.of(kv("interval", AstDSL.intLiteral(100)))); - assertThrows( - SemanticCheckException.class, () -> bag.requireString("interval", "date_histogram")); - } - - @Test - void requireStringIfPresent_returns_null_when_absent() { - NamedArguments bag = NamedArguments.parse(List.of(kv("field", AstDSL.stringLiteral("ts")))); - assertNull(bag.requireStringIfPresent("format")); - } - - @Test - void requireStringIfPresent_returns_value_when_present() { - NamedArguments bag = NamedArguments.parse(List.of(kv("format", AstDSL.stringLiteral("yyyy")))); - assertEquals(AstDSL.stringLiteral("yyyy"), bag.requireStringIfPresent("format")); - } - - @Test - void requireStringIfPresent_rejects_non_string_value_when_present() { - NamedArguments bag = NamedArguments.parse(List.of(kv("format", AstDSL.intLiteral(2024)))); - assertThrows(SemanticCheckException.class, () -> bag.requireStringIfPresent("format")); - } - - @Test - void rejectIfPresent_throws_when_key_present() { - NamedArguments bag = NamedArguments.parse(List.of(kv("script", AstDSL.stringLiteral("x")))); - SyntaxCheckException ex = - assertThrows(SyntaxCheckException.class, () -> bag.rejectIfPresent("script", "no!")); - assertTrue(ex.getMessage().contains("no!")); - } - - @Test - void rejectIfPresent_no_op_when_key_absent() { - NamedArguments bag = NamedArguments.parse(List.of(kv("field", AstDSL.stringLiteral("ts")))); - bag.rejectIfPresent("script", "no!"); - assertEquals(1, bag.size()); - } - - @Test - void consumeSilently_drops_listed_keys() { - NamedArguments bag = - NamedArguments.parse( - List.of( - kv("alias", AstDSL.stringLiteral("x")), - kv("nested", AstDSL.stringLiteral("p")), - kv("interval", AstDSL.intLiteral(10)))); - bag.consumeSilently(Set.of("alias", "nested")); - assertEquals(1, bag.size()); - } - - @Test - void rejectRemaining_single_key_uses_parameter_label() { - NamedArguments bag = NamedArguments.parse(List.of(kv("mystery", AstDSL.intLiteral(5)))); - SyntaxCheckException ex = - assertThrows(SyntaxCheckException.class, () -> bag.rejectRemaining("HISTOGRAM")); - assertTrue(ex.getMessage().contains("histogram")); - assertTrue(ex.getMessage().contains("does not accept parameter:")); - assertTrue(ex.getMessage().contains("mystery")); - } - - @Test - void rejectRemaining_multiple_keys_listed_in_source_order_with_plural_label() { - NamedArguments bag = - NamedArguments.parse( - List.of( - kv("foo", AstDSL.intLiteral(1)), - kv("bar", AstDSL.intLiteral(2)), - kv("baz", AstDSL.intLiteral(3)))); - SyntaxCheckException ex = - assertThrows(SyntaxCheckException.class, () -> bag.rejectRemaining("HISTOGRAM")); - assertTrue(ex.getMessage().contains("does not accept parameters:")); - assertTrue(ex.getMessage().contains("foo, bar, baz")); - } - - @Test - void rejectRemaining_no_op_when_bag_empty() { - NamedArguments bag = NamedArguments.parse(List.of(kv("alias", AstDSL.stringLiteral("x")))); - bag.consumeSilently(Set.of("alias")); - bag.rejectRemaining("histogram"); // does not throw - } - - /** Builds a Function("=", [stringLiteral(key), value]) — same shape ANTLR produces for 'k'=v. */ - private static UnresolvedExpression kv(String key, UnresolvedExpression value) { - return new Function("=", List.of(AstDSL.stringLiteral(key), value)); - } -} From 981a4387ad2140007a49f199a5102500cc13102d Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Wed, 19 Aug 2026 10:09:53 -0700 Subject: [PATCH 07/18] Make the missing parameter substitute a value the engine can evaluate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Testing the refactor against a live cluster turned up a parameter that has never worked. `missing` lowered to `coalesce`, which V2 lists in BuiltinFunctionName but does not implement, so any query using it failed at execution with "unsupported function name: coalesce". The unit tests asserted that the AST contained a coalesce node, which is true and says nothing about whether the query runs. `ifnull` is the two-argument form V2 evaluates. The integration test that pins this uses the numeric field on purpose. Substituting into a date needs a timestamp-typed replacement, and the grammar admits only literals in this position, so a date `missing` reaches IFNULL as TIMESTAMP against STRING — which V2 accepts and the analytics engine rejects. Asserting either outcome would contradict the other route. Verified after the refactor: 983 default-route tests with no failures, the analytics route 8 passed and 3 skipped with none failing, `:sql:build` green including the coverage gate, and the bucket values unchanged on both routes. Also checked argument order, function-name and key casing, extra whitespace, double-quoted keys, backticked and string field names, and numeric intervals. Signed-off-by: Jialiang Liang --- .../sql/DateHistogramBucketFunctionIT.java | 22 +++++++++++++++++++ .../sql/sql/parser/AstExpressionBuilder.java | 11 +++++++--- .../sql/parser/AstExpressionBuilderTest.java | 2 +- 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java index 76e154cf2e7..3f7e2e33652 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java @@ -137,6 +137,28 @@ public void bucketsRespectAWhereClause() throws IOException { rows("2026-01-01 03:00:00", 19)); } + /** + * `missing` substitutes a value for a null field before bucketing. Asserted end to end because + * the AST alone cannot show whether the substitution function is one the engine evaluates — + * `coalesce` is a registered name with no V2 implementation, `ifnull` is the one that runs. + * + *

Uses the numeric field: substituting into a date needs a timestamp-typed replacement, and + * the grammar admits only literals here, so a date `missing` reaches IFNULL as TIMESTAMP against + * STRING. V2 accepts that pair, the analytics engine rejects it, and a test asserting either + * result would disagree with the other route. + */ + @Test + public void missingParameterSubstitutesBeforeBucketing() throws IOException { + JSONObject response = + executeQuery( + "SELECT b, COUNT(*) FROM (SELECT histogram('field'=value, 'interval'=20, 'missing'=0)" + + " AS b FROM " + + IDX + + ") sub GROUP BY b ORDER BY b"); + + verifyDataRowsInOrder(response, rows(0, 19), rows(20, 20), rows(40, 20), rows(60, 13)); + } + @Test public void numericHistogramBucketsByInterval() throws IOException { JSONObject response = diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java index c65352335d4..0b6dc474ba1 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java @@ -204,7 +204,7 @@ public UnresolvedExpression visitBucketFunctionCall(BucketFunctionCallContext ct functionName + " does not accept parameter: " + String.join(", ", args.keySet())); } - UnresolvedExpression bucketed = coalesceMissing(normalizeField(field), missing); + UnresolvedExpression bucketed = substituteMissing(normalizeField(field), missing); if (timeZone != null) { bucketed = shiftByTimeZone(bucketed, timeZone); } @@ -273,9 +273,14 @@ private static UnresolvedExpression normalizeField(UnresolvedExpression field) { return field; } - private static UnresolvedExpression coalesceMissing( + /** + * 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("coalesce", List.of(field, missing)); + return missing == null ? field : new Function("ifnull", List.of(field, missing)); } /** diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java index 51f919001a7..c45b95f6193 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java @@ -904,7 +904,7 @@ public void canBuildNumericHistogramAsSpan() { public void canBuildDateHistogramWithMissing() { assertEquals( new Span( - function("coalesce", qualifiedName("ts"), stringLiteral("1970-01-01")), + function("ifnull", qualifiedName("ts"), stringLiteral("1970-01-01")), intLiteral(1), SpanUnit.H), buildExprAst("date_histogram('field'=ts, 'interval'='1h', 'missing'='1970-01-01')")); From cb0023a28e2c9048317e7433e1988202a46eef1c Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Wed, 19 Aug 2026 10:22:25 -0700 Subject: [PATCH 08/18] Always lower a bucket function to a span Review feedback: a histogram expression should end up as an OpenSearch histogram aggregate, so generating date_format or timestampadd was surprising. It is, and they are gone -- `format` and `time_zone` now defer to the legacy engine along with alias, min_doc_count and order. The legacy engine implements all five natively (AggMaker builds them straight onto the date_histogram aggregation), and for time_zone it does so better: `dateHistogram.timeZone(ZoneOffset.of(value))` shifts bucket boundaries properly, where this code was adding a fixed number of seconds and would have been wrong across a daylight-saving change. Handing those queries back means they are answered by the implementation that already had them right. What is left always produces a Span, which is what the earlier comment about lowering to existing AST primitives described. `missing` still wraps the field in `ifnull`, since substituting a value has to happen before bucketing. Verified on a live cluster: the plain and `missing` forms answer from V2 (12/24/17/19 and 19/20/20/13), while `format`, `time_zone` and `alias` reach legacy and answer correctly -- time_zone returning 5/18/30/19, the shifted boundaries. 983 default-route tests pass, and `:sql:build` is green including the coverage gate. Signed-off-by: Jialiang Liang --- .../sql/DateHistogramBucketFunctionIT.java | 5 +- .../sql/sql/parser/AstExpressionBuilder.java | 48 +++---------------- .../sql/parser/AstExpressionBuilderTest.java | 36 +++----------- 3 files changed, 15 insertions(+), 74 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java index 3f7e2e33652..fcb71917981 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java @@ -186,8 +186,9 @@ public void positionalCallReturnsHourlyBuckets() throws IOException { } /** - * `alias` has no lowering here but the legacy engine implements it, so the query still has to - * answer. CsvFormatResponseIT.dateHistogramTest has asserted this shape for years. + * The legacy engine implements alias, format, time_zone, min_doc_count and order through the + * native date_histogram aggregation; this lowering has no equivalent, so those queries still have + * to reach it. CsvFormatResponseIT.dateHistogramTest has asserted this shape for years. */ @Test @RequiresCapability(LEGACY_ENGINE_FALLBACK) diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java index 0b6dc474ba1..6ab74304532 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java @@ -72,7 +72,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import java.time.ZoneOffset; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; @@ -193,23 +192,18 @@ public UnresolvedExpression visitBucketFunctionCall(BucketFunctionCallContext ct UnresolvedExpression field = requireArg(args, "field", functionName); UnresolvedExpression missing = args.remove("missing"); Literal interval = intervalOf(args, functionName); - Literal format = stringArg(args, "format"); - Literal timeZone = stringArg(args, "time_zone"); - // Anything left is a parameter with no lowering here. Some of them -- alias, min_doc_count, - // order -- are implemented by the legacy engine, so decline in the one way RestSQLQueryAction - // falls back on rather than failing the request outright. + // Anything left is a parameter this lowering has no equivalent for. The legacy engine + // implements all of them -- alias, format, time_zone, min_doc_count, order -- through the + // native date_histogram aggregation, so decline in the one way RestSQLQueryAction falls back + // on rather than failing a query it can answer. if (!args.isEmpty()) { throw new SyntaxCheckException( functionName + " does not accept parameter: " + String.join(", ", args.keySet())); } - UnresolvedExpression bucketed = substituteMissing(normalizeField(field), missing); - if (timeZone != null) { - bucketed = shiftByTimeZone(bucketed, timeZone); - } - Span span = AstDSL.spanFromSpanLengthLiteral(bucketed, interval); - return format == null ? span : new Function("date_format", List.of(span, format)); + return AstDSL.spanFromSpanLengthLiteral( + substituteMissing(normalizeField(field), missing), interval); } private static UnresolvedExpression requireArg( @@ -242,18 +236,6 @@ private static Literal intervalOf(Map args, String return supplied.get(0); } - private static Literal stringArg(Map 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); - } - return literal; - } - private static Literal stringOrNumericArg(Map args, String name) { UnresolvedExpression value = args.remove(name); if (value == null) { @@ -283,24 +265,6 @@ private static UnresolvedExpression substituteMissing( return missing == null ? field : new Function("ifnull", List.of(field, missing)); } - /** - * Shifts the field by a {@link ZoneOffset} before bucketing. Validated here so an invalid offset - * is reported rather than surfacing as an arithmetic failure at execution. - */ - private static UnresolvedExpression shiftByTimeZone( - UnresolvedExpression field, Literal timeZone) { - String offset = timeZone.getValue().toString(); - int seconds; - try { - seconds = ZoneOffset.of(offset).getTotalSeconds(); - } catch (RuntimeException e) { - throw new SemanticCheckException( - "time_zone must be a valid offset like '+05:30' or 'Z'; got '" + offset + "'"); - } - return new Function( - "timestampadd", List.of(AstDSL.stringLiteral("SECOND"), AstDSL.intLiteral(seconds), field)); - } - @Override public UnresolvedExpression visitGetFormatFunctionCall(GetFormatFunctionCallContext ctx) { return new Function( diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java index c45b95f6193..119a7c5738a 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java @@ -910,27 +910,6 @@ public void canBuildDateHistogramWithMissing() { buildExprAst("date_histogram('field'=ts, 'interval'='1h', 'missing'='1970-01-01')")); } - @Test - public void canBuildDateHistogramWithTimeZoneShift() { - assertEquals( - new Span( - function( - "timestampadd", stringLiteral("SECOND"), intLiteral(19800), qualifiedName("ts")), - intLiteral(1), - SpanUnit.H), - buildExprAst("date_histogram('field'=ts, 'interval'='1h', 'time_zone'='+05:30')")); - } - - @Test - public void canBuildDateHistogramWithFormat() { - assertEquals( - function( - "date_format", - new Span(qualifiedName("ts"), intLiteral(1), SpanUnit.D), - stringLiteral("yyyy-MM-dd")), - buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'format'='yyyy-MM-dd')")); - } - /** * A parameter with no lowering here has to raise SyntaxCheckException -- the one type * RestSQLQueryAction falls back on -- because the legacy engine implements alias, min_doc_count @@ -944,6 +923,12 @@ public void unsupportedBucketParameterDefersToLegacyEngine() { assertThrows( SyntaxCheckException.class, () -> buildExprAst("histogram('field'=age, 'interval'=10, 'min_doc_count'=1)")); + assertThrows( + SyntaxCheckException.class, + () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'format'='yyyy-MM-dd')")); + assertThrows( + SyntaxCheckException.class, + () -> buildExprAst("date_histogram('field'=ts, 'interval'='1h', 'time_zone'='+05:30')")); } /** A bad argument inside a shape we own must not fall back, so the caller sees this message. */ @@ -955,15 +940,6 @@ public void badBucketArgumentIsReportedRatherThanDeferred() { assertThrows( SemanticCheckException.class, () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'fixed_interval'='2d')")); - assertThrows( - SemanticCheckException.class, - () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'time_zone'='nope')")); - assertThrows( - SemanticCheckException.class, - () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'format'=7)")); - assertThrows( - SemanticCheckException.class, - () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'format'=other)")); assertThrows( SemanticCheckException.class, () -> buildExprAst("date_histogram('field'=ts, 'interval'=ts)")); From bb83e903c3c53caf2f668b064a5f6fd29e2f6d53 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Wed, 19 Aug 2026 10:57:03 -0700 Subject: [PATCH 09/18] Let V2 answer bucket calls written with bare argument names Review feedback: with the grammar change these should be handled by V2 rather than deferred. They are now. `bucketArgName` admits a bare identifier as well as a quoted string, so `date_histogram(field=ts, interval='1h')` -- the spelling the legacy engine has always taken -- lowers to a Span like any other call. INTERVAL, MISSING, ORDER and TIME_ZONE are listed explicitly because they are reserved words that `ident` excludes. I had assumed V2 could not group directly on an expression and that these queries could only ever come from legacy. That was wrong: the limitation is specific to two grouping keys over a bare table scan, and a single key is fine. Confirmed by the explain plan (ProjectOperator over OpenSearchIndexScan) and by the return type, which is long from V2 where legacy gives double. Two of the three capability-gated tests are gone as a result -- both routes now answer those queries and agree on the values. Only the `alias` case still defers, since that parameter has no lowering here and the analytics route has no legacy engine to hand it to. Verified: 983 default-route tests with no failures; the analytics route 10 passed, 1 skipped, none failed; `:sql:build` green including the coverage gate. Against a main baseline on the same cluster the analytics suite moved 15 pass->fail and 14 fail->pass, all in unrelated classes -- the same noise floor measured earlier, where re-running three classes on main alone flipped 5 of 106. Signed-off-by: Jialiang Liang --- .../sql/DateHistogramBucketFunctionIT.java | 28 +++++++++++-------- .../src/main/antlr4/OpenSearchSQLParser.g4 | 5 ++++ sql/src/main/antlr/OpenSearchSQLParser.g4 | 5 ++++ .../sql/parser/AstExpressionBuilderTest.java | 11 ++++++++ 4 files changed, 37 insertions(+), 12 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java index fcb71917981..e2e00ee6262 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java @@ -171,18 +171,21 @@ public void numericHistogramBucketsByInterval() throws IOException { verifyDataRowsInOrder(response, rows(0, 19), rows(20, 20), rows(40, 20), rows(60, 13)); } - /** - * Only the legacy V1 engine understands this spelling, and it answered before these names entered - * the V2 grammar. The expander has to keep declining with SyntaxCheckException so it still does. - */ + /** Argument names may be written bare, the spelling the legacy engine has always accepted. */ @Test - @RequiresCapability(LEGACY_ENGINE_FALLBACK) - public void positionalCallReturnsHourlyBuckets() throws IOException { + public void unquotedArgumentNamesReturnHourlyBuckets() throws IOException { JSONObject response = executeQuery( - "SELECT COUNT(*) FROM " + IDX + " GROUP BY date_histogram(field='ts','interval'='1h')"); + "SELECT b, COUNT(*) FROM (SELECT date_histogram(field=ts, interval='1h') AS b FROM " + + IDX + + ") sub GROUP BY b ORDER BY b"); - verifyDataRows(response, rows(12), rows(24), rows(17), rows(19)); + verifyDataRowsInOrder( + response, + rows("2026-01-01 00:00:00", 12), + rows("2026-01-01 01:00:00", 24), + rows("2026-01-01 02:00:00", 17), + rows("2026-01-01 03:00:00", 19)); } /** @@ -203,12 +206,13 @@ public void callWithAliasParameterReturnsHourlyBuckets() throws IOException { } @Test - @RequiresCapability(LEGACY_ENGINE_FALLBACK) - public void positionalNumericHistogramReturnsBuckets() throws IOException { + public void unquotedArgumentNamesReturnNumericBuckets() throws IOException { JSONObject response = executeQuery( - "SELECT COUNT(*) FROM " + IDX + " GROUP BY histogram(field='value','interval'='20')"); + "SELECT b, COUNT(*) FROM (SELECT histogram(field=value, interval=20) AS b FROM " + + IDX + + ") sub GROUP BY b ORDER BY b"); - verifyDataRows(response, rows(19), rows(20), rows(20), rows(13)); + verifyDataRowsInOrder(response, rows(0, 19), rows(20, 20), rows(40, 20), rows(60, 13)); } } diff --git a/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 b/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 index 5162e6d1e78..ad1e88214b1 100644 --- a/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 +++ b/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 @@ -772,6 +772,11 @@ highlightArgName bucketArgName : stringLiteral + | ident + | INTERVAL + | MISSING + | ORDER + | TIME_ZONE ; relevanceFieldAndWeight diff --git a/sql/src/main/antlr/OpenSearchSQLParser.g4 b/sql/src/main/antlr/OpenSearchSQLParser.g4 index fa0b5b91ea9..35b98ec1dc7 100644 --- a/sql/src/main/antlr/OpenSearchSQLParser.g4 +++ b/sql/src/main/antlr/OpenSearchSQLParser.g4 @@ -805,6 +805,11 @@ highlightArgName bucketArgName : stringLiteral + | ident + | INTERVAL + | MISSING + | ORDER + | TIME_ZONE ; relevanceFieldAndWeight diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java index 119a7c5738a..8298c81ab7d 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java @@ -871,6 +871,17 @@ public void canBuildDateHistogramAsSpan() { buildExprAst("date_histogram('field'=ts, 'interval'='1h')")); } + /** Bare argument names are the spelling the legacy engine accepts; both forms lower alike. */ + @Test + public void canBuildBucketFunctionWithUnquotedArgumentNames() { + assertEquals( + new Span(qualifiedName("ts"), intLiteral(1), SpanUnit.H), + buildExprAst("date_histogram(field=ts, interval='1h')")); + assertEquals( + new Span(qualifiedName("age"), intLiteral(10), SpanUnit.NONE), + buildExprAst("histogram(field=age, interval=10)")); + } + @Test public void canBuildDateHistogramWithIntervalSynonyms() { Span expected = new Span(qualifiedName("ts"), intLiteral(1), SpanUnit.D); From 3a2b1e6209d0b00d5c68e80bd956dfc31e58992a Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Wed, 19 Aug 2026 11:15:42 -0700 Subject: [PATCH 10/18] Report a misspelled bucket parameter instead of deferring it Follow-up to the review. Accepting bare argument names means anything now parses, so an unrecognised name reaches the builder as a leftover argument and was being declined as a syntax check -- which routes it to the legacy engine. A typo would quietly become a legacy-engine query instead of an error, the failure mode the earlier review comment was about. Only the parameters the legacy engine actually implements -- alias, format, time_zone, min_doc_count, order -- defer now. Anything else is a semantic check, so the caller sees the message. Also in this commit: `ifnull` is built from BuiltinFunctionName like the other constant function names in this file rather than a string literal; the new capability constant no longer sits between LEGACY_METHOD_QUERY and its javadoc, which left that constant undocumented. Correcting the previous commit message: it said the grouping limitation was specific to two grouping keys and that a single key was fine. That is wrong. A span over a bare table scan cannot resolve its field either way -- SELECT date_histogram('field'=ts, 'interval'='1h') AS b, COUNT(*) FROM idx GROUP BY date_histogram('field'=ts, 'interval'='1h') fails on both routes, with or without the select alias, so the bucket always has to be projected in a derived table first. What the grammar change did fix is the bare-name spelling, which is what let the two capability gates go. A test now pins the rejection, asserting only that it is rejected, since the two routes word the error differently. Added coverage for the 1M and 1y calendar units Dashboards emits at the wider zoom levels, which nothing exercised before. Verified: 13 integration tests, none failing or skipped, on the default route; `:sql:build` green including the coverage gate. Signed-off-by: Jialiang Liang --- .../sql/DateHistogramBucketFunctionIT.java | 39 ++++++++++++++++--- .../org/opensearch/sql/util/Capability.java | 16 ++++---- .../sql/sql/parser/AstExpressionBuilder.java | 27 ++++++++----- .../sql/parser/AstExpressionBuilderTest.java | 14 +++++++ 4 files changed, 73 insertions(+), 23 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java index e2e00ee6262..d85cee7676e 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java @@ -5,6 +5,7 @@ package org.opensearch.sql.sql; +import static org.junit.Assert.assertThrows; import static org.opensearch.sql.util.Capability.LEGACY_ENGINE_FALLBACK; import static org.opensearch.sql.util.MatcherUtils.rows; import static org.opensearch.sql.util.MatcherUtils.verifyDataRows; @@ -13,6 +14,7 @@ import java.io.IOException; import org.json.JSONObject; import org.junit.Test; +import org.opensearch.client.ResponseException; import org.opensearch.sql.legacy.SQLIntegTestCase; import org.opensearch.sql.util.RequiresCapability; @@ -38,7 +40,10 @@ protected void init() throws Exception { loadIndex(Index.DATE_HISTOGRAM_TEST); } - /** The planner rejects {@code GROUP BY }, so the bucket is aliased in a subquery. */ + /** + * The bucket has to be projected in a derived table before it can be grouped on; see {@link + * #groupingOnTheBucketWithoutADerivedTableIsRejected}. This is also the shape Dashboards emits. + */ private static String bucketed(String bucketExpr) { return "SELECT b, COUNT(*) FROM (SELECT " + bucketExpr @@ -99,10 +104,7 @@ public void intervalSynonymsProduceTheSameBuckets() throws IOException { } } - /** - * The scan sits in its own derived table because the V2 engine cannot resolve the span's field - * otherwise when a second grouping key is present. - */ + /** A second grouping key needs the scan in a derived table of its own as well. */ @Test public void bucketsCombineWithAnAdditionalGroupingKey() throws IOException { JSONObject response = @@ -215,4 +217,31 @@ public void unquotedArgumentNamesReturnNumericBuckets() throws IOException { verifyDataRowsInOrder(response, rows(0, 19), rows(20, 20), rows(40, 20), rows(60, 13)); } + + /** + * A span over a bare table scan cannot resolve its field, with or without a select alias, so the + * bucket always has to be projected in a derived table first. Both routes reject this; only the + * message differs, so the assertion is on the rejection alone. + */ + @Test + public void groupingOnTheBucketWithoutADerivedTableIsRejected() { + assertThrows( + ResponseException.class, + () -> + executeQuery( + "SELECT date_histogram('field'=ts, 'interval'='1h') AS b, COUNT(*) FROM " + + IDX + + " GROUP BY date_histogram('field'=ts, 'interval'='1h')")); + } + + /** Calendar units: Dashboards emits 1M and 1y at the wider zoom levels. */ + @Test + public void calendarIntervalsBucketByMonthAndYear() throws IOException { + verifyDataRows( + executeQuery(bucketed("date_histogram('field'=ts, 'interval'='1M')")), + rows("2026-01-01 00:00:00", 72)); + verifyDataRows( + executeQuery(bucketed("date_histogram('field'=ts, 'interval'='1y')")), + rows("2026-01-01 00:00:00", 72)); + } } diff --git a/integ-test/src/test/java/org/opensearch/sql/util/Capability.java b/integ-test/src/test/java/org/opensearch/sql/util/Capability.java index f5a5b784e38..bb99dc85115 100644 --- a/integ-test/src/test/java/org/opensearch/sql/util/Capability.java +++ b/integ-test/src/test/java/org/opensearch/sql/util/Capability.java @@ -537,21 +537,21 @@ public enum Capability { PREPARED_STATEMENT( "Prepared statements are unsupported on the analytics-engine route (Calcite path)."), - /** - * FRONTEND: legacy method-query syntax (regexp_query/wildcard_query) is not in the Calcite - * grammar. - */ /** * FRONTEND: the legacy V1 engine answers call shapes the V2 grammar declines, but only on the * default route. Requests reach it when RestSQLQueryAction catches a SyntaxCheckException; the * analytics-engine route enters through RestUnifiedQueryAction, which has no such fallback. */ LEGACY_ENGINE_FALLBACK( - "A call shape only the legacy V1 engine understands (e.g. positional" - + " date_histogram(field=, ...), or an `alias` parameter) can't be answered on the" - + " analytics-engine route: reaching that engine depends on RestSQLQueryAction's" - + " SyntaxCheckException fallback, and the analytics route does not go through it."), + "A call shape only the legacy V1 engine understands (e.g. a date_histogram `alias`" + + " parameter) can't be answered on the analytics-engine route: reaching that engine" + + " depends on RestSQLQueryAction's SyntaxCheckException fallback, and the analytics" + + " route does not go through it."), + /** + * FRONTEND: legacy method-query syntax (regexp_query/wildcard_query) is not in the Calcite + * grammar. + */ LEGACY_METHOD_QUERY( "Legacy method-query syntax (regexp_query/wildcard_query/query/matchquery) is not in the" + " Calcite grammar used by the analytics-engine route."), diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java index 6ab74304532..4091d3ba52f 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java @@ -8,6 +8,7 @@ import static org.opensearch.sql.ast.dsl.AstDSL.between; import static org.opensearch.sql.ast.dsl.AstDSL.not; import static org.opensearch.sql.ast.dsl.AstDSL.qualifiedName; +import static org.opensearch.sql.expression.function.BuiltinFunctionName.IFNULL; import static org.opensearch.sql.expression.function.BuiltinFunctionName.IS_NOT_NULL; import static org.opensearch.sql.expression.function.BuiltinFunctionName.IS_NULL; import static org.opensearch.sql.expression.function.BuiltinFunctionName.LIKE; @@ -81,6 +82,7 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; import org.antlr.v4.runtime.RuleContext; @@ -110,6 +112,10 @@ /** Expression builder to parse text to expression in AST. */ public class AstExpressionBuilder extends OpenSearchSQLParserBaseVisitor { + /** Bucket parameters a span cannot express, which the legacy engine answers instead. */ + private static final Set LEGACY_ONLY_BUCKET_ARGS = + Set.of("alias", "format", "time_zone", "min_doc_count", "order"); + private final AstBuildGuard guard; public AstExpressionBuilder() { @@ -173,9 +179,9 @@ public UnresolvedExpression visitScalarFunctionCall(ScalarFunctionCallContext ct /** * Lowers {@code histogram} and {@code date_histogram} to a {@link Span} over the bucketed field. - * The grammar admits only the {@code 'name'=value} form, so the positional spelling the legacy - * engine has always answered never reaches here -- it stays an unknown scalar function, and - * RestSQLQueryAction hands it back to that engine. + * Parameters the span cannot express are implemented by the legacy engine through the native + * date_histogram aggregation, so those calls are declined as a syntax check to let + * RestSQLQueryAction hand them back to it. */ @Override public UnresolvedExpression visitBucketFunctionCall(BucketFunctionCallContext ctx) { @@ -193,13 +199,12 @@ public UnresolvedExpression visitBucketFunctionCall(BucketFunctionCallContext ct UnresolvedExpression missing = args.remove("missing"); Literal interval = intervalOf(args, functionName); - // Anything left is a parameter this lowering has no equivalent for. The legacy engine - // implements all of them -- alias, format, time_zone, min_doc_count, order -- through the - // native date_histogram aggregation, so decline in the one way RestSQLQueryAction falls back - // on rather than failing a query it can answer. if (!args.isEmpty()) { - throw new SyntaxCheckException( - functionName + " does not accept parameter: " + String.join(", ", args.keySet())); + String names = String.join(", ", args.keySet()); + if (LEGACY_ONLY_BUCKET_ARGS.containsAll(args.keySet())) { + throw new SyntaxCheckException(functionName + " does not accept parameter: " + names); + } + throw new SemanticCheckException(functionName + " does not accept parameter: " + names); } return AstDSL.spanFromSpanLengthLiteral( @@ -262,7 +267,9 @@ private static UnresolvedExpression normalizeField(UnresolvedExpression field) { */ private static UnresolvedExpression substituteMissing( UnresolvedExpression field, UnresolvedExpression missing) { - return missing == null ? field : new Function("ifnull", List.of(field, missing)); + return missing == null + ? field + : new Function(IFNULL.getName().getFunctionName(), List.of(field, missing)); } @Override diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java index 8298c81ab7d..2c21a3ad8b2 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java @@ -959,6 +959,20 @@ public void badBucketArgumentIsReportedRatherThanDeferred() { () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'interval'='2d')")); } + /** + * Bare argument names let anything parse, so a name the legacy engine does not implement either + * is a typo. Reporting it keeps a misspelling from silently becoming a legacy-engine query. + */ + @Test + public void unknownBucketParameterIsReportedRatherThanDeferred() { + assertThrows( + SemanticCheckException.class, + () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'feild'='ts')")); + assertThrows( + SemanticCheckException.class, + () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'alias'='d', 'nope'=1)")); + } + private Node buildExprAst(String expr) { return buildExprAst(expr, astExprBuilder); } From 3f6b51c948f18a60bdc6cc2f7c4a200afd1f47e4 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Wed, 19 Aug 2026 17:54:05 -0700 Subject: [PATCH 11/18] Fold the bucket argument checks into the visitor Review feedback: the validation read as more machinery than the other OpenSearch functions carry. The three helpers are gone -- the checks are inline, the two sets of parameter names are declared as data, and the messages now match the wording RelevanceQuery already uses ("Parameter %s is invalid for %s function.", "Parameter '%s' can only be specified once."). 69 lines to 52. On delegating the checks to execution instead: that works for the relevance functions because they survive as a FunctionExpression all the way to RelevanceQuery.build(), which is where their parameter table lives. A bucket call is lowered to a Span while the AST is being built, so nothing downstream still sees a function to check. What is left cannot be deferred either -- AstDSL.spanFromSpanLengthLiteral dereferences the interval on its first line, so a missing one is an NPE rather than a message. Parse-time lowering is also what keeps this one change serving both engines. Span is consumed independently by ExpressionAnalyzer, CompositeAggregationBuilder and Rounding on the V2 side, and by CalciteRexNodeVisitor and CalciteRelNodeVisitor on the analytics side -- and the Calcite path never goes through ExpressionAnalyzer, so the AST is the only point the two share. Keeping the call as a function would mean teaching each of those about it separately, and CompositeAggregationBuilder dispatches on `instanceof SpanExpression`, so a function would fall through to a terms aggregation instead of a histogram. This follows the span half of the earlier suggestion: PPL builds its span the same way, in visitSpanClause, through the same AstDSL call. Verified: 13 integration tests, none failing or skipped; `:sql:build` green including the coverage gate. Signed-off-by: Jialiang Liang --- .../sql/sql/parser/AstExpressionBuilder.java | 82 ++++++++----------- 1 file changed, 34 insertions(+), 48 deletions(-) diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java index 4091d3ba52f..cf606977782 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java @@ -84,7 +84,6 @@ import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; -import java.util.stream.Stream; import org.antlr.v4.runtime.RuleContext; import org.antlr.v4.runtime.tree.ParseTree; import org.antlr.v4.runtime.tree.RuleNode; @@ -112,8 +111,12 @@ /** Expression builder to parse text to expression in AST. */ public class AstExpressionBuilder extends OpenSearchSQLParserBaseVisitor { + /** Synonyms for the bucket width; exactly one is required. */ + private static final List INTERVAL_ARGS = + List.of("interval", "fixed_interval", "calendar_interval"); + /** Bucket parameters a span cannot express, which the legacy engine answers instead. */ - private static final Set LEGACY_ONLY_BUCKET_ARGS = + private static final Set LEGACY_ONLY_ARGS = Set.of("alias", "format", "time_zone", "min_doc_count", "order"); private final AstBuildGuard guard; @@ -191,65 +194,48 @@ public UnresolvedExpression visitBucketFunctionCall(BucketFunctionCallContext ct for (BucketArgContext arg : ctx.bucketFunction().bucketArg()) { String name = StringUtils.unquoteText(arg.bucketArgName().getText()).toLowerCase(Locale.ROOT); if (args.put(name, visit(arg.bucketArgValue())) != null) { - throw new SemanticCheckException("Duplicate parameter: " + name); + throw new SemanticCheckException( + String.format("Parameter '%s' can only be specified once.", name)); } } - UnresolvedExpression field = requireArg(args, "field", functionName); + UnresolvedExpression field = args.remove("field"); UnresolvedExpression missing = args.remove("missing"); - Literal interval = intervalOf(args, functionName); + List intervals = + INTERVAL_ARGS.stream() + .map(args::remove) + .filter(Objects::nonNull) + .collect(Collectors.toList()); + // Whatever is left is a parameter this lowering has no equivalent for. The ones the legacy + // engine implements are declined as a syntax check so RestSQLQueryAction hands the query to + // it; anything else is a misspelling and is reported. if (!args.isEmpty()) { - String names = String.join(", ", args.keySet()); - if (LEGACY_ONLY_BUCKET_ARGS.containsAll(args.keySet())) { - throw new SyntaxCheckException(functionName + " does not accept parameter: " + names); - } - throw new SemanticCheckException(functionName + " does not accept parameter: " + names); + String message = + String.format( + "Parameter %s is invalid for %s function.", + String.join(", ", args.keySet()), functionName); + throw LEGACY_ONLY_ARGS.containsAll(args.keySet()) + ? new SyntaxCheckException(message) + : new SemanticCheckException(message); } - - return AstDSL.spanFromSpanLengthLiteral( - substituteMissing(normalizeField(field), missing), interval); - } - - private static UnresolvedExpression requireArg( - Map args, String name, String functionName) { - UnresolvedExpression value = args.remove(name); - if (value == null) { - throw new SemanticCheckException(functionName + " requires " + name + " parameter"); + if (field == null) { + throw new SemanticCheckException( + String.format("Parameter field is required for %s function.", functionName)); } - return value; - } - - /** - * {@code interval}, {@code fixed_interval} and {@code calendar_interval} are synonyms; exactly - * one must be present. The distinction between calendar and fixed intervals is not preserved. - */ - private static Literal intervalOf(Map args, String functionName) { - List supplied = - Stream.of("interval", "fixed_interval", "calendar_interval") - .map(key -> stringOrNumericArg(args, key)) - .filter(Objects::nonNull) - .collect(Collectors.toList()); - if (supplied.isEmpty()) { + if (intervals.size() != 1) { throw new SemanticCheckException( - functionName + " requires one of: interval, fixed_interval, calendar_interval"); + String.format( + "Exactly one of %s is required for %s function.", + String.join(", ", INTERVAL_ARGS), functionName)); } - if (supplied.size() > 1) { + if (!(intervals.get(0) instanceof Literal interval)) { throw new SemanticCheckException( - functionName + " accepts only one of: interval, fixed_interval, calendar_interval"); + String.format("Parameter interval must be a literal for %s function.", functionName)); } - return supplied.get(0); - } - private static Literal stringOrNumericArg(Map 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); - } - return literal; + return AstDSL.spanFromSpanLengthLiteral( + substituteMissing(normalizeField(field), missing), interval); } /** A string literal naming a column is coerced so downstream sees a column reference. */ From 7950169e3c2654e42db603fa1863ada5c48986d7 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Wed, 19 Aug 2026 19:07:13 -0700 Subject: [PATCH 12/18] Drop an unreachable bucket argument name `bucketArgName` listed MISSING among the reserved words it accepts, but the lexer never emits that token: MISSING_LITERAL matches the same text and is declared first, so the alternative could not be reached. Confirmed against a running cluster -- `missing=0` written bare is declined by the V2 parser and handed to the legacy engine, while `'missing'=0` in quotes works and stays on the V2 path, which is the spelling the integration test already uses. The other three reserved words are reachable and stay: `interval=` answers directly, and `order=`/`time_zone=` reach the builder and are declined there by name, as intended. Verified: 13 integration tests, none failing or skipped; `:sql:build` green including the coverage gate. Signed-off-by: Jialiang Liang --- language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 | 1 - sql/src/main/antlr/OpenSearchSQLParser.g4 | 1 - 2 files changed, 2 deletions(-) diff --git a/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 b/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 index ad1e88214b1..027424d334a 100644 --- a/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 +++ b/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 @@ -774,7 +774,6 @@ bucketArgName : stringLiteral | ident | INTERVAL - | MISSING | ORDER | TIME_ZONE ; diff --git a/sql/src/main/antlr/OpenSearchSQLParser.g4 b/sql/src/main/antlr/OpenSearchSQLParser.g4 index 35b98ec1dc7..fef69a3692c 100644 --- a/sql/src/main/antlr/OpenSearchSQLParser.g4 +++ b/sql/src/main/antlr/OpenSearchSQLParser.g4 @@ -807,7 +807,6 @@ bucketArgName : stringLiteral | ident | INTERVAL - | MISSING | ORDER | TIME_ZONE ; From 448b3ad50cfca479351a1af3a5457e1b897e0ccb Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Thu, 20 Aug 2026 10:34:36 -0700 Subject: [PATCH 13/18] Keep deferring the bucket parameters legacy implements `LEGACY_ONLY_ARGS` listed five names, but AggMaker accepts nine on the bucket aggregations: `children`, `extended_bounds`, `nested` and `reverse_nested` were missing. Those four reached the leftover-argument branch, failed the `containsAll` check and were reported as semantic errors -- and RestSQLQueryAction only falls back on a syntax check, so the query stopped short of the engine that implements them. Before the grammar rule existed these calls were a V2 syntax error and legacy answered them, so this was a regression introduced by defining the function here. Confirmed against a running cluster: with the four names registered, `date_histogram('field'='ts','fixed_interval'='1h','extended_bounds'='0:100')` reaches legacy again, matching `alias`. The set is now the union of what AggMaker.dateHistogram and AggMaker.histogram accept, minus the parameters this lowering handles itself. Verified: 13 integration tests, none failing or skipped; `:sql:build` green including the coverage gate. Signed-off-by: Jialiang Liang --- .../sql/sql/parser/AstExpressionBuilder.java | 13 +++++++++++-- .../sql/sql/parser/AstExpressionBuilderTest.java | 7 +++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java index cf606977782..8b4d600f3d7 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java @@ -115,9 +115,18 @@ public class AstExpressionBuilder extends OpenSearchSQLParserBaseVisitor INTERVAL_ARGS = List.of("interval", "fixed_interval", "calendar_interval"); - /** Bucket parameters a span cannot express, which the legacy engine answers instead. */ + /** Bucket parameters a span cannot express; AggMaker implements these on the legacy engine. */ private static final Set LEGACY_ONLY_ARGS = - Set.of("alias", "format", "time_zone", "min_doc_count", "order"); + Set.of( + "alias", + "children", + "extended_bounds", + "format", + "min_doc_count", + "nested", + "order", + "reverse_nested", + "time_zone"); private final AstBuildGuard guard; diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java index 2c21a3ad8b2..4db301c37b8 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java @@ -940,6 +940,13 @@ public void unsupportedBucketParameterDefersToLegacyEngine() { assertThrows( SyntaxCheckException.class, () -> buildExprAst("date_histogram('field'=ts, 'interval'='1h', 'time_zone'='+05:30')")); + for (String name : List.of("children", "extended_bounds", "nested", "reverse_nested")) { + assertThrows( + SyntaxCheckException.class, + () -> + buildExprAst( + String.format("date_histogram('field'=ts, 'interval'='1d', '%s'='x')", name))); + } } /** A bad argument inside a shape we own must not fall back, so the caller sees this message. */ From f3aaf4fd1881e955ae74ef7831840437dcee3d55 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Thu, 20 Aug 2026 10:39:52 -0700 Subject: [PATCH 14/18] Follow the conventions this repo already has for the bucket tests Three things the surrounding code already had a way of doing. The mapping for the test index was an inline JSON string, which the formatter had split mid-token. It is a file under `indexDefinitions/` now, loaded with `getMappingFile` -- 72 of the 76 index entries take their mapping from a file or a helper rather than a literal. Four tests were rebuilding the string `bucketed()` already produces; they call it now. The message for a parameter that belongs to the legacy engine says so, matching the five other places that decline this way -- `AstBuilder` for JOIN, UNION and a nested function in HAVING, and this file for IN and EXISTS subqueries. The message on the semantic branch is unchanged, since it is a real error rather than a handoff. Verified: 13 integration tests, none failing or skipped; `:sql:build` green including the coverage gate. Signed-off-by: Jialiang Liang --- .../sql/legacy/SQLIntegTestCase.java | 3 +-- .../sql/DateHistogramBucketFunctionIT.java | 24 ++++--------------- .../date_histogram_test_index_mapping.json | 16 +++++++++++++ .../sql/sql/parser/AstExpressionBuilder.java | 17 +++++++------ 4 files changed, 31 insertions(+), 29 deletions(-) create mode 100644 integ-test/src/test/resources/indexDefinitions/date_histogram_test_index_mapping.json diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java index e0f4b53e3f6..2bd1f98a5ee 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java @@ -998,8 +998,7 @@ public enum Index { DATE_HISTOGRAM_TEST( "date_histogram_test", "date_histogram_test", - "{\"mappings\":{\"properties\":{\"ts\":{\"type\":\"date\",\"format\":\"yyyy-MM-dd" - + " HH:mm:ss\"},\"category\":{\"type\":\"keyword\"},\"value\":{\"type\":\"integer\"}}}}", + getMappingFile("date_histogram_test_index_mapping.json"), "src/test/resources/date_histogram_test.json"); private final String name; diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java index d85cee7676e..b913f17a25a 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java @@ -152,22 +152,14 @@ public void bucketsRespectAWhereClause() throws IOException { @Test public void missingParameterSubstitutesBeforeBucketing() throws IOException { JSONObject response = - executeQuery( - "SELECT b, COUNT(*) FROM (SELECT histogram('field'=value, 'interval'=20, 'missing'=0)" - + " AS b FROM " - + IDX - + ") sub GROUP BY b ORDER BY b"); + executeQuery(bucketed("histogram('field'=value, 'interval'=20, 'missing'=0)")); verifyDataRowsInOrder(response, rows(0, 19), rows(20, 20), rows(40, 20), rows(60, 13)); } @Test public void numericHistogramBucketsByInterval() throws IOException { - JSONObject response = - executeQuery( - "SELECT b, COUNT(*) FROM (SELECT histogram('field'=value, 'interval'=20) AS b FROM " - + IDX - + ") sub GROUP BY b ORDER BY b"); + JSONObject response = executeQuery(bucketed("histogram('field'=value, 'interval'=20)")); // value runs 1..72, so the 20-wide buckets hold 19, 20, 20 and 13 documents. verifyDataRowsInOrder(response, rows(0, 19), rows(20, 20), rows(40, 20), rows(60, 13)); @@ -176,11 +168,7 @@ public void numericHistogramBucketsByInterval() throws IOException { /** Argument names may be written bare, the spelling the legacy engine has always accepted. */ @Test public void unquotedArgumentNamesReturnHourlyBuckets() throws IOException { - JSONObject response = - executeQuery( - "SELECT b, COUNT(*) FROM (SELECT date_histogram(field=ts, interval='1h') AS b FROM " - + IDX - + ") sub GROUP BY b ORDER BY b"); + JSONObject response = executeQuery(bucketed("date_histogram(field=ts, interval='1h')")); verifyDataRowsInOrder( response, @@ -209,11 +197,7 @@ public void callWithAliasParameterReturnsHourlyBuckets() throws IOException { @Test public void unquotedArgumentNamesReturnNumericBuckets() throws IOException { - JSONObject response = - executeQuery( - "SELECT b, COUNT(*) FROM (SELECT histogram(field=value, interval=20) AS b FROM " - + IDX - + ") sub GROUP BY b ORDER BY b"); + JSONObject response = executeQuery(bucketed("histogram(field=value, interval=20)")); verifyDataRowsInOrder(response, rows(0, 19), rows(20, 20), rows(40, 20), rows(60, 13)); } diff --git a/integ-test/src/test/resources/indexDefinitions/date_histogram_test_index_mapping.json b/integ-test/src/test/resources/indexDefinitions/date_histogram_test_index_mapping.json new file mode 100644 index 00000000000..d8538083641 --- /dev/null +++ b/integ-test/src/test/resources/indexDefinitions/date_histogram_test_index_mapping.json @@ -0,0 +1,16 @@ +{ + "mappings": { + "properties": { + "ts": { + "type": "date", + "format": "yyyy-MM-dd HH:mm:ss" + }, + "category": { + "type": "keyword" + }, + "value": { + "type": "integer" + } + } + } +} diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java index 8b4d600f3d7..4d0894c4d29 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java @@ -220,13 +220,16 @@ public UnresolvedExpression visitBucketFunctionCall(BucketFunctionCallContext ct // engine implements are declined as a syntax check so RestSQLQueryAction hands the query to // it; anything else is a misspelling and is reported. if (!args.isEmpty()) { - String message = - String.format( - "Parameter %s is invalid for %s function.", - String.join(", ", args.keySet()), functionName); - throw LEGACY_ONLY_ARGS.containsAll(args.keySet()) - ? new SyntaxCheckException(message) - : new SemanticCheckException(message); + String names = String.join(", ", args.keySet()); + if (LEGACY_ONLY_ARGS.containsAll(args.keySet())) { + throw new SyntaxCheckException( + String.format( + "Parameter %s of %s is not supported in the V2 SQL engine. Falling back to legacy" + + " engine.", + names, functionName)); + } + throw new SemanticCheckException( + String.format("Parameter %s is invalid for %s function.", names, functionName)); } if (field == null) { throw new SemanticCheckException( From 9d798fca04aef7e21949f07a0284a2ecb731a7b0 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Thu, 20 Aug 2026 13:11:07 -0700 Subject: [PATCH 15/18] Let the grammar decide which bucket parameters we answer Review feedback: the argument-name rule was open where the other OpenSearch functions enumerate their names, and the accepted set had to be mirrored in Java as a result. `bucketArgName` lists the four names this lowering handles -- `field`, `interval`, `fixed_interval`, `calendar_interval` -- so anything else is a parse error, which is the one exception RestSQLQueryAction falls back on. The handoff is the grammar's now, not a table's. `LEGACY_ONLY_ARGS` is gone with it, and so is the leftover-argument branch: once the four names are taken out of the map it is always empty. What remains are the three checks that cannot move downstream, because `spanFromSpanLengthLiteral` dereferences the interval on its first line. This also removes the failure mode behind the previous commit. That set had to list every parameter the legacy engine implements, and four were missing; with the grammar deciding, a name nobody listed falls back on its own. Two consequences worth stating. The quoted spelling now reaches the legacy engine rather than being lowered here -- which is where it went before this function was defined at all, so nothing that used to work stops working. And `missing` is dropped: `AggMaker` does not implement it either, so there is nothing to defer to, and the `MISSING` token was unreachable behind `MISSING_LITERAL` (ANTLR warns about this directly). `FIXED_INTERVAL` and `CALENDAR_INTERVAL` are new tokens, added to `keywordsCanBeId` so they can still name a column. Verified: 82 parser unit tests, none failing; `:sql:build` green including the coverage gate. Integration tests could not run locally -- the 3.9.0 distro no longer bundles Jackson 2.x, so the plugin fails to install with jar hell on `main` as well, pending #5703. Signed-off-by: Jialiang Liang --- .../sql/DateHistogramBucketFunctionIT.java | 58 +++++-------------- .../src/main/antlr4/OpenSearchSQLLexer.g4 | 2 + .../src/main/antlr4/OpenSearchSQLParser.g4 | 9 +-- sql/src/main/antlr/OpenSearchSQLLexer.g4 | 2 + sql/src/main/antlr/OpenSearchSQLParser.g4 | 9 +-- .../sql/sql/parser/AstExpressionBuilder.java | 48 +-------------- .../sql/parser/AstExpressionBuilderTest.java | 58 +++++++------------ 7 files changed, 49 insertions(+), 137 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java index b913f17a25a..939e79f71f0 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java @@ -54,7 +54,7 @@ private static String bucketed(String bucketExpr) { @Test public void hourlyBucketsCarryKeysAndCounts() throws IOException { - JSONObject response = executeQuery(bucketed("date_histogram('field'=ts, 'interval'='1h')")); + JSONObject response = executeQuery(bucketed("date_histogram(field=ts, interval='1h')")); verifyDataRowsInOrder( response, @@ -67,7 +67,7 @@ public void hourlyBucketsCarryKeysAndCounts() throws IOException { /** A sub-hour interval must split 00:00/00:30 and 01:00/01:45 rather than merge them. */ @Test public void halfHourlyBucketsSplitWithinTheHour() throws IOException { - JSONObject response = executeQuery(bucketed("date_histogram('field'=ts, 'interval'='30m')")); + JSONObject response = executeQuery(bucketed("date_histogram(field=ts, interval='30m')")); verifyDataRowsInOrder( response, @@ -81,7 +81,7 @@ public void halfHourlyBucketsSplitWithinTheHour() throws IOException { @Test public void dailyIntervalCollapsesEverythingIntoOneBucket() throws IOException { - JSONObject response = executeQuery(bucketed("date_histogram('field'=ts, 'interval'='1d')")); + JSONObject response = executeQuery(bucketed("date_histogram(field=ts, interval='1d')")); verifyDataRows(response, rows("2026-01-01 00:00:00", 72)); } @@ -89,10 +89,9 @@ public void dailyIntervalCollapsesEverythingIntoOneBucket() throws IOException { /** {@code fixed_interval} and {@code calendar_interval} are accepted as synonyms of interval. */ @Test public void intervalSynonymsProduceTheSameBuckets() throws IOException { - JSONObject viaFixed = - executeQuery(bucketed("date_histogram('field'=ts, 'fixed_interval'='1h')")); + JSONObject viaFixed = executeQuery(bucketed("date_histogram(field=ts, fixed_interval='1h')")); JSONObject viaCalendar = - executeQuery(bucketed("date_histogram('field'=ts, 'calendar_interval'='1h')")); + executeQuery(bucketed("date_histogram(field=ts, calendar_interval='1h')")); for (JSONObject response : new JSONObject[] {viaFixed, viaCalendar}) { verifyDataRowsInOrder( @@ -109,7 +108,7 @@ public void intervalSynonymsProduceTheSameBuckets() throws IOException { public void bucketsCombineWithAnAdditionalGroupingKey() throws IOException { JSONObject response = executeQuery( - "SELECT b, c, COUNT(*) FROM (SELECT date_histogram('field'=ts, 'interval'='1h') AS b," + "SELECT b, c, COUNT(*) FROM (SELECT date_histogram(field=ts, interval='1h') AS b," + " category AS c FROM (SELECT * FROM " + IDX + ") inner_scan) sub GROUP BY b, c ORDER BY b, c"); @@ -128,7 +127,7 @@ public void bucketsCombineWithAnAdditionalGroupingKey() throws IOException { public void bucketsRespectAWhereClause() throws IOException { JSONObject response = executeQuery( - "SELECT b, COUNT(*) FROM (SELECT date_histogram('field'=ts, 'interval'='1h') AS b FROM " + "SELECT b, COUNT(*) FROM (SELECT date_histogram(field=ts, interval='1h') AS b FROM " + IDX + " WHERE category = 'alpha') sub GROUP BY b ORDER BY b"); @@ -139,45 +138,14 @@ public void bucketsRespectAWhereClause() throws IOException { rows("2026-01-01 03:00:00", 19)); } - /** - * `missing` substitutes a value for a null field before bucketing. Asserted end to end because - * the AST alone cannot show whether the substitution function is one the engine evaluates — - * `coalesce` is a registered name with no V2 implementation, `ifnull` is the one that runs. - * - *

Uses the numeric field: substituting into a date needs a timestamp-typed replacement, and - * the grammar admits only literals here, so a date `missing` reaches IFNULL as TIMESTAMP against - * STRING. V2 accepts that pair, the analytics engine rejects it, and a test asserting either - * result would disagree with the other route. - */ - @Test - public void missingParameterSubstitutesBeforeBucketing() throws IOException { - JSONObject response = - executeQuery(bucketed("histogram('field'=value, 'interval'=20, 'missing'=0)")); - - verifyDataRowsInOrder(response, rows(0, 19), rows(20, 20), rows(40, 20), rows(60, 13)); - } - @Test public void numericHistogramBucketsByInterval() throws IOException { - JSONObject response = executeQuery(bucketed("histogram('field'=value, 'interval'=20)")); + JSONObject response = executeQuery(bucketed("histogram(field=value, interval=20)")); // value runs 1..72, so the 20-wide buckets hold 19, 20, 20 and 13 documents. verifyDataRowsInOrder(response, rows(0, 19), rows(20, 20), rows(40, 20), rows(60, 13)); } - /** Argument names may be written bare, the spelling the legacy engine has always accepted. */ - @Test - public void unquotedArgumentNamesReturnHourlyBuckets() throws IOException { - JSONObject response = executeQuery(bucketed("date_histogram(field=ts, interval='1h')")); - - verifyDataRowsInOrder( - response, - rows("2026-01-01 00:00:00", 12), - rows("2026-01-01 01:00:00", 24), - rows("2026-01-01 02:00:00", 17), - rows("2026-01-01 03:00:00", 19)); - } - /** * The legacy engine implements alias, format, time_zone, min_doc_count and order through the * native date_histogram aggregation; this lowering has no equivalent, so those queries still have @@ -190,7 +158,7 @@ public void callWithAliasParameterReturnsHourlyBuckets() throws IOException { executeQuery( "SELECT COUNT(*) FROM " + IDX - + " GROUP BY date_histogram('field'='ts','fixed_interval'='1h','alias'='hours')"); + + " GROUP BY date_histogram(field='ts',fixed_interval='1h','alias'='hours')"); verifyDataRows(response, rows(12), rows(24), rows(17), rows(19)); } @@ -213,19 +181,19 @@ public void groupingOnTheBucketWithoutADerivedTableIsRejected() { ResponseException.class, () -> executeQuery( - "SELECT date_histogram('field'=ts, 'interval'='1h') AS b, COUNT(*) FROM " + "SELECT date_histogram(field=ts, interval='1h') AS b, COUNT(*) FROM " + IDX - + " GROUP BY date_histogram('field'=ts, 'interval'='1h')")); + + " GROUP BY date_histogram(field=ts, interval='1h')")); } /** Calendar units: Dashboards emits 1M and 1y at the wider zoom levels. */ @Test public void calendarIntervalsBucketByMonthAndYear() throws IOException { verifyDataRows( - executeQuery(bucketed("date_histogram('field'=ts, 'interval'='1M')")), + executeQuery(bucketed("date_histogram(field=ts, interval='1M')")), rows("2026-01-01 00:00:00", 72)); verifyDataRows( - executeQuery(bucketed("date_histogram('field'=ts, 'interval'='1y')")), + executeQuery(bucketed("date_histogram(field=ts, interval='1y')")), rows("2026-01-01 00:00:00", 72)); } } diff --git a/language-grammar/src/main/antlr4/OpenSearchSQLLexer.g4 b/language-grammar/src/main/antlr4/OpenSearchSQLLexer.g4 index ba7c5be85ab..9960908869f 100644 --- a/language-grammar/src/main/antlr4/OpenSearchSQLLexer.g4 +++ b/language-grammar/src/main/antlr4/OpenSearchSQLLexer.g4 @@ -143,6 +143,8 @@ OFFSET: 'OFFSET'; // INTERVAL AND UNIT KEYWORDS INTERVAL: 'INTERVAL'; +FIXED_INTERVAL: 'FIXED_INTERVAL'; +CALENDAR_INTERVAL: 'CALENDAR_INTERVAL'; MICROSECOND: 'MICROSECOND'; SECOND: 'SECOND'; MINUTE: 'MINUTE'; diff --git a/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 b/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 index 027424d334a..ae1d032eee3 100644 --- a/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 +++ b/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 @@ -771,11 +771,10 @@ highlightArgName ; bucketArgName - : stringLiteral - | ident + : FIELD | INTERVAL - | ORDER - | TIME_ZONE + | FIXED_INTERVAL + | CALENDAR_INTERVAL ; relevanceFieldAndWeight @@ -857,6 +856,8 @@ ident keywordsCanBeId : FULL | FIELD + | FIXED_INTERVAL + | CALENDAR_INTERVAL | D | T | TS // OD SQL and ODBC special diff --git a/sql/src/main/antlr/OpenSearchSQLLexer.g4 b/sql/src/main/antlr/OpenSearchSQLLexer.g4 index ba7c5be85ab..9960908869f 100644 --- a/sql/src/main/antlr/OpenSearchSQLLexer.g4 +++ b/sql/src/main/antlr/OpenSearchSQLLexer.g4 @@ -143,6 +143,8 @@ OFFSET: 'OFFSET'; // INTERVAL AND UNIT KEYWORDS INTERVAL: 'INTERVAL'; +FIXED_INTERVAL: 'FIXED_INTERVAL'; +CALENDAR_INTERVAL: 'CALENDAR_INTERVAL'; MICROSECOND: 'MICROSECOND'; SECOND: 'SECOND'; MINUTE: 'MINUTE'; diff --git a/sql/src/main/antlr/OpenSearchSQLParser.g4 b/sql/src/main/antlr/OpenSearchSQLParser.g4 index fef69a3692c..1fab2554c60 100644 --- a/sql/src/main/antlr/OpenSearchSQLParser.g4 +++ b/sql/src/main/antlr/OpenSearchSQLParser.g4 @@ -804,11 +804,10 @@ highlightArgName ; bucketArgName - : stringLiteral - | ident + : FIELD | INTERVAL - | ORDER - | TIME_ZONE + | FIXED_INTERVAL + | CALENDAR_INTERVAL ; relevanceFieldAndWeight @@ -890,6 +889,8 @@ ident keywordsCanBeId : FULL | FIELD + | FIXED_INTERVAL + | CALENDAR_INTERVAL | D | T | TS // OD SQL and ODBC special diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java index 4d0894c4d29..a431a8fc1a1 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java @@ -8,7 +8,6 @@ import static org.opensearch.sql.ast.dsl.AstDSL.between; import static org.opensearch.sql.ast.dsl.AstDSL.not; import static org.opensearch.sql.ast.dsl.AstDSL.qualifiedName; -import static org.opensearch.sql.expression.function.BuiltinFunctionName.IFNULL; import static org.opensearch.sql.expression.function.BuiltinFunctionName.IS_NOT_NULL; import static org.opensearch.sql.expression.function.BuiltinFunctionName.IS_NULL; import static org.opensearch.sql.expression.function.BuiltinFunctionName.LIKE; @@ -82,7 +81,6 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; -import java.util.Set; import java.util.stream.Collectors; import org.antlr.v4.runtime.RuleContext; import org.antlr.v4.runtime.tree.ParseTree; @@ -115,19 +113,6 @@ public class AstExpressionBuilder extends OpenSearchSQLParserBaseVisitor INTERVAL_ARGS = List.of("interval", "fixed_interval", "calendar_interval"); - /** Bucket parameters a span cannot express; AggMaker implements these on the legacy engine. */ - private static final Set LEGACY_ONLY_ARGS = - Set.of( - "alias", - "children", - "extended_bounds", - "format", - "min_doc_count", - "nested", - "order", - "reverse_nested", - "time_zone"); - private final AstBuildGuard guard; public AstExpressionBuilder() { @@ -201,7 +186,7 @@ public UnresolvedExpression visitBucketFunctionCall(BucketFunctionCallContext ct ctx.bucketFunction().bucketFunctionName().getText().toLowerCase(Locale.ROOT); Map args = new LinkedHashMap<>(); for (BucketArgContext arg : ctx.bucketFunction().bucketArg()) { - String name = StringUtils.unquoteText(arg.bucketArgName().getText()).toLowerCase(Locale.ROOT); + String name = arg.bucketArgName().getText().toLowerCase(Locale.ROOT); if (args.put(name, visit(arg.bucketArgValue())) != null) { throw new SemanticCheckException( String.format("Parameter '%s' can only be specified once.", name)); @@ -209,28 +194,12 @@ public UnresolvedExpression visitBucketFunctionCall(BucketFunctionCallContext ct } UnresolvedExpression field = args.remove("field"); - UnresolvedExpression missing = args.remove("missing"); List intervals = INTERVAL_ARGS.stream() .map(args::remove) .filter(Objects::nonNull) .collect(Collectors.toList()); - // Whatever is left is a parameter this lowering has no equivalent for. The ones the legacy - // engine implements are declined as a syntax check so RestSQLQueryAction hands the query to - // it; anything else is a misspelling and is reported. - if (!args.isEmpty()) { - String names = String.join(", ", args.keySet()); - if (LEGACY_ONLY_ARGS.containsAll(args.keySet())) { - throw new SyntaxCheckException( - String.format( - "Parameter %s of %s is not supported in the V2 SQL engine. Falling back to legacy" - + " engine.", - names, functionName)); - } - throw new SemanticCheckException( - String.format("Parameter %s is invalid for %s function.", names, functionName)); - } if (field == null) { throw new SemanticCheckException( String.format("Parameter field is required for %s function.", functionName)); @@ -246,8 +215,7 @@ public UnresolvedExpression visitBucketFunctionCall(BucketFunctionCallContext ct String.format("Parameter interval must be a literal for %s function.", functionName)); } - return AstDSL.spanFromSpanLengthLiteral( - substituteMissing(normalizeField(field), missing), interval); + return AstDSL.spanFromSpanLengthLiteral(normalizeField(field), interval); } /** A string literal naming a column is coerced so downstream sees a column reference. */ @@ -258,18 +226,6 @@ private static UnresolvedExpression normalizeField(UnresolvedExpression field) { return field; } - /** - * 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)); - } - @Override public UnresolvedExpression visitGetFormatFunctionCall(GetFormatFunctionCallContext ctx) { return new Function( diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java index 4db301c37b8..9cd8de109c9 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java @@ -868,7 +868,7 @@ private static String nest(int depth, String base, UnaryOperator wrap) { public void canBuildDateHistogramAsSpan() { assertEquals( new Span(qualifiedName("ts"), intLiteral(1), SpanUnit.H), - buildExprAst("date_histogram('field'=ts, 'interval'='1h')")); + buildExprAst("date_histogram(field=ts, interval='1h')")); } /** Bare argument names are the spelling the legacy engine accepts; both forms lower alike. */ @@ -885,15 +885,15 @@ public void canBuildBucketFunctionWithUnquotedArgumentNames() { @Test public void canBuildDateHistogramWithIntervalSynonyms() { Span expected = new Span(qualifiedName("ts"), intLiteral(1), SpanUnit.D); - assertEquals(expected, buildExprAst("date_histogram('field'=ts, 'fixed_interval'='1d')")); - assertEquals(expected, buildExprAst("date_histogram('field'=ts, 'calendar_interval'='1d')")); + assertEquals(expected, buildExprAst("date_histogram(field=ts, fixed_interval='1d')")); + assertEquals(expected, buildExprAst("date_histogram(field=ts, calendar_interval='1d')")); } @Test public void canBuildDateHistogramWithStringFieldName() { assertEquals( new Span(qualifiedName("ts"), intLiteral(30), SpanUnit.m), - buildExprAst("date_histogram('field'='ts', 'interval'='30m')")); + buildExprAst("date_histogram(field='ts', interval='30m')")); } /** A numeric literal field is left alone rather than coerced to a column reference. */ @@ -901,24 +901,14 @@ public void canBuildDateHistogramWithStringFieldName() { public void bucketFieldGivenNonStringLiteralIsPassedThrough() { assertEquals( new Span(intLiteral(1), intLiteral(10), SpanUnit.NONE), - buildExprAst("histogram('field'=1, 'interval'=10)")); + buildExprAst("histogram(field=1, interval=10)")); } @Test public void canBuildNumericHistogramAsSpan() { assertEquals( new Span(qualifiedName("age"), intLiteral(10), SpanUnit.NONE), - buildExprAst("histogram('field'=age, 'interval'=10)")); - } - - @Test - public void canBuildDateHistogramWithMissing() { - assertEquals( - new Span( - function("ifnull", qualifiedName("ts"), stringLiteral("1970-01-01")), - intLiteral(1), - SpanUnit.H), - buildExprAst("date_histogram('field'=ts, 'interval'='1h', 'missing'='1970-01-01')")); + buildExprAst("histogram(field=age, interval=10)")); } /** @@ -930,54 +920,46 @@ public void canBuildDateHistogramWithMissing() { public void unsupportedBucketParameterDefersToLegacyEngine() { assertThrows( SyntaxCheckException.class, - () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'alias'='days')")); + () -> buildExprAst("date_histogram(field=ts, interval='1d', 'alias'='days')")); assertThrows( SyntaxCheckException.class, - () -> buildExprAst("histogram('field'=age, 'interval'=10, 'min_doc_count'=1)")); + () -> buildExprAst("histogram(field=age, interval=10, 'min_doc_count'=1)")); assertThrows( SyntaxCheckException.class, - () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'format'='yyyy-MM-dd')")); + () -> buildExprAst("date_histogram(field=ts, interval='1d', 'format'='yyyy-MM-dd')")); assertThrows( SyntaxCheckException.class, - () -> buildExprAst("date_histogram('field'=ts, 'interval'='1h', 'time_zone'='+05:30')")); + () -> buildExprAst("date_histogram(field=ts, interval='1h', 'time_zone'='+05:30')")); for (String name : List.of("children", "extended_bounds", "nested", "reverse_nested")) { assertThrows( SyntaxCheckException.class, () -> buildExprAst( - String.format("date_histogram('field'=ts, 'interval'='1d', '%s'='x')", name))); + String.format("date_histogram(field=ts, interval='1d', '%s'='x')", name))); } } /** A bad argument inside a shape we own must not fall back, so the caller sees this message. */ @Test public void badBucketArgumentIsReportedRatherThanDeferred() { - assertThrows( - SemanticCheckException.class, () -> buildExprAst("date_histogram('interval'='1d')")); - assertThrows(SemanticCheckException.class, () -> buildExprAst("date_histogram('field'=ts)")); + assertThrows(SemanticCheckException.class, () -> buildExprAst("date_histogram(interval='1d')")); + assertThrows(SemanticCheckException.class, () -> buildExprAst("date_histogram(field=ts)")); assertThrows( SemanticCheckException.class, - () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'fixed_interval'='2d')")); + () -> buildExprAst("date_histogram(field=ts, interval='1d', fixed_interval='2d')")); assertThrows( - SemanticCheckException.class, - () -> buildExprAst("date_histogram('field'=ts, 'interval'=ts)")); + SemanticCheckException.class, () -> buildExprAst("date_histogram(field=ts, interval=ts)")); assertThrows( SemanticCheckException.class, - () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'interval'='2d')")); + () -> buildExprAst("date_histogram(field=ts, interval='1d', interval='2d')")); } - /** - * Bare argument names let anything parse, so a name the legacy engine does not implement either - * is a typo. Reporting it keeps a misspelling from silently becoming a legacy-engine query. - */ + /** A name the grammar does not list is a parse error, which routes to the legacy engine. */ @Test - public void unknownBucketParameterIsReportedRatherThanDeferred() { + public void unknownBucketParameterIsASyntaxError() { assertThrows( - SemanticCheckException.class, - () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'feild'='ts')")); - assertThrows( - SemanticCheckException.class, - () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'alias'='d', 'nope'=1)")); + SyntaxCheckException.class, + () -> buildExprAst("date_histogram(field=ts, interval='1d', 'feild'='ts')")); } private Node buildExprAst(String expr) { From ed7284e88627cb4665c194021920e5509b574417 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Thu, 20 Aug 2026 14:14:52 -0700 Subject: [PATCH 16/18] Build the bucket span the way PPL builds its own Follow-up on the two set fields. `bucketFunction` now spells out both operands the way `spanClause` does in the PPL grammar -- the field and the interval are positional and required, with named labels to reach them -- so the visitor is the same three lines `visitSpanClause` is, through the same AstDSL call. Both sets are gone, and so is everything they supported: no argument map, no required-parameter checks, no duplicate detection, no literal check. The grammar makes each of those unrepresentable rather than detectable. AstExpressionBuilder loses 46 lines and gains 6. The one constraint this adds is ordering: `field` comes first. Writing the interval first is a parse error, so it reaches the legacy engine, which accepts either order. Verified on a live cluster, both the default and analytics routes: `interval`, `fixed_interval`, `calendar_interval`, the numeric `histogram`, and the shape Dashboards emits all return the same buckets on each; a reversed argument order falls back; `alias` still reaches legacy on the default route. 82 parser unit tests, none failing; `:sql:build` green including the coverage gate. Signed-off-by: Jialiang Liang --- .../src/main/antlr4/OpenSearchSQLParser.g4 | 16 +++--- sql/src/main/antlr/OpenSearchSQLParser.g4 | 16 +++--- .../sql/sql/parser/AstExpressionBuilder.java | 52 +++---------------- .../sql/parser/AstExpressionBuilderTest.java | 24 ++++----- 4 files changed, 29 insertions(+), 79 deletions(-) diff --git a/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 b/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 index ae1d032eee3..947bcb3bf64 100644 --- a/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 +++ b/language-grammar/src/main/antlr4/OpenSearchSQLParser.g4 @@ -398,11 +398,14 @@ highlightFunction ; bucketFunction - : bucketFunctionName LR_BRACKET bucketArg (COMMA bucketArg)* RR_BRACKET + : bucketFunctionName LR_BRACKET FIELD EQUAL_SYMBOL field = bucketArgValue COMMA + intervalArgName EQUAL_SYMBOL interval = constant RR_BRACKET ; -bucketArg - : bucketArgName EQUAL_SYMBOL bucketArgValue +intervalArgName + : INTERVAL + | FIXED_INTERVAL + | CALENDAR_INTERVAL ; positionFunction @@ -770,13 +773,6 @@ highlightArgName | HIGHLIGHT_PRE_TAGS ; -bucketArgName - : FIELD - | INTERVAL - | FIXED_INTERVAL - | CALENDAR_INTERVAL - ; - relevanceFieldAndWeight : field = relevanceField | field = relevanceField weight = relevanceFieldWeight diff --git a/sql/src/main/antlr/OpenSearchSQLParser.g4 b/sql/src/main/antlr/OpenSearchSQLParser.g4 index 1fab2554c60..e372382805c 100644 --- a/sql/src/main/antlr/OpenSearchSQLParser.g4 +++ b/sql/src/main/antlr/OpenSearchSQLParser.g4 @@ -431,11 +431,14 @@ highlightFunction ; bucketFunction - : bucketFunctionName LR_BRACKET bucketArg (COMMA bucketArg)* RR_BRACKET + : bucketFunctionName LR_BRACKET FIELD EQUAL_SYMBOL field = bucketArgValue COMMA + intervalArgName EQUAL_SYMBOL interval = constant RR_BRACKET ; -bucketArg - : bucketArgName EQUAL_SYMBOL bucketArgValue +intervalArgName + : INTERVAL + | FIXED_INTERVAL + | CALENDAR_INTERVAL ; positionFunction @@ -803,13 +806,6 @@ highlightArgName | HIGHLIGHT_PRE_TAGS ; -bucketArgName - : FIELD - | INTERVAL - | FIXED_INTERVAL - | CALENDAR_INTERVAL - ; - relevanceFieldAndWeight : field = relevanceField | field = relevanceField weight = relevanceFieldWeight diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java index a431a8fc1a1..d371961d11a 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java @@ -20,7 +20,6 @@ import static org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.BetweenPredicateContext; import static org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.BinaryComparisonPredicateContext; import static org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.BooleanContext; -import static org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.BucketArgContext; import static org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.BucketFunctionCallContext; import static org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.CaseFuncAlternativeContext; import static org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.CaseFunctionCallContext; @@ -75,11 +74,9 @@ import java.util.Arrays; import java.util.Collections; import java.util.HashMap; -import java.util.LinkedHashMap; import java.util.List; import java.util.Locale; import java.util.Map; -import java.util.Objects; import java.util.Optional; import java.util.stream.Collectors; import org.antlr.v4.runtime.RuleContext; @@ -93,7 +90,6 @@ import org.opensearch.sql.common.antlr.AstBuildGuard; import org.opensearch.sql.common.antlr.SyntaxCheckException; import org.opensearch.sql.common.utils.StringUtils; -import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.expression.function.BuiltinFunctionName; import org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser; import org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser.AlternateMultiMatchQueryContext; @@ -109,10 +105,6 @@ /** Expression builder to parse text to expression in AST. */ public class AstExpressionBuilder extends OpenSearchSQLParserBaseVisitor { - /** Synonyms for the bucket width; exactly one is required. */ - private static final List INTERVAL_ARGS = - List.of("interval", "fixed_interval", "calendar_interval"); - private final AstBuildGuard guard; public AstExpressionBuilder() { @@ -175,47 +167,15 @@ public UnresolvedExpression visitScalarFunctionCall(ScalarFunctionCallContext ct } /** - * Lowers {@code histogram} and {@code date_histogram} to a {@link Span} over the bucketed field. - * Parameters the span cannot express are implemented by the legacy engine through the native - * date_histogram aggregation, so those calls are declined as a syntax check to let - * RestSQLQueryAction hand them back to it. + * Lowers {@code histogram} and {@code date_histogram} to a {@link Span}, the same node PPL's + * {@code span()} produces. Anything the grammar does not admit here is a syntax error, which + * RestSQLQueryAction hands to the legacy engine. */ @Override public UnresolvedExpression visitBucketFunctionCall(BucketFunctionCallContext ctx) { - String functionName = - ctx.bucketFunction().bucketFunctionName().getText().toLowerCase(Locale.ROOT); - Map args = new LinkedHashMap<>(); - for (BucketArgContext arg : ctx.bucketFunction().bucketArg()) { - String name = arg.bucketArgName().getText().toLowerCase(Locale.ROOT); - if (args.put(name, visit(arg.bucketArgValue())) != null) { - throw new SemanticCheckException( - String.format("Parameter '%s' can only be specified once.", name)); - } - } - - UnresolvedExpression field = args.remove("field"); - List intervals = - INTERVAL_ARGS.stream() - .map(args::remove) - .filter(Objects::nonNull) - .collect(Collectors.toList()); - - 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 (!(intervals.get(0) instanceof Literal interval)) { - throw new SemanticCheckException( - String.format("Parameter interval must be a literal for %s function.", functionName)); - } - - return AstDSL.spanFromSpanLengthLiteral(normalizeField(field), interval); + OpenSearchSQLParser.BucketFunctionContext bucket = ctx.bucketFunction(); + return AstDSL.spanFromSpanLengthLiteral( + normalizeField(visit(bucket.field)), (Literal) visit(bucket.interval)); } /** A string literal naming a column is coerced so downstream sees a column reference. */ diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java index 9cd8de109c9..1902283a458 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java @@ -56,7 +56,6 @@ import org.opensearch.sql.common.antlr.CaseInsensitiveCharStream; import org.opensearch.sql.common.antlr.SyntaxAnalysisErrorListener; import org.opensearch.sql.common.antlr.SyntaxCheckException; -import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.sql.antlr.parser.OpenSearchSQLLexer; import org.opensearch.sql.sql.antlr.parser.OpenSearchSQLParser; @@ -939,19 +938,18 @@ public void unsupportedBucketParameterDefersToLegacyEngine() { } } - /** A bad argument inside a shape we own must not fall back, so the caller sees this message. */ + /** A shape the grammar does not admit is a syntax error, which routes to the legacy engine. */ @Test - public void badBucketArgumentIsReportedRatherThanDeferred() { - assertThrows(SemanticCheckException.class, () -> buildExprAst("date_histogram(interval='1d')")); - assertThrows(SemanticCheckException.class, () -> buildExprAst("date_histogram(field=ts)")); - assertThrows( - SemanticCheckException.class, - () -> buildExprAst("date_histogram(field=ts, interval='1d', fixed_interval='2d')")); - assertThrows( - SemanticCheckException.class, () -> buildExprAst("date_histogram(field=ts, interval=ts)")); - assertThrows( - SemanticCheckException.class, - () -> buildExprAst("date_histogram(field=ts, interval='1d', interval='2d')")); + public void badBucketArgumentIsASyntaxError() { + for (String call : + List.of( + "date_histogram(interval='1d')", + "date_histogram(field=ts)", + "date_histogram(field=ts, interval='1d', fixed_interval='2d')", + "date_histogram(field=ts, interval=ts)", + "date_histogram(field=ts, interval='1d', interval='2d')")) { + assertThrows(SyntaxCheckException.class, () -> buildExprAst(call)); + } } /** A name the grammar does not list is a parse error, which routes to the legacy engine. */ From 7f6f3a369d0ff82a619770003369859cd7ab8dc1 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Thu, 20 Aug 2026 14:33:53 -0700 Subject: [PATCH 17/18] Document the bucket functions where GROUP BY is documented `date_histogram` appeared once in the whole docs tree, in a dev note about pagination, and `aggregations.rst` described a group-by expression as an identifier, an ordinal or an expression. A bucket function is a fourth kind, so it goes in that list, next to the other three. The examples run under doctest, which already covers this file. They use the indices it loads rather than adding new ones, and the prose states the two things that are easy to get wrong from an Elasticsearch habit: the field comes first, and the interval parameter is one of three names. Signed-off-by: Jialiang Liang --- docs/user/dql/aggregations.rst | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/docs/user/dql/aggregations.rst b/docs/user/dql/aggregations.rst index 4b8ca57a32d..e828441b708 100644 --- a/docs/user/dql/aggregations.rst +++ b/docs/user/dql/aggregations.rst @@ -74,6 +74,40 @@ The group by expression could be expression:: +---------------------+----------+ +Bucket Function +--------------- + +The group by expression could be a bucket function, which splits a field into +fixed-width buckets. ``date_histogram`` takes a time interval, ``histogram`` a +numeric width, and the field is given first. The interval parameter is one of +``interval``, ``fixed_interval`` or ``calendar_interval``. A bucket has to be +projected in a subquery before it can be grouped on:: + + os> SELECT b, count(*) FROM (SELECT date_histogram(field=timestamp, interval='1w') AS b FROM nyc_taxi) sub GROUP BY b ORDER BY b; + fetched rows / total rows = 4/4 + +---------------------+----------+ + | b | count(*) | + |---------------------+----------| + | 2014-06-30 00:00:00 | 288 | + | 2014-07-07 00:00:00 | 336 | + | 2014-07-14 00:00:00 | 336 | + | 2014-07-21 00:00:00 | 13 | + +---------------------+----------+ + +The time units are millisecond (``ms``), second (``s``), minute (``m``), hour +(``h``), day (``d``), week (``w``), month (``M``), quarter (``q``) and year +(``y``). A numeric field is bucketed the same way, with the width as a number:: + + os> SELECT b, count(*) FROM (SELECT histogram(field=age, interval=10) AS b FROM accounts) sub GROUP BY b ORDER BY b; + fetched rows / total rows = 2/2 + +----+----------+ + | b | count(*) | + |----+----------| + | 20 | 1 | + | 30 | 3 | + +----+----------+ + + Aggregation =========== From 724b1c4c8c5435f28081bc8223d76cd13c859331 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Thu, 20 Aug 2026 14:36:32 -0700 Subject: [PATCH 18/18] Point at the bucket functions from the function list They are not in `functions.rst` because they are only valid as a grouping key, but that is where someone looks for a function by name. The introduction says where they live, in the form `expressions.rst` already uses to point at that same file from the other direction. Signed-off-by: Jialiang Liang --- docs/user/dql/functions.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/user/dql/functions.rst b/docs/user/dql/functions.rst index c8a61e417d3..54820a298b9 100644 --- a/docs/user/dql/functions.rst +++ b/docs/user/dql/functions.rst @@ -15,6 +15,8 @@ There is support for a wide variety of functions shared by SQL/PPL. We are inten Most of the specifications can be self explained just as a regular function with data type as argument. The only notation that needs elaboration is generic type ``T`` which binds to an actual type and can be used as return type. For example, ``ABS(NUMBER T) -> T`` means function ``ABS`` accepts an numerical argument of type ``T`` which could be any sub-type of ``NUMBER`` type and returns the actual type of ``T`` as return type. The actual type binds to generic type at runtime dynamically. +The bucket functions ``date_histogram`` and ``histogram`` are not listed here because they are only valid as a grouping key, please see also: `Aggregations `_ + Type Conversion ===============