From 65737864d5b805dad402df2cb59c5f914b6e646f Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Mon, 17 Aug 2026 11:22:35 -0700 Subject: [PATCH 1/6] 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 2/6] 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 3/6] 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 4/6] 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 5/6] 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 6/6] 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)); - } -}