From 013f71d65155d5e1c46e60a3ca8fc0be07860d85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Fri, 2 Oct 2026 17:28:57 +0200 Subject: [PATCH] fix(sql): a view's HAVING reads every aggregate it names; GREATEST/LEAST skip NULL; DATEDIFF per group and by unit One core function (Criteria.bucketMetrics / Having.metricNames) lists every aggregate a HAVING condition reads, for core's view rules, search and extensions. A view's HAVING may compare two aggregates or apply COALESCE, GREATEST, LEAST or SIGN over several; a bare ISNULL(MIN(a)) deploys again; a per-group calculation channel (BucketScriptTransformAggregation) lets a view store and filter SELECT arithmetic over aggregates. ISNULL/ISNOTNULL and the DATEDIFF family parse an aggregate operand as the aggregate; a SELECT alias inside a HAVING function is replaced by its aggregate at every depth; GREATEST/LEAST skip NULL arguments in HAVING. DATEDIFF / DATE_DIFF / TIMESTAMPDIFF run per group over aggregates, compute HOUR/MINUTE/SECOND on timestamps (DAY and above keep calendar dates), type literal operands and support QUARTER; over a window function with no GROUP BY they are refused by name. Docs follow the engine's date2 - date1 sign. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../sql/HavingFunctionEmissionSpec.scala | 32 +- .../elastic/sql/SQLQuerySpec.scala | 4 +- .../MaterializedViewHavingGatewaySpec.scala | 10 +- documentation/sql/dql_statements.md | 18 +- documentation/sql/functions_date_time.md | 28 +- documentation/sql/functions_math.md | 2 +- .../sql/HavingFunctionEmissionSpec.scala | 32 +- .../elastic/sql/SQLQuerySpec.scala | 4 +- .../elastic/sql/function/cond/package.scala | 23 +- .../elastic/sql/function/time/package.scala | 138 ++++- .../app/softnetwork/elastic/sql/package.scala | 27 +- .../elastic/sql/parser/Parser.scala | 46 +- .../sql/parser/function/cond/package.scala | 10 +- .../sql/parser/function/time/package.scala | 10 +- .../elastic/sql/query/GroupBy.scala | 65 ++- .../elastic/sql/query/Having.scala | 132 ++++- .../softnetwork/elastic/sql/query/Where.scala | 123 +++-- .../elastic/sql/query/package.scala | 514 ++++++++++-------- .../sql/transform/TransformAggregation.scala | 47 ++ .../sql/query/HavingAliasResolutionSpec.scala | 304 +++++++++++ .../query/HavingNullAwareSelectorSpec.scala | 24 +- .../HavingOverAggregateFunctionSpec.scala | 46 +- .../query/MaterializedViewHavingSpec.scala | 425 +++++++++++---- .../client/DateFunctionExecutionSpec.scala | 310 +++++++++++ .../client/GroupByCompletenessSpec.scala | 470 ++++++++++++++++ 25 files changed, 2346 insertions(+), 498 deletions(-) create mode 100644 sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingAliasResolutionSpec.scala diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala index 7dcc6d8b8..718bd1743 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala @@ -183,14 +183,14 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { ).mkString } - "GREATEST over an aggregate" should "emit a bucket_selector, null-guarded" in { + "GREATEST over an aggregate" should "emit a bucket_selector skipping a NULL argument" in { + // `GREATEST(NULL, 0)` is 0 -- the docs, and WHERE: with a literal argument it is never NULL. queryOf(group + "GREATEST(COUNT(*), 0) > 1") shouldBe Seq( """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"status":{""", terms, ""","aggs":{"c":{"value_count":{"field":"_index"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c"},""", - """"script":{"source":"(params.c == null ? false : """, - """(Math.max(params.c, 0) > 1))"}}}}}}}""" + """"script":{"source":"(params.c == null ? 0 : Math.max(params.c, 0)) > 1"}}}}}}}""" ).mkString } @@ -339,14 +339,14 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { "a conjunction of a bare and a wrapped aggregate" should "emit both conditions" in { queryOf(group + "COUNT(*) > 1 AND GREATEST(COUNT(*), 0) > 2") should include( """"script":{"source":"(params.c == null ? false : (params.c > 1)) && """ + - """(params.c == null ? false : (Math.max(params.c, 0) > 2))"}""" + """(params.c == null ? 0 : Math.max(params.c, 0)) > 2"}""" ) } "a disjunction" should "emit both conditions" in { // Dropping a disjunct makes the filter STRICTER than written: rows disappear silently. queryOf(group + "GREATEST(COUNT(*), 0) > 1 OR COUNT(*) > 5") should include( - """(params.c == null ? false : (Math.max(params.c, 0) > 1)) || """ + + """(params.c == null ? 0 : Math.max(params.c, 0)) > 1 || """ + """(params.c == null ? false : (params.c > 5))""" ) } @@ -392,8 +392,8 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"count_all":{"value_count":{"field":"_index"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"count_all":"count_all"},""", - """"script":{"source":"(params.count_all == null ? false : """, - """(Math.max(params.count_all, 0) > 1))"}}}}}}}""" + """"script":{"source":"(params.count_all == null ? 0 : """, + """Math.max(params.count_all, 0)) > 1"}}}}}}}""" ).mkString } @@ -408,8 +408,8 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { """"aggs":{"e.name":{"terms":{"field":"emails.name","size":65536,"min_doc_count":1},""", """"aggs":{"count_e_address":{"value_count":{"field":"emails.address"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"count_e_address":"count_e_address"},""", - """"script":{"source":"(params.count_e_address == null ? false : """, - """(Math.max(params.count_e_address, 0) > 1))"}}}}}}}}}""" + """"script":{"source":"(params.count_e_address == null ? 0 : """, + """Math.max(params.count_e_address, 0)) > 1"}}}}}}}}}""" ).mkString } @@ -423,14 +423,13 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"c":{"value_count":{"field":"_index"}},"max_x":{"max":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c","max_x":"max_x"},""", - """"script":{"source":"(params.c == null""", - """ || ((def) (params.max_x == null""", + """"script":{"source":"(params.c == null ? false : (params.c > (""", + """((def) (params.max_x == null""", """ || Double.isNaN(params.max_x)""", - """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == null ? false : """, - """(params.c > Math.max""", - """(((def) (params.max_x == null""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == null ? 0 : Math.max(""", + """((def) (params.max_x == null""", """ || Double.isNaN(params.max_x)""", - """ || Double.isInfinite(params.max_x) ? null : params.max_x)), 0)))"}}}}}}}""" + """ || Double.isInfinite(params.max_x) ? null : params.max_x)), 0))))"}}}}}}}""" ).mkString } @@ -444,8 +443,7 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { """"aggs":{"__whole_table_having__":{"filters":{"filters":{"_all":{"match_all":{}}}},""", """"aggs":{"c":{"value_count":{"field":"_index"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c"},""", - """"script":{"source":"(params.c == null ? false : """, - """(Math.max(params.c, 0) > 1))"}}}}}}}""" + """"script":{"source":"(params.c == null ? 0 : Math.max(params.c, 0)) > 1"}}}}}}}""" ).mkString } diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala index d90e767fd..785d3234d 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala @@ -1655,7 +1655,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "diff": { | "script": { | "lang": "painless", - | "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value.toInstant().atZone(ZoneId.of('Z')).toLocalDate()); def param2 = (doc['updatedAt'].size() == 0 ? null : doc['updatedAt'].value.toInstant().atZone(ZoneId.of('Z')).toLocalDate()); (param1 == null || param2 == null) ? null : Long.valueOf(ChronoUnit.DAYS.between(param1, param2))" + | "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value.toInstant().atZone(ZoneId.of('Z'))); def param2 = (doc['updatedAt'].size() == 0 ? null : doc['updatedAt'].value.toInstant().atZone(ZoneId.of('Z'))); (param1 == null || param2 == null) ? null : Long.valueOf(ChronoUnit.DAYS.between(param1.toLocalDate(), param2.toLocalDate()))" | } | } | }, @@ -1712,7 +1712,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "max": { | "script": { | "lang": "painless", - | "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value.toLocalDate()); def param2 = (doc['updatedAt'].size() == 0 ? null : doc['updatedAt'].value.toInstant().atZone(ZoneId.of('Z')).toLocalDate()); def param3 = (param1 == null) ? null : ZonedDateTime.parse(param1, new DateTimeFormatterBuilder().appendPattern(\"yyyy-MM-dd HH:mm:ss\").appendFraction(ChronoField.NANO_OF_SECOND, 0, 9, true).toFormatter().withZone(ZoneId.of('Z'))); def param4 = (param3 != null ? param3.toLocalDate() : null); (param1 == null || param2 == null) ? null : Long.valueOf(ChronoUnit.DAYS.between(param4, param2))" + | "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value); def param2 = (doc['updatedAt'].size() == 0 ? null : doc['updatedAt'].value.toInstant().atZone(ZoneId.of('Z'))); def param3 = (param1 == null) ? null : ZonedDateTime.parse(param1, new DateTimeFormatterBuilder().appendPattern(\"yyyy-MM-dd HH:mm:ss\").appendFraction(ChronoField.NANO_OF_SECOND, 0, 9, true).toFormatter().withZone(ZoneId.of('Z'))); (param1 == null || param2 == null) ? null : Long.valueOf(ChronoUnit.DAYS.between(param3.toLocalDate(), param2.toLocalDate()))" | } | } | } diff --git a/core/src/test/scala/app/softnetwork/elastic/client/MaterializedViewHavingGatewaySpec.scala b/core/src/test/scala/app/softnetwork/elastic/client/MaterializedViewHavingGatewaySpec.scala index 2a3e47d2e..32ee2100b 100644 --- a/core/src/test/scala/app/softnetwork/elastic/client/MaterializedViewHavingGatewaySpec.scala +++ b/core/src/test/scala/app/softnetwork/elastic/client/MaterializedViewHavingGatewaySpec.scala @@ -75,10 +75,10 @@ class MaterializedViewHavingGatewaySpec "CREATE MATERIALIZED VIEW mv AS SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING MAX(amount) > 1", "an aggregate no transform can compute" -> "CREATE MATERIALIZED VIEW mv AS SELECT city, STDDEV(amount) AS sd FROM customers GROUP BY city HAVING STDDEV(amount) > 1", - "an expression over aggregates" -> - "CREATE MATERIALIZED VIEW mv AS SELECT city, MAX(amount) - MIN(amount) AS d FROM customers GROUP BY city HAVING d > 3", - "two aggregates compared" -> - "CREATE MATERIALIZED VIEW mv AS SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city HAVING MAX(amount) > MIN(amount)" + "an expression over an aggregate no transform can compute" -> + "CREATE MATERIALIZED VIEW mv AS SELECT city, STDDEV(amount) - MIN(amount) AS d FROM customers GROUP BY city HAVING d > 3", + "a child predicate beside a metric" -> + "CREATE MATERIALIZED VIEW mv AS SELECT city, SUM(amount) AS s FROM customers GROUP BY city HAVING s > 5 AND child(c.x = 1)" ) "an unmaterializable HAVING" should "answer 400, naming the clause" in { @@ -129,6 +129,8 @@ class MaterializedViewHavingGatewaySpec Seq( "CREATE MATERIALIZED VIEW mv AS SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING COUNT(*) > 1", "CREATE MATERIALIZED VIEW mv AS SELECT city, MAX(amount) AS mx FROM customers GROUP BY city HAVING MAX(amount) > 1", + "CREATE MATERIALIZED VIEW mv AS SELECT city, MAX(amount) - MIN(amount) AS d FROM customers GROUP BY city HAVING d > 3", + "CREATE MATERIALIZED VIEW mv AS SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city HAVING MAX(amount) > MIN(amount)", "CREATE OR REPLACE MATERIALIZED VIEW mv AS SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING COUNT(*) > 1", "CREATE MATERIALIZED VIEW mv AS SELECT city, COUNT(*) AS c FROM customers GROUP BY city" ).foreach { sql => diff --git a/documentation/sql/dql_statements.md b/documentation/sql/dql_statements.md index 08f8aab76..85fe3cd07 100644 --- a/documentation/sql/dql_statements.md +++ b/documentation/sql/dql_statements.md @@ -1291,14 +1291,14 @@ FROM dql_users; ##### **Arithmetic:** -| Function | Description | -|-------------------------------------|----------------------------------| -| `DATE_ADD(date, INTERVAL n unit)` | Add interval | -| `DATE_SUB(date, INTERVAL n unit)` | Subtract interval | -| `DATETIME_ADD(ts, INTERVAL n unit)` | Add interval to timestamp | -| `DATETIME_SUB(ts, INTERVAL n unit)` | Subtract interval from timestamp | -| `DATE_DIFF(date1, date2, unit)` | Difference in units | -| `DATE_TRUNC(date, unit)` | Truncate to unit | +| Function | Description | +|-------------------------------------|-----------------------------------------------------------------------------------------------| +| `DATE_ADD(date, INTERVAL n unit)` | Add interval | +| `DATE_SUB(date, INTERVAL n unit)` | Subtract interval | +| `DATETIME_ADD(ts, INTERVAL n unit)` | Add interval to timestamp | +| `DATETIME_SUB(ts, INTERVAL n unit)` | Subtract interval from timestamp | +| `DATE_DIFF(date1, date2, unit)` | Difference in units: elapsed `HOUR` / `MINUTE` / `SECOND`, calendar dates from `DAY` up (UTC) | +| `DATE_TRUNC(date, unit)` | Truncate to unit | ##### **Formatting & parsing:** @@ -1327,7 +1327,7 @@ SELECT id, MONTH(CURRENT_DATE) AS current_month, DAY(CURRENT_DATE) AS current_day, YEAR(birthdate) AS year_b, - DATE_DIFF(CURRENT_DATE, birthdate, YEAR) AS diff_years, + DATE_DIFF(birthdate, CURRENT_DATE, YEAR) AS diff_years, DATE_TRUNC(birthdate, MONTH) AS trunc_month, DATETIME_FORMAT(birthdate, '%Y-%m-%d') AS birth_str FROM dql_users; diff --git a/documentation/sql/functions_date_time.md b/documentation/sql/functions_date_time.md index 50741c79a..b8e312834 100644 --- a/documentation/sql/functions_date_time.md +++ b/documentation/sql/functions_date_time.md @@ -318,7 +318,8 @@ SELECT DATETIME_SUB('2025-01-10T12:00:00Z'::TIMESTAMP, INTERVAL 1 MONTH) AS last #### DATEDIFF / DATE_DIFF -Difference between 2 dates (date1 - date2) in the specified time unit. +Difference between 2 dates (date2 - date1) in the specified time unit: `date1` is the start and `date2` the end. +MySQL's two-argument `DATEDIFF(a, b)` gives `a - b`. **Syntax:** ```sql @@ -337,38 +338,47 @@ DATE_DIFF(date1, date2, unit) **Output:** - `BIGINT` +**Units:** +- `HOUR`, `MINUTE`, `SECOND` count the elapsed whole units between the two instants, in UTC, truncated toward zero. A `DATE` operand counts from the start of its day (00:00 UTC). +- `DAY`, `WEEK`, `MONTH`, `QUARTER`, `YEAR` compare the two calendar dates, in UTC, whatever the time of day: `2025-01-10T23:30:00Z` and `2025-01-11T00:30:00Z` are 1 day apart, as MySQL's `DATEDIFF` counts them. A week is 7 whole days, a month counts once its day of month is reached, a quarter is 3 whole months and a year 12; every count is truncated toward zero. + +**Literals:** +- A string literal is read as the temporal it spells: `'2025-01-10'` (or `'2025/01/10'`) is a `DATE`; a literal with a time of day (`'2025-01-10 14:00:00'`, `'2025-01-10T14:00:00Z'`) is a `TIMESTAMP`, in UTC unless it names a zone. +- This holds in every clause, for each row and for each group: `DATEDIFF(MAX(created_at), '2025-01-10 08:00:00', HOUR)`. +- A `NULL` operand, or an aggregate over a group that has no value, gives `NULL`. + **Examples:** ```sql --- Difference in days (default) +-- Difference in days (default), MySQL's two-argument form: date1 - date2 SELECT DATEDIFF('2025-01-10'::DATE, '2025-01-01'::DATE) AS diff; -- Result: 9 -- Difference in days (explicit) -SELECT DATEDIFF('2025-01-10'::DATE, '2025-01-01'::DATE, DAY) AS diff_days; +SELECT DATEDIFF('2025-01-01'::DATE, '2025-01-10'::DATE, DAY) AS diff_days; -- Result: 9 -- Difference in weeks -SELECT DATE_DIFF('2025-01-31'::DATE, '2025-01-01'::DATE, WEEK) AS diff_weeks; +SELECT DATE_DIFF('2025-01-01'::DATE, '2025-01-31'::DATE, WEEK) AS diff_weeks; -- Result: 4 -- Difference in months -SELECT DATEDIFF('2025-06-01'::DATE, '2025-01-01'::DATE, MONTH) AS diff_months; +SELECT DATEDIFF('2025-01-01'::DATE, '2025-06-01'::DATE, MONTH) AS diff_months; -- Result: 5 -- Difference in years -SELECT DATEDIFF('2027-01-01'::DATE, '2025-01-01'::DATE, YEAR) AS diff_years; +SELECT DATEDIFF('2025-01-01'::DATE, '2027-01-01'::DATE, YEAR) AS diff_years; -- Result: 2 -- Difference in hours (with timestamps) -SELECT DATEDIFF('2025-01-10T14:00:00Z'::TIMESTAMP, '2025-01-10T12:00:00Z'::TIMESTAMP, HOUR) AS diff_hours; +SELECT DATEDIFF('2025-01-10T12:00:00Z'::TIMESTAMP, '2025-01-10T14:00:00Z'::TIMESTAMP, HOUR) AS diff_hours; -- Result: 2 -- Difference in minutes -SELECT DATEDIFF('2025-01-10T12:30:00Z'::TIMESTAMP, '2025-01-10T12:00:00Z'::TIMESTAMP, MINUTE) AS diff_minutes; +SELECT DATEDIFF('2025-01-10T12:00:00Z'::TIMESTAMP, '2025-01-10T12:30:00Z'::TIMESTAMP, MINUTE) AS diff_minutes; -- Result: 30 -- Difference in seconds -SELECT DATEDIFF('2025-01-10T12:00:45Z'::TIMESTAMP, '2025-01-10T12:00:00Z'::TIMESTAMP, SECOND) AS diff_seconds; +SELECT DATEDIFF('2025-01-10T12:00:00Z'::TIMESTAMP, '2025-01-10T12:00:45Z'::TIMESTAMP, SECOND) AS diff_seconds; -- Result: 45 ``` diff --git a/documentation/sql/functions_math.md b/documentation/sql/functions_math.md index 0181f9eb4..2964cc482 100644 --- a/documentation/sql/functions_math.md +++ b/documentation/sql/functions_math.md @@ -206,7 +206,7 @@ SELECT FLOOR(123.999) AS f; SELECT user_id, name, - FLOOR(DATEDIFF(CURRENT_DATE, birth_date, DAY) / 365.25) AS age + FLOOR(DATEDIFF(birth_date, CURRENT_DATE, DAY) / 365.25) AS age FROM users; -- Bucket values diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala index 006af302f..d50dcf8fc 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala @@ -189,14 +189,14 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { ).mkString } - "GREATEST over an aggregate" should "emit a bucket_selector, null-guarded" in { + "GREATEST over an aggregate" should "emit a bucket_selector skipping a NULL argument" in { + // `GREATEST(NULL, 0)` is 0 -- the docs, and WHERE: with a literal argument it is never NULL. queryOf(group + "GREATEST(COUNT(*), 0) > 1") shouldBe Seq( """{"query":{"match_all":{}},"size":0,"_source":false,"aggs":{"status":{""", terms, ""","aggs":{"c":{"value_count":{"field":"_index"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c"},""", - """"script":{"source":"(params.c == null ? false : """, - """(Math.max(params.c, 0) > 1))"}}}}}}}""" + """"script":{"source":"(params.c == null ? 0 : Math.max(params.c, 0)) > 1"}}}}}}}""" ).mkString } @@ -353,14 +353,14 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { "a conjunction of a bare and a wrapped aggregate" should "emit both conditions" in { queryOf(group + "COUNT(*) > 1 AND GREATEST(COUNT(*), 0) > 2") should include( """"script":{"source":"(params.c == null ? false : (params.c > 1)) && """ + - """(params.c == null ? false : (Math.max(params.c, 0) > 2))"}""" + """(params.c == null ? 0 : Math.max(params.c, 0)) > 2"}""" ) } "a disjunction" should "emit both conditions" in { // Dropping a disjunct makes the filter STRICTER than written: rows disappear silently. queryOf(group + "GREATEST(COUNT(*), 0) > 1 OR COUNT(*) > 5") should include( - """(params.c == null ? false : (Math.max(params.c, 0) > 1)) || """ + + """(params.c == null ? 0 : Math.max(params.c, 0)) > 1 || """ + """(params.c == null ? false : (params.c > 5))""" ) } @@ -407,8 +407,8 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"count_all":{"value_count":{"field":"_index"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"count_all":"count_all"},""", - """"script":{"source":"(params.count_all == null ? false : """, - """(Math.max(params.count_all, 0) > 1))"}}}}}}}""" + """"script":{"source":"(params.count_all == null ? 0 : """, + """Math.max(params.count_all, 0)) > 1"}}}}}}}""" ).mkString } @@ -423,8 +423,8 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { """"aggs":{"e.name":{"terms":{"field":"emails.name","size":65536,"min_doc_count":1},""", """"aggs":{"count_e_address":{"value_count":{"field":"emails.address"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"count_e_address":"count_e_address"},""", - """"script":{"source":"(params.count_e_address == null ? false : """, - """(Math.max(params.count_e_address, 0) > 1))"}}}}}}}}}""" + """"script":{"source":"(params.count_e_address == null ? 0 : """, + """Math.max(params.count_e_address, 0)) > 1"}}}}}}}}}""" ).mkString } @@ -438,14 +438,13 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"c":{"value_count":{"field":"_index"}},"max_x":{"max":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c","max_x":"max_x"},""", - """"script":{"source":"(params.c == null""", - """ || ((def) (params.max_x == null""", + """"script":{"source":"(params.c == null ? false : (params.c > (""", + """((def) (params.max_x == null""", """ || Double.isNaN(params.max_x)""", - """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == null ? false : """, - """(params.c > Math.max""", - """(((def) (params.max_x == null""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == null ? 0 : Math.max(""", + """((def) (params.max_x == null""", """ || Double.isNaN(params.max_x)""", - """ || Double.isInfinite(params.max_x) ? null : params.max_x)), 0)))"}}}}}}}""" + """ || Double.isInfinite(params.max_x) ? null : params.max_x)), 0))))"}}}}}}}""" ).mkString } @@ -459,8 +458,7 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { """"aggs":{"__whole_table_having__":{"filters":{"filters":{"_all":{"match_all":{}}}},""", """"aggs":{"c":{"value_count":{"field":"_index"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"c":"c"},""", - """"script":{"source":"(params.c == null ? false : """, - """(Math.max(params.c, 0) > 1))"}}}}}}}""" + """"script":{"source":"(params.c == null ? 0 : Math.max(params.c, 0)) > 1"}}}}}}}""" ).mkString } diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala index 30b47208d..ab09f06a3 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala @@ -1655,7 +1655,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "diff": { | "script": { | "lang": "painless", - | "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value.toInstant().atZone(ZoneId.of('Z')).toLocalDate()); def param2 = (doc['updatedAt'].size() == 0 ? null : doc['updatedAt'].value.toInstant().atZone(ZoneId.of('Z')).toLocalDate()); (param1 == null || param2 == null) ? null : Long.valueOf(ChronoUnit.DAYS.between(param1, param2))" + | "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value.toInstant().atZone(ZoneId.of('Z'))); def param2 = (doc['updatedAt'].size() == 0 ? null : doc['updatedAt'].value.toInstant().atZone(ZoneId.of('Z'))); (param1 == null || param2 == null) ? null : Long.valueOf(ChronoUnit.DAYS.between(param1.toLocalDate(), param2.toLocalDate()))" | } | } | }, @@ -1712,7 +1712,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "max": { | "script": { | "lang": "painless", - | "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value.toLocalDate()); def param2 = (doc['updatedAt'].size() == 0 ? null : doc['updatedAt'].value.toInstant().atZone(ZoneId.of('Z')).toLocalDate()); def param3 = (param1 == null) ? null : ZonedDateTime.parse(param1, new DateTimeFormatterBuilder().appendPattern(\"yyyy-MM-dd HH:mm:ss\").appendFraction(ChronoField.NANO_OF_SECOND, 0, 9, true).toFormatter().withZone(ZoneId.of('Z'))); def param4 = (param3 != null ? param3.toLocalDate() : null); (param1 == null || param2 == null) ? null : Long.valueOf(ChronoUnit.DAYS.between(param4, param2))" + | "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value); def param2 = (doc['updatedAt'].size() == 0 ? null : doc['updatedAt'].value.toInstant().atZone(ZoneId.of('Z'))); def param3 = (param1 == null) ? null : ZonedDateTime.parse(param1, new DateTimeFormatterBuilder().appendPattern(\"yyyy-MM-dd HH:mm:ss\").appendFraction(ChronoField.NANO_OF_SECOND, 0, 9, true).toFormatter().withZone(ZoneId.of('Z'))); (param1 == null || param2 == null) ? null : Long.valueOf(ChronoUnit.DAYS.between(param3.toLocalDate(), param2.toLocalDate()))" | } | } | } diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/function/cond/package.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/function/cond/package.scala index 1aa736040..4f3f8f1dd 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/function/cond/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/function/cond/package.scala @@ -1028,6 +1028,27 @@ package object cond { override def nullable: Boolean = values.forall(_.nullable) + /** Can this argument's rendering be NULL here? + * + * πŸ”΄ In a group filter (`HAVING`), an argument that reads a metric is NULL whenever the group + * has none of its values -- the filter reads every aggregate as SELECT returns it + * (`MetricSelectorScript.nullAwareSelectorScript`) -- so it is skipped like any other NULL + * argument, the rule the docs state and WHERE applies to a missing column. `nullable` alone + * answers `false` for an aggregate, and the reducer emitted `Math.max(params.m, k)`: only the + * comparison's guard kept it from throwing, and that guard dropped the group as soon as ONE + * argument was NULL. MEASURED on Elasticsearch 8.18.3: 5 of 6 shapes kept the wrong groups. + * + * Asked of the group filter alone ([[query.MetricSelectorScript.rendersGroupFilter]]): the + * SELECT list's `bucket_script` renders this same function context-free, and its gap policy + * never hands it a NULL metric, so its script does not move. + */ + private def nullableArgument(argument: PainlessScript, context: Option[PainlessContext]) = + argument.nullable || (context.isEmpty && query.MetricSelectorScript.rendersGroupFilter && + (argument match { + case id: Identifier => id.bucketMetrics.nonEmpty + case _ => false + })) + override def toPainlessCall( callArgs: List[String], context: Option[PainlessContext] @@ -1061,7 +1082,7 @@ package object cond { case Nil => throw new IllegalArgumentException(s"$operator requires at least one argument") case x :: Nil => x - case _ => fold(callArgs.zip(values.map(_.nullable)))._1 + case _ => fold(callArgs.zip(values.map(nullableArgument(_, context))))._1 } } } diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/function/time/package.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/function/time/package.scala index 2b0bdf9fd..cfc9e5687 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/function/time/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/function/time/package.scala @@ -545,6 +545,27 @@ package object time { case object DateDiff extends Expr("DATE_DIFF") with TokenRegex with PainlessScript { override def painless(context: Option[PainlessContext]): String = ".between" override lazy val words: List[String] = List(sql, "TIMESTAMPDIFF") + + /** A calendar date alone, in the two separators the DATE parse accepts (`SQLTypeUtils.coerce`). + */ + private val CalendarDate = """\d{4}[-/]\d{2}[-/]\d{2}""".r + + /** The temporal a STRING-LITERAL operand spells: a DATE when it is a calendar date alone + * (`'2024-01-01'`), a TIMESTAMP when it carries anything more -- a time of day, a zone + * (`'2024-01-01 10:00:00'`, `'2024-01-01T10:00:00Z'`). `None` for any other operand. + */ + private[time] def literalType(operand: PainlessScript): Option[SQLType] = operand match { + case id: Identifier if id.name.isEmpty => + id.functions match { + case List(s: StringValue) => + Some( + if (CalendarDate.pattern.matcher(s.value).matches()) SQLTypes.Date + else SQLTypes.Timestamp + ) + case _ => None + } + case _ => None + } } /** MySQL's `DATEDIFF`, which is a DIFFERENT function from the one above and needs its own token @@ -625,14 +646,127 @@ package object time { case DateDiffSpelling.DateFirst => s"$sql(${start.sql}, ${end.sql}, ${unit.sql})" } - override def in: SQLType = SQLTypes.Date + /** The type a COLUMN operand is read as, whatever the unit: the instant it denotes, in UTC + * (`FunctionN`'s argument path folds that read onto the column's parameter). + * [[toPainlessCall]] then brings every operand to [[comparedIn]]. + * + * πŸ”΄ Not the unit's own type. One column is ONE parameter per script, shared by every call + * that reads it, and the conversion folded onto it is the first caller's (the + * parameter-identity family, issue #370). A fold that depended on the unit made `CASE WHEN + * DATE_DIFF(d, ts, HOUR) > 1 THEN DATE_DIFF(d, ts, DAY) END` read both calls through the + * HOUR's `ZonedDateTime` and count ELAPSED days. Read losslessly once, each call narrows its + * own operands. + */ + override def in: SQLType = SQLTypes.Timestamp + /** The type the two operands are COMPARED in, and the UNIT decides it. + * + * - `HOUR`, `MINUTE` and `SECOND` count the ELAPSED whole units between two instants (UTC), + * truncated toward zero; a DATE operand is the start of its day. They used to be compared + * as a `LocalDate`, which has no time of day, so every such call failed with `Unsupported + * unit: Hours`. + * - `DAY` and every larger unit compare the two CALENDAR dates (UTC): 23:30 and 00:30 the + * next day are one day apart, as MySQL's `DATEDIFF` answers. That is what they computed. + */ + private def comparedIn: SQLType = unit match { + case TimeUnit.HOURS | TimeUnit.MINUTES | TimeUnit.SECONDS => SQLTypes.Timestamp + case _ => SQLTypes.Date + } + + /** A context-free rendering over an aggregate is the PER-GROUP calculation: the `bucket_script` + * of a SELECT item (`DATEDIFF(MAX(d), '2024-01-01') AS x`), which a HAVING over `x` reads by + * name and a materialized view's pivot computes too (`SingleSearch.transformBucketScripts`). + * Its operands are converted in [[toPainlessCall]], the rendering row level ends in too. + * + * An empty group needs no guard here: under the default gap policy Elasticsearch SKIPS the + * script when an operand metric has no value (6.8 to 9.0, `BucketScriptPipelineAggregator`), + * so the item has no value for that group and SELECT and HAVING read NULL. + */ + override def painless(context: Option[PainlessContext]): String = + context match { + case None if hasAggregation => toPainlessCall(args.map(_.painless(None)), context) + case _ => super[BinaryFunction].painless(context) + } + + /** One operand, brought to [[comparedIn]] through the coercion arms a CAST uses. + * + * - A string LITERAL is first read as the temporal it spells ([[DateDiff.literalType]]: UTC + * unless it names a zone). Row level used to hand Painless the bare string + * (`between("2024-01-01", param1)`, which it cannot call), and per group parsed it as a + * DATE whatever it held, so a time of day failed the search. + * - Per group, an aggregate is the metric Elasticsearch computed, which a `bucket_script` + * receives as a `java.lang.Double` holding EPOCH MILLIS (`(long)` is required: Painless + * refuses to cast a `def` double to `long` implicitly). Any other operand is converted + * from its own type. + * - At row level a column operand holds its UTC instant ([[in]]), so its calendar date is + * `toLocalDate()`. An operand with no column of its own (`'2025-01-10'::DATE`, + * `CURRENT_DATE`, `NOW()`) renders as it did, except a DATE under a sub-day unit, which is + * the start of its day: a `LocalDate` has no hours. + * + * πŸ”΄ An INGEST processor renders as it did: there a column is the raw document value, parsed + * at runtime by `SQLTypeUtils.processorTemporal`. + */ + private def operand( + arg: PainlessScript, + rendered: String, + context: Option[PainlessContext], + perGroup: Boolean + ): String = + DateDiff.literalType(arg) match { + case _ if context.exists(_.isProcessor) => rendered + case Some(literal) => + val value = + SQLTypeUtils.coerce(rendered, SQLTypes.Varchar, literal, nullable = false, None) + SQLTypeUtils.coerce(value, literal, comparedIn, nullable = false, None) + case None if perGroup => + arg match { + case metric: Identifier if metric.isAggregation => + val epochMillis = SQLTypeUtils + .coerce(rendered, SQLTypes.Double, SQLTypes.BigInt, nullable = false, None) + // The instant in UTC already (`Instant.ofEpochMilli(...).atZone(ZoneId.of('Z'))`), so + // DAY and above read its calendar date off it: ONE conversion per aggregate. The + // TIMESTAMP -> DATE arm would normalise it to UTC a second time + // (`.toInstant().atZone(ZoneId.of('Z'))`), as it must an operand of unknown zone. + val utc = SQLTypeUtils + .coerce(epochMillis, SQLTypes.BigInt, SQLTypes.Timestamp, nullable = false, None) + if (comparedIn == SQLTypes.Date) s"$utc.toLocalDate()" else utc + case other => + SQLTypeUtils.coerce(rendered, other.baseType, comparedIn, nullable = false, None) + } + case None if context.isDefined => + arg match { + case column: Identifier if column.name.trim.nonEmpty => + if (comparedIn == SQLTypes.Date) s"$rendered.toLocalDate()" else rendered + case value: Identifier + if comparedIn == SQLTypes.Timestamp && value.baseType == SQLTypes.Date => + SQLTypeUtils.coerce(rendered, SQLTypes.Date, comparedIn, nullable = false, None) + case _ => rendered + } + case None => rendered + } + + /** `ChronoUnit` has no `QUARTERS`, so a `QUARTER` call failed to compile in Elasticsearch. The + * ISO quarter-year unit counts the whole quarters between two calendar dates: the whole months + * between them, divided by 3. + */ + private def unitPainless(context: Option[PainlessContext]): String = unit match { + case TimeUnit.QUARTERS => "java.time.temporal.IsoFields.QUARTER_YEARS" + case other => other.painless(context) + } + + /** The function's ONE rendering: row level reaches it from `FunctionN.painless` with its + * operands rendered, per group from [[painless]]; [[operand]] converts each one. + */ override def toPainlessCall( callArgs: List[String], context: Option[PainlessContext] ): String = { + val perGroup = context.isEmpty && hasAggregation + val operands = args.zip(callArgs).map { case (arg, rendered) => + operand(arg, rendered, context, perGroup) + } val ret = - s"Long.valueOf(${unit.painless(context)}${DateDiff.painless(context)}(${callArgs.mkString(", ")}))" + s"Long.valueOf(${unitPainless(context)}${DateDiff.painless(context)}(${operands.mkString(", ")}))" context match { case Some(ctx) if ctx.isProcessor => // to fix bug in painless script processor context with elasticsearch v6 diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala index b3998bef1..bf3006f36 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/package.scala @@ -1618,10 +1618,10 @@ package object sql { } /** The aggregations a bucket pipeline reading THIS identifier addresses, in order -- the ONE - * derivation behind [[allMetricsPath]] (what `buckets_path` publishes), behind - * `Criteria.extractAggregationFields` (which aggregations are created) and behind the - * `bucket_selector` null guard. Three answers to one question would be three answers that - * drift (`project_self_join_alias_resolution`). + * derivation behind `Criteria.bucketMetrics` (what `buckets_path` publishes and a view's pivot + * must create), behind `Criteria.extractAggregationFields` (which aggregations are created) + * and behind the `bucket_selector` null guard. Three answers to one question would be three + * answers that drift (`project_self_join_alias_resolution`). * * Three arms, and the third is issue #389's: * 1. the identifier IS an aggregate -- one metric, itself; 2. it is the alias of a SELECT @@ -1634,9 +1634,6 @@ package object sql { if (metricName.isDefined || (hasAggregation && fieldAlias.isDefined)) Seq(this) else referencedAggregates - lazy val allMetricsPath: Map[String, String] = - bucketMetrics.map(id => id.metricPathKey -> id.metricPathKey).toMap - override def sql: String = { var parts: Seq[String] = name.split("\\.").toSeq tableAlias match { @@ -1731,7 +1728,7 @@ package object sql { /** The `buckets_path` key this identifier is addressed by: its derived [[metricName]] when it * is an aggregate, else the alias a SELECT `bucket_script` item published it under. ONE - * derivation, read by [[metricParam]] and by [[allMetricsPath]]. + * derivation, read by [[metricParam]] and by `Criteria.bucketMetrics`. */ lazy val metricPathKey: String = metricName.getOrElse(aliasOrName) @@ -2242,6 +2239,20 @@ package object sql { override def reportedLeafType: SQLType = col.map(_.dataType).getOrElse(super.reportedLeafType) def update(request: SingleSearch): Identifier = + query.Having.aliasedAggregate(this) match { + // A SELECT alias named in a HAVING, at any depth (`query.Having.resolveAggregateAliases`): + // the aliased SELECT item, under whatever this name applied to it (`CAST(c AS DOUBLE)` is + // the cast of the aggregate `c` stands for, exactly the tree its bare spelling parses to). + case Some(item) => + val target = item.update(request) + if (functions.isEmpty) target + else target.withFunctions(updateFunctions(request) ++ target.functions) + // An aggregate's operand is a document column, never a SELECT alias. + case None if isAggregation => query.Having.outsideAliasScope(resolved(request)) + case None => resolved(request) + } + + private def resolved(request: SingleSearch): Identifier = resolve(request) match { case resolved: GenericIdentifier => resolved.withNestedElementOf(request) case other => other diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/Parser.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/Parser.scala index b930eb6e0..abf2539ec 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/Parser.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/Parser.scala @@ -17,6 +17,7 @@ package app.softnetwork.elastic.sql.parser import app.softnetwork.elastic.sql._ +import app.softnetwork.elastic.sql.function.convert.CastOperator import app.softnetwork.elastic.sql.function.time.DateTimeFunction import app.softnetwork.elastic.sql.function._ import app.softnetwork.elastic.sql.operator._ @@ -2066,9 +2067,13 @@ trait Parser lazy val separator: PackratParser[Delimiter] = "," ^^ (_ => Separator) - lazy val valueExpr: PackratParser[PainlessScript] = { + /** The value expression, typed as what every one of its alternatives produces: an [[Identifier]]. + * ONE production with two static types -- [[valueExpr]] IS this parser, so the two share one + * packrat memo. + */ + lazy val valueIdentifier: PackratParser[Identifier] = { // the order is important here - identifierWithWindowFunction | + (guard(callHead) ~> identifierWithWindowFunction) | identifierWithTransformation | // transformations applied to an identifier identifierWithIntervalFunction | identifierWithFunction | // fonctions applied to an identifier @@ -2077,6 +2082,43 @@ trait Parser identifier } + /** The operand of `ISNULL` / `ISNOTNULL` and of the DATEDIFF family (`DATEDIFF`, `DATE_DIFF`, + * `TIMESTAMPDIFF`): a value expression, so an aggregate in it is the aggregate every other + * position builds. + * + * πŸ”΄ These five used to list their own alternatives WITHOUT `identifierWithWindowFunction`, so + * `MIN(a)` reached `identifierWithFunction` and came out as the bare `MIN` token instead of the + * `MinAgg` every other position builds: the transform conversion did not recognise it, and its + * rendered name (`MIN(a)`) no longer matched a qualified SELECT item's (`MIN(r.a)`). + * + * πŸ”΄ That reading of an aggregate stops at the call's `)`, while their own alternatives read a + * `::TYPE` cast or a `+ / - INTERVAL` after it (`ISNULL(MAX(a)::DOUBLE)`, `DATEDIFF(MAX(d) - + * INTERVAL 1 DAY, '2024-01-01')`). So where such a suffix follows, those alternatives read the + * operand, exactly as they always did, and the statement keeps the verdict and the message it + * had -- rather than failing with `end of input expected`, MEASURED on 13 shapes. Everywhere + * else they are the alternatives that follow. + */ + lazy val operandIdentifier: PackratParser[Identifier] = + (guard(callHead) ~> identifierWithWindowFunction <~ not(CastOperator.regex) <~ not( + intervalFunction + )) | + identifierWithTransformation | + identifierWithIntervalFunction | + identifierWithFunction | + identifier + + /** A name followed by its opening parenthesis, consuming nothing: what every alternative of + * `identifierWithWindowFunction` starts with (`MAX(`, `COUNT (`, `ROW_NUMBER(`, ...), so + * [[valueIdentifier]] and [[operandIdentifier]] try that production only where it can match. It + * changes no parse -- it declines only where every alternative would -- and it is ONE regex + * where the production tries one per window function. An operand that is a column or a literal + * (`d`, `CURRENT_DATE`, `'2024-01-01'`) paid them all once the DATEDIFF and ISNULL operands were + * read through the window functions: 2 to 4 microseconds per operand, MEASURED. + */ + private lazy val callHead: Parser[String] = """[A-Za-z_][A-Za-z0-9_]*\s*\(""".r + + lazy val valueExpr: PackratParser[PainlessScript] = valueIdentifier + implicit def functionAsIdentifier(mf: Function): Identifier = mf match { case id: Identifier => id case fid: FunctionWithIdentifier => diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/function/cond/package.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/function/cond/package.scala index ee82b7480..3e0498559 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/function/cond/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/function/cond/package.scala @@ -45,14 +45,16 @@ package object cond { trait CondParser { self: Parser with WhereParser => + // The operand is a value expression (`Parser.operandIdentifier`), so an aggregate in it is the + // aggregate every other position builds. lazy val is_null: PackratParser[ConditionalFunction[_]] = - "(?i)isnull".r ~ start ~ (identifierWithTransformation | identifierWithIntervalFunction | identifierWithFunction | identifier) ~ end ^^ { - case _ ~ _ ~ i ~ _ => IsNull(i) + "(?i)isnull".r ~ start ~ operandIdentifier ~ end ^^ { case _ ~ _ ~ i ~ _ => + IsNull(i) } lazy val is_notnull: PackratParser[ConditionalFunction[_]] = - "(?i)isnotnull".r ~ start ~ (identifierWithTransformation | identifierWithIntervalFunction | identifierWithFunction | identifier) ~ end ^^ { - case _ ~ _ ~ i ~ _ => IsNotNull(i) + "(?i)isnotnull".r ~ start ~ operandIdentifier ~ end ^^ { case _ ~ _ ~ i ~ _ => + IsNotNull(i) } lazy val coalesce: PackratParser[Coalesce] = diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/function/time/package.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/function/time/package.scala index bfdebdb54..909a0a62f 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/function/time/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/function/time/package.scala @@ -200,8 +200,12 @@ package object time { trait TemporalParser extends CurrentParser with TimeParser with DateParser with DateTimeParser { self: Parser => + // The operands of the three DATEDIFF productions are value expressions + // (`Parser.operandIdentifier`), as `ISNULL`'s is: an aggregate in them is the aggregate every + // other position builds, not the bare `MAX` token -- which a view's transform did not + // recognise, and whose rendered name (`MAX(d)`) did not match a qualified SELECT item's. lazy val date_diff: PackratParser[BinaryFunction[_, _, _]] = - DateDiff.regex ~ start ~ (identifierWithTransformation | identifierWithIntervalFunction | identifierWithFunction | identifier) ~ separator ~ (identifierWithTransformation | identifierWithIntervalFunction | identifierWithFunction | identifier) ~ (separator ~ time_unit).? ~ end ^^ { + DateDiff.regex ~ start ~ operandIdentifier ~ separator ~ operandIdentifier ~ (separator ~ time_unit).? ~ end ^^ { case _ ~ _ ~ d1 ~ _ ~ d2 ~ u ~ _ => DateDiff( d1, @@ -223,7 +227,7 @@ package object time { * on a plain column, so `DATEDIFF(a, b)` still falls through to the MySQL form. */ lazy val date_diff_transact_sql: PackratParser[BinaryFunction[_, _, _]] = - (DateDiff.regex | MySqlDateDiff.regex) ~ start ~> time_unit ~ separator ~ (identifierWithTransformation | identifierWithIntervalFunction | identifierWithFunction | identifier) ~ separator ~ (identifierWithTransformation | identifierWithIntervalFunction | identifierWithFunction | identifier) <~ end ^^ { + (DateDiff.regex | MySqlDateDiff.regex) ~ start ~> time_unit ~ separator ~ operandIdentifier ~ separator ~ operandIdentifier <~ end ^^ { case u ~ _ ~ d1 ~ _ ~ d2 => DateDiff(d1, d2, u, DateDiffSpelling.UnitFirst) } @@ -241,7 +245,7 @@ package object time { * rather than discovered. */ lazy val mysql_date_diff: PackratParser[BinaryFunction[_, _, _]] = - MySqlDateDiff.regex ~ start ~ (identifierWithTransformation | identifierWithIntervalFunction | identifierWithFunction | identifier) ~ separator ~ (identifierWithTransformation | identifierWithIntervalFunction | identifierWithFunction | identifier) ~ (separator ~ time_unit).? ~ end ^^ { + MySqlDateDiff.regex ~ start ~ operandIdentifier ~ separator ~ operandIdentifier ~ (separator ~ time_unit).? ~ end ^^ { case _ ~ _ ~ d1 ~ _ ~ d2 ~ u ~ _ => u match { case Some(_ ~ unit) => DateDiff(d1, d2, unit, DateDiffSpelling.DateFirst) diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/GroupBy.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/GroupBy.scala index 3544194fa..7c2e7641f 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/GroupBy.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/GroupBy.scala @@ -19,7 +19,7 @@ package app.softnetwork.elastic.sql.query import app.softnetwork.elastic.sql.`type`.SQLType import app.softnetwork.elastic.sql.function.aggregate._ import app.softnetwork.elastic.sql.operator._ -import scala.util.Try +import scala.util.{DynamicVariable, Try} import scala.util.matching.Regex import app.softnetwork.elastic.sql.{ quoteIdentifier, @@ -420,15 +420,15 @@ object MetricSelectorScript { * * Everything else is the existing rendering: a comparison's guard collapses the UNKNOWN to * `false` OUTSIDE its `NOT` (` && [!]()`), `ISNULL` / `ISNOTNULL` - * test the value, and a `COALESCE` is guarded on its result, never on its arguments - * (`Expression.bucketPipelineGuard`). The read is substituted on the RENDERING, after every rule - * has been decided on the unchanged one: the representability gate, the guards and their - * placement are exactly those of [[selectorScript]], and a criterion with no such aggregate is - * returned byte for byte. + * test the value, and a `COALESCE`, a `GREATEST` or a `LEAST` is guarded on its result, never on + * its arguments (`Expression.bucketPipelineGuard`). The read is substituted on the RENDERING, + * after every rule has been decided on the unchanged one: the representability gate, the guards + * and their placement are exactly those of [[selectorScript]], and a criterion with no such + * aggregate is returned byte for byte. */ def nullAwareSelectorScript(expr: Criteria): Option[String] = selectorScript(expr).map { script => - val nullable = bucketMetricsOf(expr).filter(nullOverEmptyInput).map(_.metricPathKey).toSet + val nullable = expr.bucketMetrics.filter(nullOverEmptyInput).map(_.metricPathKey).toSet if (nullable.isEmpty) script else readAsSelectReturns(script, nullable) } @@ -445,22 +445,6 @@ object MetricSelectorScript { s"((def) ($param == null || Double.isNaN($param) || Double.isInfinite($param) ? null : $param))" } - /** Every metric the selector reads, in statement order, deduplicated by `metricPathKey` -- the - * leaves [[selector]] renders as a filter, through the same `Expression.bucketMetrics` their - * renderings guard. - */ - private def bucketMetricsOf(expr: Criteria): Seq[Identifier] = { - def walk(c: Criteria): Seq[Identifier] = c match { - case Predicate(left, _, right, _, _) => walk(left) ++ walk(right) - case relation: ElasticRelation => walk(relation.criteria) - case e: Expression => e.bucketMetrics - case _ => Nil - } - walk(expr).foldLeft(Seq.empty[Identifier]) { (acc, id) => - if (acc.exists(_.metricPathKey == id.metricPathKey)) acc else acc :+ id - } - } - /** Does SELECT answer NULL for this metric over a group with none of its values? F1's list: the * rule of `ClientAggregation.nullOverEmptyInput`, restated over the parsed aggregate because * this module cannot see the client one -- the core suite asserts that the two agree for every @@ -506,6 +490,20 @@ object MetricSelectorScript { /** One `params.` read, the name taken whole. */ private val MetricRead: Regex = """(?` but `__now__`, the opaque script parameter a temporal literal is rendered + * against (see the bridge's `metricSelectorForBucket`). + */ + private[query] def metricsRead(script: String): Set[String] = + paramsRead(script).toSet - "__now__" + + /** EVERY parameter a bucket-pipeline `script` reads (`params.`, string literals skipped), + * in the order it first reads them: the metrics its `buckets_path` binds AND the script + * parameters its caller binds, such as `__now__` (`CURRENT_DATE`, `NOW()`, ...). + */ + private[query] def paramsRead(script: String): Seq[String] = + MetricRead.findAllMatchIn(blankStringLiterals(script)).map(_.group(1)).toList.distinct + /** The bucket-pipeline script of a `HAVING` criterion, or `"1 == 1"` when there is nothing to * filter at this level: [[nullAwareSelectorScript]] with its placeholder, so it reads every * aggregate exactly as the bridge's `bucket_selector` and `Having.script` do (#292). @@ -678,12 +676,31 @@ object MetricSelectorScript { * never read as code. */ private[query] def representable(e: Expression): Either[String, String] = - Try(e.functionBucketPipelinePainless).toOption match { + Try(groupFilter.withValue(true)(e.functionBucketPipelinePainless)).toOption match { case None => Left("it cannot be rendered without a document, and a bucket pipeline has none") case Some(rendering) => disqualifyRendering(rendering).toLeft(rendering) } + /** `true` while [[representable]] renders a `HAVING` leaf -- the ONE rendering every group filter + * is built from (both bridges' `bucket_selector`, `Having.script`, the view rules) and the + * verdict is taken on -- and nothing else. + * + * Read by `GREATEST` / `LEAST` ([[rendersGroupFilter]]), which skip a NULL argument and must + * know that a metric argument can be one HERE: a group filter reads every aggregate as SELECT + * returns it, NULL over a group with none of its values ([[nullAwareRead]]). The SELECT list's + * `bucket_script` renders the same function context-free, and there the gap policy skips such a + * group before the script runs, so its script does not move. + * + * A thread-scoped carrier, as `Having.aliasScope`, for the same reason: the function sits at any + * depth of an operand and is reached only through the rendering of every function of the + * dialect, which takes nothing but its context. Rendering is synchronous. + */ + private[this] val groupFilter: DynamicVariable[Boolean] = new DynamicVariable[Boolean](false) + + /** Is a group filter being rendered ([[groupFilter]])? */ + private[sql] def rendersGroupFilter: Boolean = groupFilter.value + /** Rules 2-6 of [[representable]], asked of a RENDERING rather than of an expression. * * Exposed as its own function because it is a pure function of a string and is therefore the diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala index a04956221..f688c7aa6 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Having.scala @@ -17,6 +17,11 @@ package app.softnetwork.elastic.sql.query import app.softnetwork.elastic.sql.{Expr, Identifier, TokenRegex, Updateable} +import app.softnetwork.elastic.sql.function.{FunctionN, FunctionUtils} +import app.softnetwork.elastic.sql.function.cond.Case +import app.softnetwork.elastic.sql.function.convert.Conversion + +import scala.util.DynamicVariable case object Having extends Expr("HAVING") with TokenRegex { @@ -24,10 +29,10 @@ case object Having extends Expr("HAVING") with TokenRegex { * aggregates (`MAX(x) - MIN(x) AS d`), the latter being a `bucket_script` that a * `bucket_selector` may read as a sibling pipeline aggregation by name. * - * ONE derivation, read by [[resolveAggregateAliases]] (which SUBSTITUTES a bare reference) and - * by `SingleSearch.validate()` (which REFUSES a reference substitution cannot reach, issue #389: - * `HAVING NULLIF(c, 0) > 1` hides the alias inside a function ARGUMENT, where the substitution - * below -- deliberately scoped to an operand -- does not go). + * ONE derivation, read by [[resolveAggregateAliases]] (which SUBSTITUTES a reference, at every + * depth of a value expression) and by `SingleSearch.validate()` (which REFUSES, issue #389, a + * function of an alias the substitution leaves in place: inside a relation predicate or a MATCH, + * which it does not touch). */ private[query] def aggregateAliases(request: SingleSearch): Map[String, Identifier] = request.select.fields.collect { @@ -35,6 +40,36 @@ case object Having extends Expr("HAVING") with TokenRegex { f.fieldAlias.get.alias -> f.identifier }.toMap + /** The SELECT aggregates a bare name denotes while [[resolveAggregateAliases]] runs, and nothing + * outside it: read by `GenericIdentifier.update` through [[aliasedAggregate]]. + * + * πŸ”΄ Why a thread-scoped carrier, not a parameter -- the reason `SubqueryScope` gives for its + * own. An alias hides at ANY depth of a value expression: an operand, a function argument + * (`COALESCE(max_a, min_b)`), an arithmetic operand, a `CASE` branch. The only walk that reaches + * every one of those AND rebuilds the node it found is `update(request)`, implemented by every + * function of the dialect; a second walk with its own rebuild for each of them would be a second + * derivation that drifts. `update` takes the statement and nothing else, so the alias map rides + * beside it, set around the substitution below and restored on exit. Resolution is synchronous, + * so the value is read on the thread that set it. + */ + private[this] val aliasScope: DynamicVariable[Map[String, Identifier]] = + new DynamicVariable[Map[String, Identifier]](Map.empty) + + /** The SELECT item a bare HAVING name stands for, while [[resolveAggregateAliases]] runs. + * + * Never for an identifier that IS an aggregate: its name is the column the aggregate reads (`a` + * in `MAX(a)`), not a reference to a SELECT item -- and `MAX(a) AS a` would otherwise substitute + * the aggregate into its own operand forever. Never for a nested column either, as before. + */ + private[sql] def aliasedAggregate(id: Identifier): Option[Identifier] = { + val aliases = aliasScope.value + if (aliases.isEmpty || id.nested || id.isAggregation) None else aliases.get(id.name) + } + + /** `body` with no alias substitution: what an aggregate's own operands are resolved under. */ + private[sql] def outsideAliasScope[T](body: => T): T = + if (aliasScope.value.isEmpty) body else aliasScope.withValue(Map.empty)(body) + /** `HAVING cnt > 1` where `cnt` aliases a SELECT aggregate (`COUNT(name) AS cnt`). The bare * identifier carries no aggregate function of its own, so the selector rendering saw no metric * in the condition and the whole HAVING degenerated to `1 == 1`: every group came back β€” the @@ -43,7 +78,14 @@ case object Having extends Expr("HAVING") with TokenRegex { * other aggregate: the alias comes back through `fieldAliases`, the selector reads `params.cnt`, * and no extra aggregation is created since the item is already a SELECT aggregate. Scoped to * HAVING on purpose: ORDER BY resolves an alias by name already, and WHERE must keep reading a - * bare name as a field. Relation predicates (`NESTED(...)`) are left untouched. + * bare name as a field. Relation predicates (`NESTED(...)`) and `MATCH` are left untouched. + * + * πŸ”΄ At EVERY depth of the leaf's operands, not only the operand itself. These become the very + * trees their bare spellings parse to: `SIGN(min_a)`, `max_a - min_b`, `CAST(min_a AS DOUBLE)`, + * `COALESCE(max_a, min_b)`. So the bare, qualified and alias spellings of one shape get ONE + * verdict and ONE message. Substituting the operand alone left an alias inside a function a bare + * column, which the statement rules then had to refuse by name ("compare the aggregate itself") + * -- for shapes whose bare spelling is accepted. */ private[query] def resolveAggregateAliases( criteria: Criteria, @@ -52,8 +94,75 @@ case object Having extends Expr("HAVING") with TokenRegex { val aliased: Map[String, Identifier] = aggregateAliases(request) if (aliased.isEmpty) return criteria + // Does this operand name an alias where the rules that read the substituted tree can see it -- + // itself, a function argument at any depth (`FunctionUtils.funIdentifiers`, the walk behind + // `Identifier.bucketMetrics`), a `CASE` condition? + // + // ⚠️ Deliberately the derivation's walk, not `update`'s. `ST_DISTANCE` keeps its operands + // outside `args`: `update` reaches them, `funIdentifiers` does not. Substituted there, an + // aggregate would sit where no rule can see it, and `ST_DISTANCE(max_a, min_b) > 1` would be + // refused as reading "neither a GROUP BY key nor an aggregate" with a remedy -- move it to + // WHERE -- that now names aggregates WHERE refuses. Left unsubstituted, it keeps its verdict. + def namesAlias(id: Identifier): Boolean = + FunctionUtils.funIdentifiers(id).exists(i => aliased.contains(i.name)) || + Case.conditionsOf(id).exists(_.referencedIdentifiers.exists(namesAlias)) + + // The aliases of SELECT `bucket_script` items: expressions over aggregates (`MAX(a) - MIN(b) AS + // d`), not aggregates. + val bucketScriptAliases: Set[String] = + aliased.collect { + case (alias, item) if !item.isAggregation && item.hasAggregation => alias + }.toSet + + // Does this operand read such an alias where no group filter answers it right, at any depth? + // + // ⚠️ Left unsubstituted, so it keeps the refusal it has always had (`SingleSearch.validate`: + // "cannot apply a function to the aggregate alias"). Two places, both MEASURED on + // Elasticsearch 8.18.3 over groups lacking an operand: + // - among the arguments of a function that decides what a NULL argument means -- COALESCE, + // GREATEST, LEAST ([[NullDecider]]). There the item is read through its OPERANDS + // (`params.max_a - params.min_b`), and an operand's NULL throws instead of making the item + // NULL, the one thing those functions exist to decide: `COALESCE(d, 0)`, `GREATEST(d, 1)` + // and `LEAST(d, 1)` failed the search with a null-pointer error; + // - under a conversion of the alias itself (`CAST(d AS DOUBLE)`, `d::INT`, `TRY_CAST`, + // `CONVERT`). The conversion is a function OF the item, so the filter declares the item -- + // but a comparison reads `params.d` and DROPS the conversion (`CAST(d AS BIGINT) > 1` + // compared `d` unconverted), and the value side and `ISNULL` / `ISNOTNULL` read the + // undeclared operands and failed with a null-pointer error. + // `SIGN(d)` -- NULL whenever an operand is, its filter declaring the operands it reads -- + // answered right in every position. + def unansweredOverBucketScript(id: Identifier): Boolean = + bucketScriptAliases.nonEmpty && (decidesNullOfBucketScript(id) || FunctionUtils + .funIdentifiers(id) + .exists(i => + bucketScriptAliases.contains(i.name) && i.functions.exists(_.isInstanceOf[Conversion]) + )) + + def decidesNullOfBucketScript(id: Identifier): Boolean = + id.functions.exists { + case NullDecider(arguments) => + arguments.exists { + case argument: Identifier => + FunctionUtils + .funIdentifiers(argument) + .exists(i => bucketScriptAliases.contains(i.name)) + case _ => false + } + case f: FunctionN[_, _] => + f.args.exists { + case argument: Identifier => decidesNullOfBucketScript(argument) + case _ => false + } + case _ => false + } + + // `update` is the walk that rebuilds: `GenericIdentifier.update` swaps every alias it meets for + // its SELECT item (see [[aliasScope]]). The result is updated again with the clause, as the + // operand alone always was. def substitute(id: Identifier): Identifier = - if (id.functions.isEmpty && !id.nested) aliased.getOrElse(id.name, id) else id + if (namesAlias(id) && !unansweredOverBucketScript(id)) + aliasScope.withValue(aliased)(id.update(request)) + else id def rewrite(c: Criteria): Criteria = c match { case p: Predicate => @@ -95,6 +204,17 @@ case class Having(criteria: Option[Criteria]) extends Updateable { criteria.map(c => Having.resolveAggregateAliases(c, request).update(request)) ) + /** The name of every metric this clause reads, in statement order: the `metricPathKey` of each of + * `Criteria.bucketMetrics` -- the keys of the `buckets_path` a `bucket_selector` over this + * clause must declare, and exactly the `params.` [[script]] reads. + * + * πŸ”΄ PUBLIC for `softclient4es-extensions`: a materialized view's pivot names each aggregation + * after its SELECT alias, and that alias IS the metric key here, so the view's filter declares + * `metricNames.map(n => n -> n)` and nothing else. The view rules in `SingleSearch` refuse a + * clause one of whose names the pivot does not create (`transformAggregationNames`). + */ + def metricNames: Seq[String] = criteria.toSeq.flatMap(_.bucketMetrics).map(_.metricPathKey) + /** Story 22.2 (AD-6) β€” HAVING reaches `criteria` through the SHARED `whereCriteria` production, * so a subquery node is grammatically reachable here. It is DECLINED in `validate()` rather than * by duplicating the alternation: one grammar, one reduction diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Where.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Where.scala index 0cb891ea2..c548263e2 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Where.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Where.scala @@ -27,7 +27,13 @@ import app.softnetwork.elastic.sql.`type`.{ } import app.softnetwork.elastic.sql.function.cond.Case import app.softnetwork.elastic.sql.function._ -import app.softnetwork.elastic.sql.function.cond.{Coalesce, ConditionalFunction, IsNotNull, IsNull} +import app.softnetwork.elastic.sql.function.cond.{ + Coalesce, + ConditionalFunction, + IsNotNull, + IsNull, + NumericReducer +} import app.softnetwork.elastic.sql.function.convert.Conversion import app.softnetwork.elastic.sql.function.geo.Distance import app.softnetwork.elastic.sql.parser.Validator @@ -196,15 +202,34 @@ sealed trait Criteria extends Updateable with PainlessScript { } } - def extractAllMetricsPath: Map[String, String] = - this match { - case Predicate(left, _, right, _, _) => - left.extractAllMetricsPath ++ right.extractAllMetricsPath - case relation: ElasticRelation => relation.criteria.extractAllMetricsPath - case _: MultiMatchCriteria => Map.empty - case e: Expression => e.extractAllMetricsPath - case _ => Map.empty + /** Every metric a bucket pipeline reading this criteria tree addresses, in statement order, + * deduplicated by `metricPathKey`: what the `bucket_selector` script reads, what `buckets_path` + * publishes, and -- for a materialized view -- the SELECT aliases its pivot must create. + * + * πŸ”΄ ONE function for every consumer (#292): [[extractAllMetricsPath]] (the search bridges' + * `buckets_path`), `MetricSelectorScript.nullAwareSelectorScript` (which of the metrics it reads + * as SELECT returns them), `Having.metricNames` and the materialized-view rules all read it, so + * the parameters a filter reads and the ones its path declares cannot drift apart. It walks both + * operands of every leaf through `Expression.leafBucketMetrics`, so an aggregate on the value + * side of a comparison, or inside a function argument, is never missed. + */ + def bucketMetrics: Seq[Identifier] = { + val all: Seq[Identifier] = this match { + case Predicate(left, _, right, _, _) => left.bucketMetrics ++ right.bucketMetrics + case relation: ElasticRelation => relation.criteria.bucketMetrics + case e: Expression => e.leafBucketMetrics + case _ => Nil } + all.foldLeft(Seq.empty[Identifier]) { (acc, id) => + if (acc.exists(_.metricPathKey == id.metricPathKey)) acc else acc :+ id + } + } + + /** The `buckets_path` of a bucket pipeline reading this criteria tree: each metric of + * [[bucketMetrics]] under its own key. + */ + def extractAllMetricsPath: Map[String, String] = + bucketMetrics.map(m => m.metricPathKey -> m.metricPathKey).toMap /** Extracts aggregation fields from criteria expressions (e.g. HAVING COUNT(*) > 1). Used to * ensure aggregations referenced only in HAVING/WHERE are included in the query. Note: returned @@ -536,6 +561,18 @@ private[query] object PredicatePrecedence { } } +/** A function that decides what a NULL argument means, and its arguments -- read by the group + * filter's null guard (`Expression.bucketPipelineGuard`). `COALESCE` takes the first argument that + * is not NULL, `GREATEST` / `LEAST` skip every NULL one: each is NULL only when every argument is. + */ +private[query] object NullDecider { + def unapply(f: Function): Option[List[PainlessScript]] = f match { + case coalesce: Coalesce => Some(coalesce.values) + case reducer: NumericReducer => Some(reducer.values) + case _ => None + } +} + sealed trait ElasticFilter case class ElasticBoolQuery( @@ -632,13 +669,6 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { case _ => identifier.dependencies } - override def extractAllMetricsPath: Map[String, String] = - maybeValue match { - case Some(v: Identifier) => - identifier.allMetricsPath ++ v.allMetricsPath - case _ => identifier.allMetricsPath - } - override def includes( bucket: Bucket, not: Boolean, @@ -1362,24 +1392,22 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { } } - /** Every metric THIS predicate reads, left operand and right operand alike, deduplicated and in - * order -- what [[bucketPipelineGuard]] guards, and the same derivation - * `Criteria.extractAggregationFields` creates the aggregations from and `extractAllMetricsPath` - * publishes. + /** Every metric THIS predicate reads, left operand and right operand alike, in order -- the leaf + * arm of `Criteria.bucketMetrics`, which deduplicates it. What [[bucketPipelineGuard]] guards, + * and the same derivation `Criteria.extractAggregationFields` creates the aggregations from and + * `extractAllMetricsPath` publishes. * * πŸ”΄ It used to be `identifier +: maybeValue.collect { case id if id.isAggregation }`, which * misses an aggregate reached through a function: `HAVING COUNT(*) > ABS(MAX(x))` emitted * `params.max_x` UNGUARDED (issue #389, measured on `main` -- `Math.abs(null)` fails the * search). */ - private[query] def bucketMetrics: Seq[Identifier] = - (identifier.bucketMetrics ++ maybeValue.toSeq + private[query] def leafBucketMetrics: Seq[Identifier] = + identifier.bucketMetrics ++ maybeValue.toSeq .collect { case id: Identifier => id } - .flatMap(_.bucketMetrics)).foldLeft(Seq.empty[Identifier]) { (acc, id) => - if (acc.exists(_.metricPathKey == id.metricPathKey)) acc else acc :+ id - } + .flatMap(_.bucketMetrics) /** The bucket-pipeline rendering of a predicate that reads an aggregate through a FUNCTION * (`HAVING COALESCE(COUNT(*), 0) > 1`, issue #389). @@ -1423,11 +1451,13 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { * Guarding an argument instead made `HAVING COALESCE(MAX(a), MIN(b)) > 1` drop a group whose * documents all lack `b` although `MAX(a)` is 5 (MEASURED on Elasticsearch 8.18.3, once such a * group's `MIN(b)` read as NULL): the guard landed on the argument the rendering does not test, - * the last one. Every other predicate keeps exactly the guard it had. + * the last one. `GREATEST` and `LEAST` decide it too -- they skip every NULL argument, and are + * NULL only when every argument is -- so they are guarded the same way, and their rendering + * skips a NULL metric (`NumericReducer`). Every other predicate keeps exactly the guard it had. */ private[query] def bucketPipelineGuard(rendering: Option[String]): Seq[String] = { val operands = identifier +: maybeValue.toSeq.collect { case id: Identifier => id } - if (operands.exists(readsCoalesce)) { + if (operands.exists(readsNullDecider)) { val nullTest = operator == IS_NULL || operator == IS_NOT_NULL val terms = operands.zipWithIndex.flatMap { case (operand, i) => // a null test reads its operand's value, NULL included @@ -1447,24 +1477,24 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { /** The guard terms of ONE operand of the comparison. * * `nullHandled`: what surrounds the operand decides what its NULL means -- an enclosing - * `COALESCE`, or the null test it is the operand of -- so its NULL is not guarded, only what - * would make it unreadable. A metric is always readable (a missing one reads as `null`); a - * function of one dereferences it. + * `COALESCE`, `GREATEST` or `LEAST`, or the null test it is the operand of -- so its NULL is not + * guarded, only what would make it unreadable. A metric is always readable (a missing one reads + * as `null`); a function of one dereferences it. * - * A `COALESCE` guards its arguments as `nullHandled` and adds the test of its own value, - * ` == null`, which holds exactly when every argument is NULL -- unless it cannot be - * NULL at all (a literal other than `NULL` among its arguments), or what surrounds it handles - * its NULL in turn. A function OF a `COALESCE` (`SIGN(COALESCE(...))`) propagates or - * dereferences that value, so its arguments are guarded as values. Anything else keeps the guard - * of every metric it reads. + * A function that decides what a NULL argument means ([[NullDecider]]) guards its arguments as + * `nullHandled` and adds the test of its own value, ` == null`, which holds exactly + * when every argument is NULL -- unless it cannot be NULL at all (a literal other than `NULL` + * among its arguments), or what surrounds it handles its NULL in turn. A function OF one + * (`SIGN(COALESCE(...))`) propagates or dereferences that value, so its arguments are guarded as + * values. Anything else keeps the guard of every metric it reads. */ private def nullGuardTerms(operand: Identifier, nullHandled: Boolean): Seq[String] = operand.functions match { - case List(coalesce: Coalesce) if !isMetric(operand) => - coalesce.values match { - // `COALESCE(x)` IS `x` + case List(NullDecider(values)) if !isMetric(operand) => + values match { + // `COALESCE(x)` IS `x`, and so are `GREATEST(x)` and `LEAST(x)` case List(single: Identifier) => nullGuardTerms(single, nullHandled) - case values => + case _ => val arguments = values .collect { case argument: Identifier => argument } .flatMap(nullGuardTerms(_, nullHandled = true)) @@ -1473,7 +1503,7 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { else Seq(s"${operand.painless(None)} == null") arguments ++ result } - case List(f: FunctionN[_, _]) if readsCoalesce(operand) => + case List(f: FunctionN[_, _]) if readsNullDecider(operand) => f.args .collect { case argument: Identifier => argument } .flatMap(nullGuardTerms(_, nullHandled = false)) @@ -1489,19 +1519,20 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { case _ => false } - /** Does this operand read a `COALESCE` -- its own, or one among its functions' arguments? */ - private def readsCoalesce(operand: Identifier): Boolean = + /** Does this operand read a [[NullDecider]] -- its own, or one among its functions' arguments? */ + private def readsNullDecider(operand: Identifier): Boolean = !isMetric(operand) && operand.functions.exists { - case _: Coalesce => true + case NullDecider(_) => true case f: FunctionN[_, _] => f.args.exists { - case argument: Identifier => readsCoalesce(argument) + case argument: Identifier => readsNullDecider(argument) case _ => false } case _ => false } - /** A `COALESCE` argument that is a literal other than `NULL`: that `COALESCE` is never NULL. */ + /** A [[NullDecider]] argument that is a literal other than `NULL`: that function is never NULL. + */ private def nonNullLiteral(argument: PainlessScript): Boolean = argument match { case literal: Value[_] => !literal.nullable case wrapped: Identifier if wrapped.name.isEmpty && !isMetric(wrapped) => diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/package.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/package.scala index a831a5c36..f511370e2 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/query/package.scala @@ -37,10 +37,12 @@ import app.softnetwork.elastic.sql.schema.{ import app.softnetwork.elastic.sql.function.{FunctionChain, FunctionUtils} import app.softnetwork.elastic.sql.function.cond.{Case, NullIf} import app.softnetwork.elastic.sql.function.aggregate.WindowFunction +import app.softnetwork.elastic.sql.function.time.DateDiff import app.softnetwork.elastic.sql.policy.{EnrichPolicy, EnrichPolicyType} import app.softnetwork.elastic.sql.serialization._ import app.softnetwork.elastic.sql.transform.{ AggregateConversion, + BucketScriptTransformAggregation, Delay, Frequency, TransformTimeInterval, @@ -57,6 +59,7 @@ import com.fasterxml.jackson.databind.JsonNode import java.time.{Duration, Instant} import scala.collection.immutable.ListMap +import scala.util.Try package object query { @@ -148,10 +151,10 @@ package object query { * watcher renders its SELECT into a SEARCH body and therefore keeps all five, so it deliberately * does NOT read this. * - * SIX rules. Each was MEASURED at RENDER level (the generated `TransformConfig`) either silently - * dropping the clause or deploying a `bucket_selector` that cannot run, before it was refused. - * They are stated as paragraphs rather than a numbered list because the formatter rewraps list - * items into the previous item's prose. + * SEVEN rules. Each was MEASURED at RENDER level (the generated `TransformConfig`) either + * silently dropping the clause or deploying a `bucket_selector` that cannot run, before it was + * refused. They are stated as paragraphs rather than a numbered list because the formatter + * rewraps list items into the previous item's prose. * * RULE 1 -- NO `GROUP BY`. A transform's pivot is built from the GROUP BY alone, and with no * pivot there is nothing for a `bucket_selector` to hang on: the clause was never even @@ -166,60 +169,50 @@ package object query { * RULE 3 -- an aggregate a view's transform cannot compute at all. * `AggregateConversion.toTransformAggregation` has arms for MIN / MAX / SUM / AVG / COUNT and * answers `None` for every other aggregate; `Stage.buildAggregations()` `flatMap`s that `None` - * away while `Stage.extractAggregatePaths` keys on *is this an aggregate*, so the two DIVERGE. - * MEASURED: `STDDEV`, `VARIANCE` and `PERCENTILE_CONT` in the SELECT list AND the HAVING - * produced a `buckets_path` naming an aggregation the transform never creates. + * away, so the filter would read an aggregation that is never created. MEASURED: `STDDEV`, + * `VARIANCE` and `PERCENTILE_CONT` in the SELECT list AND the HAVING produced a `buckets_path` + * naming an aggregation the transform never creates. * - * RULE 4 -- a SELECT `bucket_script` alias (`MAX(x) - MIN(x) AS d … HAVING d > 3`). This repo's - * transform pivot model has a `bucketSelector` field and NO `bucketScript` one - * (`TransformPivot`), and such a SELECT item is not an aggregate, so it never reaches the - * stage's aggregate list either: MEASURED `buckets_path` EMPTY, clause dropped. - * - * RULE 5 -- an aggregate on the RIGHT of a comparison THAT NO LEAF NAMES ON ITS LEFT. - * `Stage.extractAggregatePaths` walks `expr.identifier` ONLY and never `expr.maybeValue`, while - * core's own `Expression.extractAllMetricsPath` walks BOTH. So an aggregate reached only through - * a value side lands in `buildAggregations` and NEVER in `buckets_path`. MEASURED: the selector - * IS built, reads `params.` which is null, the null guard short-circuits, and EVERY - * bucket is rejected -- an EMPTY view at HTTP 200. - * - * πŸ”΄ Both halves of that sentence are load-bearing, and each was measured after a gate found the - * rule too wide. `extractAggregatePaths` declares the LEFT identifier of EVERY leaf in the whole - * clause, so (i) a sibling conjunct naming the same aggregate DECLARES it and the view runs - * correctly -- firing on mere presence over-refused 74 cells that are correct on `main`; and - * (ii) when nothing names it on the left but the SELECT list does not publish it either, adding - * the SELECT alias is what makes the selector declare it, so RULE 6 owns that family and this - * rule stands down. `HAVING MIN(amount) > 1 AND MAX(amount) > MIN(amount)` is ACCEPTED once - * `MIN(amount) AS mn` is published; `HAVING MAX(amount) > MIN(amount)` is not. See - * [[SingleSearch.havingLeftHandNames]]. + * RULE 4 -- a SELECT `bucket_script` alias (`MAX(x) - MIN(x) AS d … HAVING d > 3`) the view's + * per-group calculation channel does not serve. A view computes such an item with the + * `bucket_script` of [[SingleSearch.transformBucketScripts]], and its filter reads it by name + * like any other metric. Not served: an item one of whose operands no transform computes (it is + * never created), and a condition whose rendering reads the item's operands rather than `d` + * (`ISNULL(d)`, [[SingleSearch.bucketScriptServed]]). * * RULE 6 -- an aggregate a transform COULD compute but the SELECT list does not publish, in the * spelling the HAVING uses. The selector would read a metric the view never creates: either * `buckets_path` comes back empty and the whole clause is dropped, or -- with a published metric * beside it -- the path is PARTIAL and the script reads a `params.*` the path never declares. * - * RULE 7 -- THE BACKSTOP: a `HAVING` leaf whose name matches no aggregation the pivot creates. - * It covers two families the six specific rules are structurally blind to. (a) An aggregate with - * NO SELECT alias: `Select.fieldAliases` mints a generated internal alias and - * `Identifier.update` copies it, so the script reads THAT name, while `RequiredField.apply` - * names the aggregation `fieldAlias.getOrElse(sourceField)` -- the USER alias only. The two - * never match, `buckets_path` comes back EMPTY and the clause is SILENTLY DROPPED; MEASURED - * invariant across every computable aggregate, every connective, one and two grouping keys, and - * a JOIN body. (b) A relation predicate (`NESTED` / `CHILD` / `PARENT`) beside a metric - * conjunct: `extractAggregatePaths` falls to its `case _ => acc` for a relation and - * `havingLeaves` excludes them, so the selector is emitted reading the metric alone -- a PARTIAL - * filter answering 200 with the wrong groups. + * RELATIONS -- a `NESTED` / `CHILD` / `PARENT` predicate. A view's filter is a `bucket_selector` + * over the pivot's own aggregations, and a relation predicate has no form there: beside a metric + * conjunct the selector reads the metric alone -- a PARTIAL filter answering 200 with the wrong + * groups. * - * πŸ”΄ The backstop is LAST, and the rule set is deliberately NOT collapsed into it. It subsumes - * rules 2, 3 and 4 entirely and 5 and 6 in part, so deleting them is tempting -- do not. Those - * rules exist for their SPECIFIC REMEDIES (filter the key in WHERE; this aggregate exists in no - * view; compare with a constant; publish it under an alias), and a single generic "names no - * aggregation this view creates" would lose every one of them. Specific first, backstop behind. + * RULE 7 -- THE BACKSTOP: a metric the `HAVING` reads that the pivot does not create. The view's + * filter declares exactly `Having.metricNames` -- every metric the clause reads, the value side + * of a comparison and a function's arguments included (`Criteria.bucketMetrics`, #292) -- and + * the pivot names each aggregation after its SELECT alias ([[transformAggregationNames]]), so + * every name must be one of them. Its commonest family: an aggregate with NO SELECT alias. + * `Select.fieldAliases` mints a generated internal alias and `Identifier.update` copies it, so + * the script reads THAT name, while `RequiredField.apply` names the aggregation + * `fieldAlias.getOrElse(sourceField)` -- the USER alias only. The two never match, and the + * clause would be SILENTLY DROPPED; MEASURED invariant across every computable aggregate, every + * connective, one and two grouping keys, and a JOIN body. * - * ⚠️ MEASURED over 5,145 product points: rules 2, 3 and 4 have ZERO sole-owner cells -- every - * statement they refuse would be refused by a later rule anyway -- while rule 5 owns 25 (all of - * them genuinely broken), rule 6 owns 2, rule 1 owns 83 and the backstop owns 134. Their - * coverage contribution is nil ON PURPOSE and it is not a reason to delete them: the remedy is - * the deliverable, not the verdict. + * πŸ”΄ Two aggregates in one condition -- `MAX(x) > MIN(y)`, `COALESCE(MAX(x), MIN(y)) > 1`, + * `MAX(x) > COALESCE(MIN(y), 1)` -- need no rule of their own. They used to (the old rule 5 + * refused an aggregate on the value side of a comparison, and the backstop named only each + * leaf's left identifier) because the view's filter declared only the left-hand metric of each + * comparison. With the filter declaring `Having.metricNames`, every metric the script reads is + * declared, and the backstop asks the same question of every one of them. + * + * πŸ”΄ The backstop is LAST, and the rule set is deliberately NOT collapsed into it. It subsumes + * rules 3 and 4 and, in part, 6, so deleting them is tempting -- do not. Those rules exist for + * their SPECIFIC REMEDIES (filter the key in WHERE; this aggregate exists in no view; publish it + * under an alias), and a single generic "names no aggregation this view creates" would lose + * every one of them. Specific first, backstop behind. * * πŸ”΄ Every remedy that puts an aggregate in the SELECT list says `AS `, and that is not * politeness: a view's HAVING can only read an aggregate that carries a SELECT alias, so *"spell @@ -227,26 +220,16 @@ package object query { * broken artefact, because a HAVING never spells an alias. Applying each remedy literally is a * test. * - * πŸ”΄ Rules 3, 4 and 5 run BEFORE rule 6 deliberately, and the reason is measured rather than - * aesthetic. Rule 6's remedy is *"add it to the SELECT list"*, and for the populations those - * three carry, applying it LANDS SOMEWHERE WORSE: adding `STDDEV(amount)` produces the broken - * `buckets_path` rule 3 refuses, and adding the right-hand `MIN(amount)` produces the - * every-bucket-rejected selector rule 5 refuses. A refusal whose remedy makes things worse is - * the issue-#389 trap ("verify the remedy a message prescribes, the same way you verify the - * defect"). Each precedence has its own test and its own mutation. + * πŸ”΄ Rule 3 runs BEFORE rule 6 deliberately, and the reason is measured rather than aesthetic. + * Rule 6's remedy is *"add it to the SELECT list"*, and for the population rule 3 carries, + * applying it LANDS SOMEWHERE WORSE: adding `STDDEV(amount)` produces the broken `buckets_path` + * rule 3 refuses. A refusal whose remedy makes things worse is the issue-#389 trap ("verify the + * remedy a message prescribes, the same way you verify the defect"). * - * πŸ”΄ The converse is equally measured, and it is why rule 5 asks a counterfactual rather than - * *"is this parameter declared today"*: where SOME leaf names the right-hand aggregate on its - * left, rule 6's remedy DOES end the journey, so rule 5 must stand down and let rule 6 speak. - * Precedence is not a fixed order between two rules -- it is whichever remedy reaches a view - * that deploys. W1-W8 pin both directions, with the remedy applied literally in W5 and W8. - * - * πŸ”΄ Rules 3 and 5 ask the real thing, never a copy of it: rule 3 calls `toTransformAggregation` - * ITSELF rather than matching a list of function names, and rule 5 is expressed as *the value - * side of the comparison*, which is exactly the operand `extractAggregatePaths` omits. A name - * list would be a second derivation of what the transform emitter does and would drift the first - * time that mapping changed -- the same reason [[SingleSearch.notPublishedBySelect]] is hoisted - * rather than copied. + * πŸ”΄ Rule 3 asks the real thing, never a copy of it: it calls `toTransformAggregation` ITSELF + * rather than matching a list of function names. A name list would be a second derivation of + * what the transform emitter does and would drift the first time that mapping changed -- the + * same reason [[SingleSearch.notPublishedBySelect]] is hoisted rather than copied. * * ⚠️ Attribution: these are limitations of THIS ENGINE's transform model, not of Elasticsearch. * A `bucket_selector` demonstrably reaches a transform pivot -- `TransformPivot` carries one -- @@ -299,23 +282,29 @@ package object query { } } .orElse { - search.havingBucketScriptRefs.headOption.map { id => - s"MATERIALIZED VIEW cannot filter on ${id.sql} in HAVING: it is an expression " + - "over aggregates, which a search evaluates with a bucket_script and this " + - "engine's transform pivot has no bucket_script channel for, so the condition " + - "would be silently dropped. Apply the condition when querying the view." - } - } - .orElse { - search.havingValueSideAggs.headOption.map { id => - s"MATERIALIZED VIEW cannot compare two aggregates in HAVING (${id.sql} is on the " + - "right of the comparison): a materialized view's transform declares only the " + - "metric on the LEFT of each comparison in its bucket_selector buckets_path, so " + - "the right-hand aggregate is read as an undeclared parameter, the null guard " + - "rejects every group and the view comes out EMPTY. Compare the aggregate with a " + - "constant -- keeping its SELECT alias (SUM(x) AS s) -- or apply the condition " + - "when querying the view." - } + search.havingBucketScriptRefs + .collectFirst { + case (leaf, id) if !search.bucketScriptServed(leaf, id) => (leaf, id) + } + .map { case (leaf, id) => + val alias = id.metricPathKey + val item = search.select.fields.find(_.fieldAlias.exists(_.alias == alias)) + item.flatMap(search.bucketScriptOperandNoTransformComputes) match { + case Some(operand) => + s"MATERIALIZED VIEW cannot filter on $alias in HAVING: it is an expression " + + s"over aggregates (${item.map(_.identifier.sql).getOrElse(id.sql)}), which a " + + "materialized view computes with a bucket_script over the aggregations its " + + s"transform creates, and ${operand.sql} is not one a transform can create, " + + "so the condition would read a value that is never computed. Apply the " + + "condition when querying the view." + case None => + s"MATERIALIZED VIEW cannot filter on ${leaf.sql} in HAVING: it reads $alias, " + + "an expression over aggregates which a materialized view computes with a " + + s"bucket_script, through its operands rather than as $alias, and the view's " + + s"group filter declares only $alias. Compare $alias itself (HAVING $alias > " + + "10), or apply the condition when querying the view." + } + } } .orElse { search.havingOnlyAggs.headOption.map { f => @@ -333,20 +322,28 @@ package object query { "alias." } } + .orElse { + search.havingRelation.map { relation => + s"MATERIALIZED VIEW cannot filter on ${relation.sql} in HAVING: a materialized " + + "view's transform filters groups with a bucket_selector over the aggregations " + + "its pivot creates, and a nested, child or parent predicate has no form there, " + + "so the group filter would be applied only in part. Apply the condition when " + + "querying the view." + } + } .orElse { // πŸ”΄ THE BACKSTOP. Last on purpose; see the rule list above. - search.havingLeafWithNoMatchingAgg.map { case (leaf, _) => + search.havingMetricNotCreated.map { metric => val created = search.transformAggregationNames.toSeq.sorted val names = if (created.isEmpty) "this view creates none" else created.mkString(", ") - s"MATERIALIZED VIEW cannot filter on ${leaf.sql} in HAVING: a materialized " + - "view's transform names each aggregation after its SELECT alias, and this " + - s"condition matches none of the aggregations this view creates ($names), so the " + - "group filter would be silently dropped or applied only in part. Every " + + s"MATERIALIZED VIEW cannot filter on ${metric.sql} in HAVING: a materialized " + + "view's transform names each aggregation after its SELECT alias, and " + + s"${metric.sql} matches none of the aggregations this view creates ($names), so " + + "the group filter would be silently dropped or applied only in part. Every " + "aggregate a view's HAVING reads must be written in the SELECT list with an " + - "alias (SUM(x) AS s), and a nested, child or parent predicate cannot be part of " + - "a view's HAVING at all." + "alias (SUM(x) AS s)." } } case _ => None @@ -745,6 +742,25 @@ package object query { lazy val sorts: ListMap[String, SortOrder] = ListMap(orderBy.map { _.sorts.map(s => s.name -> s.direction) }.getOrElse(Seq.empty): _*) + /** The first DATEDIFF / DATE_DIFF / TIMESTAMPDIFF call of this statement with a window function + * as an operand (`MAX(d) OVER (PARTITION BY g)`, `FIRST_VALUE(d) OVER (ORDER BY d)`), as (the + * call, the operand): the SELECT list, WHERE, HAVING and ORDER BY ([[scriptedExpressions]]), + * at any depth of each. Read by the window rule in `validate()`. + * + * A window function written `OVER ()` partitions and orders nothing: it IS its aggregate. + */ + private[query] lazy val dateDiffOverWindow: Option[(Identifier, Identifier)] = + scriptedExpressions.iterator + .flatMap(FunctionUtils.funIdentifiers(_)) + .flatMap { call => + call.functions + .collect { case dateDiff: DateDiff => dateDiff } + .flatMap(_.args.collect { + case operand: Identifier if operand.windows.exists(_.isWindowing) => call -> operand + }) + } + .collectFirst { case found => found } + def update(schema: Option[Schema] = None): SingleSearch = { schema match { case Some(s) => return this.copy(schema = Some(s)).update() @@ -993,75 +1009,129 @@ package object query { /** The `HAVING` references that are an EXPRESSION over aggregates rather than an aggregate -- * what a search emits as a `bucket_script` (`MAX(x) - MIN(x) AS d ... HAVING d > 3`, resolved - * to its SELECT item by `Having.resolveAggregateAliases`). + * to its SELECT item by `Having.resolveAggregateAliases`) -- each with the leaf reading it. * - * A transform's pivot has no `bucket_script` channel, and such a SELECT item is not an - * aggregate, so it never enters the stage's aggregate list either: MEASURED, `buckets_path` - * comes back EMPTY and the whole clause is dropped. + * A view computes such an item with the `bucket_script` [[transformBucketScripts]] derives, + * and its HAVING reads that aggregation by name. Rule 4 refuses the references that channel + * does not serve. * * The predicate is the one `SingleSearch.validate()` already uses to find inline arithmetic * over aggregates, minus its `fieldAlias.isEmpty` guard -- there it refuses the UNALIASED form - * for every venue; here the ALIASED form is the one a transform cannot honour. + * for every venue; here the ALIASED form is the one the channel must serve. */ - private[query] lazy val havingBucketScriptRefs: Seq[Identifier] = - having - .flatMap(_.criteria) - .map(_.referencedIdentifiers) - .getOrElse(Nil) - .filter(id => !id.isAggregation && id.hasAggregation) - - /** The aggregates this statement's `HAVING` compares AGAINST -- the VALUE side of a comparison. - * - * πŸ”΄ The operand `Stage.extractAggregatePaths` omits. It walks `expr.identifier` only and - * never `expr.maybeValue`, while core's own [[Expression.extractAllMetricsPath]] walks BOTH, - * so a right-hand aggregate reaches `buildAggregations` (the view really does compute it) and - * never reaches `buckets_path`. MEASURED: the `bucket_selector` IS built, its script reads - * `params.`, that parameter is undeclared and therefore null, the null guard - * short-circuits and EVERY bucket is rejected -- the view materialises EMPTY at HTTP 200. - * - * Expressed as *the value side*, deliberately: that is the same operand the consumer omits, so - * the rule and the defect cannot drift apart. Where publishing the aggregate does not help, - * this must be decided before the "not published by the SELECT list" rule -- and where it DOES - * help, [[havingLeftHandNames]] hands the statement to that rule instead (see below). - * - * πŸ”΄ NARROWED after the coverage gate: PRESENCE of an aggregate on the value side is NOT the - * defect -- being UNDECLARED is. `extractAggregatePaths` declares a parameter for the left - * identifier of EVERY leaf in the whole clause, so a sibling conjunct naming the same - * aggregate declares it and the transform deploys and runs correctly. MEASURED on the control: - * `HAVING SUM(amount) > MAX(amount) AND MAX(amount) > 1` has UNDECLARED = none, and the same - * aggregate on both sides (`MAX(x) > MAX(x)`) declares itself. Firing on presence over-refused - * 74 cells that are CORRECT on main -- a regression, and its remedy ("compare with a - * constant") would have changed the meaning of a working query. - * - * πŸ”΄ NARROWED a second time, by the same argument one step further out: the set of declaring - * names is [[havingLeftHandNames]], NOT the parameters the pivot declares TODAY. The question - * this rule must answer is the COUNTERFACTUAL -- would publishing the right-hand aggregate - * make the selector declare it? MEASURED: `HAVING MIN(amount) > 1 AND MAX(amount) > - * MIN(amount)` is ACCEPTED once `MIN(amount) AS mn` joins the SELECT list, so rule 6 owns it - * and its remedy ends the journey; `HAVING MAX(amount) > MIN(amount)`, where no leaf names - * `MIN(amount)` on the left, is refused again after publishing it, so this rule owns that one. - * Asking the today-question instead sent the first family here, whose remedy ("compare with a - * constant") would have changed the MEANING of a query a one-line SELECT alias fixes. Pinned - * by the W cells, remedy applied literally in W5 / W8. - * - * ⚠️ The `isAggregation` guard is a SECOND LINE OF DEFENCE and is unreachable from SQL, which - * is stated because it was MEASURED rather than assumed: a value side that is NOT an aggregate - * is already refused by the shared rules inside `dql.validate()`, before this runs - * -- a plain column (`HAVING COUNT(*) > amount`) and a grouping key (`… > city`) by - * `Having.unrepresentable` ("its rendering can evaluate to NULL"), and `HAVING city = status` - * by the key-predicate rule. A SELECT alias of an aggregate is SUBSTITUTED by - * `Having.resolveAggregateAliases` before it gets here, so it passes the guard as the - * aggregate it is. The guard therefore keeps the derivation's NAME true for a `SingleSearch` - * assembled in code -- the same role `MetricSelectorScript.metricSelector`'s throw plays -- - * and its mutation is GREEN for that reason, not for want of a test. - */ - /** The aggregation NAMES a materialized view's pivot actually creates. + private[query] lazy val havingBucketScriptRefs: Seq[(Criteria, Identifier)] = { + def walk(c: Criteria): Seq[(Criteria, Identifier)] = c match { + case p: Predicate => walk(p.leftCriteria) ++ walk(p.rightCriteria) + case relation: ElasticRelation => walk(relation.criteria) + case leaf => + leaf.referencedIdentifiers + .filter(id => !id.isAggregation && id.hasAggregation) + .map(leaf -> _) + } + having.flatMap(_.criteria).toSeq.flatMap(walk) + } + + /** The per-group calculation channel of a materialized view: the `bucket_script` its pivot + * computes for each SELECT item that is arithmetic over aggregates (`MAX(a) - MIN(b) AS d`), + * keyed by the item's SELECT alias -- the name the pivot gives it, and the name a HAVING over + * `d` reads it by. + * + * The script is the item's own bucket-pipeline rendering, the one a search's `bucket_script` + * runs: it reads each operand aggregate as `params.`. `bucketsPath` maps that + * key to the pivot aggregation computing the operand -- the SELECT item that publishes it + * (named after its alias), or else the auxiliary aggregation of + * [[transformBucketScriptOperands]], named after the key itself. + * + * Served only when every operand is an aggregate a transform computes + * (`toTransformAggregation`, asked itself) and the item carries a SELECT alias to be named + * after. An item it does not serve is never computed by a view; rule 4 refuses a HAVING over + * one. + * + * πŸ”΄ PUBLIC for `softclient4es-extensions`, whose pivot emits each entry under its key, beside + * the aggregations of [[transformBucketScriptOperands]], and binds each of its `params`. + */ + lazy val transformBucketScripts: ListMap[String, BucketScriptTransformAggregation] = + ListMap(select.fields.flatMap { f => + f.fieldAlias.map(_.alias) match { + case Some(alias) + if f.isBucketScript && bucketScriptOperandNoTransformComputes(f).isEmpty => + // the rendering a search's `bucket_script` runs; one that cannot be rendered is not + // served, never an internal error + Try(f.identifier.painless(None)).toOption.map { script => + val bucketsPath = ListMap(f.identifier.referencedAggregates.map { op => + op.metricPathKey -> publisherOf(op).map(_.outputName).getOrElse(op.metricPathKey) + }: _*) + alias -> BucketScriptTransformAggregation( + expression = f.identifier.sql, + bucketsPath = bucketsPath, + script = script, + // what the script reads beyond its bucket paths (`__now__`), for the pivot to bind + params = MetricSelectorScript.paramsRead(script).filterNot(bucketsPath.contains) + ) + } + case _ => None + } + }: _*) + + /** The operand aggregates of [[transformBucketScripts]] the SELECT list does not publish, each + * aliased by the key its script reads it under -- the auxiliary aggregations a view's pivot + * must create for them, as a search creates them for its own `bucket_script`. + * + * πŸ”΄ PUBLIC for `softclient4es-extensions`: the pivot computes each as it computes a SELECT + * aggregate, under its alias. + */ + lazy val transformBucketScriptOperands: Seq[Field] = + select.fields + .filter(f => f.fieldAlias.exists(a => transformBucketScripts.contains(a.alias))) + .flatMap(_.identifier.referencedAggregates) + .filter(op => publisherOf(op).isEmpty) + .foldLeft(Seq.empty[Field]) { (acc, op) => + if (acc.exists(_.identifier.metricPathKey == op.metricPathKey)) acc + else acc :+ Field(op, Some(Alias(op.metricPathKey))) + } + + /** The SELECT aggregate that publishes `operand`, matched by expression like + * [[notPublishedBySelect]]. + */ + private def publisherOf(operand: Identifier): Option[Field] = + select.fields.find(f => + f.isAggregation && f.identifier.identifierName == operand.identifierName + ) + + /** Does the view's per-group calculation channel serve this `HAVING` leaf's reference to a + * SELECT `bucket_script` item? The item is created ([[transformBucketScripts]]), and the + * leaf's filter reads it as the metric its alias names -- nothing but the metrics the leaf + * declares, so `Having.metricNames` covers every parameter the view's filter reads. + * + * πŸ”΄ Asked of the RENDERING, because the reference alone cannot answer it: a null test over + * the alias (`ISNULL(d)`) references the item itself, yet renders over its OPERANDS + * (`params.max_a - params.min_b == null`) behind a guard on `params.d`, so its filter would + * read parameters the view never declares. `d > 1` reads `params.d` alone and is served. + */ + private[query] def bucketScriptServed(leaf: Criteria, ref: Identifier): Boolean = + transformBucketScripts.contains(ref.metricPathKey) && + Try(MetricSelectorScript.nullAwareSelectorScript(leaf)).toOption.exists( + _.forall(script => + MetricSelectorScript + .metricsRead(script) + .subsetOf(leaf.bucketMetrics.map(_.metricPathKey).toSet) + ) + ) + + /** The first operand of a SELECT `bucket_script` item no transform computes, if any. */ + private[query] def bucketScriptOperandNoTransformComputes(item: Field): Option[Identifier] = + item.identifier.referencedAggregates.find( + _.aggregateFunction.flatMap(_.toTransformAggregation).isEmpty + ) + + /** The aggregation NAMES a materialized view's pivot actually creates for its SELECT list. * * `Stage.buildAggregations()` keeps a SELECT aggregate only when * `AggregateConversion.toTransformAggregation` answers `Some`, and `RequiredField.apply` names * it `field.fieldAlias.map(_.alias).getOrElse(field.sourceField)` -- which is * [[Field.outputName]] character for character, so core's own derivation is used rather than a - * copy of the consumer's formula (verified: writing either spells the same set). + * copy of the consumer's formula (verified: writing either spells the same set). A SELECT item + * that is arithmetic over aggregates is created under its alias by [[transformBucketScripts]]. * * πŸ”΄ What matters is the INPUT, not the formula: `select.fields`, RAW. The generated alias * `select.fieldsWithComputedAliases` mints for an unaliased item never reaches the consumer, @@ -1070,82 +1140,51 @@ package object query { * * ⚠️ The `toTransformAggregation` filter is unreachable from SQL, because rule 3 refuses an * un-computable aggregate in a HAVING before the backstop runs; it is kept so the set is - * honestly "what the pivot creates" for a `SingleSearch` assembled in code. Its mutation is - * GREEN for that reason (see `havingValueSideAggs` for the same situation). - */ - /** The name `Stage.extractAggregatePaths` computes for an identifier -- ONE derivation, read by - * [[havingLeafAggNames]] (the declaring side) and by [[havingValueSideAggs]] (the value side), - * so the two sides of a comparison cannot be named by two different rules. - */ - private[query] def transformFieldName(id: Identifier): String = - id.fieldAlias match { - case Some(alias) => alias - case None if id.name.nonEmpty => id.name - case _ => AliasUtils.normalize(id.identifierName) - } - + * honestly "what the pivot creates" for a `SingleSearch` assembled in code. + */ private[query] lazy val transformAggregationNames: Set[String] = select.fields.flatMap { f => f.aggregateFunction.flatMap(_.toTransformAggregation).map(_ => f.outputName) - }.toSet + }.toSet ++ transformBucketScripts.keySet - /** The name each `HAVING` leaf would be looked up under, paired with the leaf itself. - * - * πŸ”΄ This walks the WHOLE criteria tree, `ElasticRelation` INCLUDED -- unlike - * [[havingLeaves]], which deliberately excludes relation predicates. That difference is the - * whole point of the backstop: `Stage.extractAggregatePaths` falls to its `case _ => acc` for - * a relation, so a relation conjunct beside a metric one is invisible to every rule built on - * `havingLeaves` and the selector is emitted reading the metric alone -- a PARTIAL filter - * answering 200 with the wrong groups. - * - * The name is computed exactly as `extractAggregatePaths` computes it, so the two cannot - * disagree about which leaf resolves to which aggregation. + /** The first metric this statement's `HAVING` reads that the view's pivot does not create, or + * `None` -- rule 7, THE BACKSTOP: every name of `Having.metricNames` must be one of + * [[transformAggregationNames]], because those names are the `buckets_path` the view's filter + * declares. ONE derivation (`Criteria.bucketMetrics`) for the names the filter reads, the + * names it declares and this check (#292), so a metric on the value side of a comparison or + * inside a function argument is checked like the left-hand one. */ - private[query] lazy val havingLeafAggNames: Seq[(Criteria, String)] = { - def walk(c: Criteria): Seq[(Criteria, String)] = c match { - case p: Predicate => walk(p.leftCriteria) ++ walk(p.rightCriteria) - case relation: ElasticRelation => walk(relation.criteria) - case e: Expression => Seq(c -> transformFieldName(e.identifier)) - case _ => Nil + private[query] lazy val havingMetricNotCreated: Option[Identifier] = + having + .flatMap(_.criteria) + .toSeq + .flatMap(_.bucketMetrics) + .find(m => !transformAggregationNames.contains(m.metricPathKey)) + + /** The first relation predicate (`NESTED` / `CHILD` / `PARENT`) of this statement's `HAVING` + * that carries a condition reading no metric -- a document-level condition (`CHILD(c.x = 1)`). + * + * A view's filter is a `bucket_selector` over the pivot's own aggregations: such a condition + * has no form there, and a selector built beside it reads the metric conjunct alone -- a + * PARTIAL filter answering 200 with the wrong groups. + * + * ⚠️ A relation whose every condition reads a metric is not one: `update` wraps a condition on + * an UNNEST aggregate in `ElasticNested` itself, and that condition reads its metric like any + * other, so rule 7 checks its name -- the verdict it always had. + */ + private[query] lazy val havingRelation: Option[ElasticRelation] = { + def readsNoMetric(c: Criteria): Boolean = c match { + case p: Predicate => readsNoMetric(p.leftCriteria) || readsNoMetric(p.rightCriteria) + case relation: ElasticRelation => readsNoMetric(relation.criteria) + case other => other.bucketMetrics.isEmpty } - having.flatMap(_.criteria).toSeq.flatMap(walk) - } - - /** Every name some `HAVING` leaf carries on its LEFT -- the names the selector's `buckets_path` - * declares, or WOULD declare once the SELECT list publishes them. - * - * πŸ”΄ `Stage.extractAggregatePaths` declares a parameter for the LEFT identifier of EVERY leaf - * in the WHOLE clause, keeping the ones the pivot creates. So a name is declared as soon as - * ANY leaf's identifier side carries it -- a sibling conjunct counts -- AND the SELECT list - * publishes it. - * - * πŸ”΄ Deliberately NOT intersected with [[transformAggregationNames]], because rule 5 asks a - * COUNTERFACTUAL, not what the pivot declares today: would publishing this aggregate make the - * selector declare it? MEASURED both ways -- `HAVING MIN(amount) > 1 AND MAX(amount) > - * MIN(amount)` becomes ACCEPTED once `MIN(amount) AS mn` joins the SELECT list, so rule 6's - * remedy ends the journey and rule 6 must speak; `HAVING MAX(amount) > MIN(amount)`, where no - * leaf names `MIN(amount)` on the left, is still refused after publishing it, so rule 5 must - * speak for that one instead of steering the user into a second refusal. Intersecting here - * routes the first family to rule 5, whose remedy ("compare with a constant") changes the - * MEANING of a query a one-line SELECT alias would have fixed. - */ - private[query] lazy val havingLeftHandNames: Set[String] = - havingLeafAggNames.map(_._2).toSet - - /** The first `HAVING` leaf whose name matches no aggregation the pivot creates, or `None`. - * - * The UNIFIED backstop (see `materializedViewHavingRefusal`): it subsumes several of the - * specific rules, which are deliberately kept IN FRONT of it for their remedies. - */ - private[query] lazy val havingLeafWithNoMatchingAgg: Option[(Criteria, String)] = - havingLeafAggNames.find { case (_, name) => !transformAggregationNames.contains(name) } - - private[query] lazy val havingValueSideAggs: Seq[Identifier] = - havingLeaves.flatMap(_.maybeValue).collect { - case id: Identifier - if id.isAggregation && !havingLeftHandNames.contains(transformFieldName(id)) => - id + def find(c: Criteria): Option[ElasticRelation] = c match { + case p: Predicate => find(p.leftCriteria).orElse(find(p.rightCriteria)) + case relation: ElasticRelation if readsNoMetric(relation.criteria) => Some(relation) + case _ => None } + having.flatMap(_.criteria).flatMap(find) + } // Aggregations referenced only in HAVING, WHERE, or ORDER BY clauses (not in SELECT) private lazy val auxiliaryAggs: Seq[Field] = { @@ -1840,11 +1879,16 @@ package object query { } _ <- { // The residual of the rule above, and the reason it needs the STATEMENT rather than the - // clause: `HAVING NULLIF(c, 0) > 1` names a SELECT aggregate by its ALIAS from inside a - // function ARGUMENT. `Having.resolveAggregateAliases` substitutes an OPERAND only, so `c` - // stays a bare column, the predicate references no aggregate at all, and the rule above - // cannot see it -- it renders `arg0 == 0 ? null : arg0 > 1`, reading a context parameter - // no bucket pipeline binds. MEASURED: dropped silently on `main`. + // clause: a function of a SELECT aggregate's ALIAS where `Having.resolveAggregateAliases` + // leaves the alias in place -- a relation predicate or a MATCH, which it does not touch + // (`HAVING NESTED(ABS(c) > 1)`), and a SELECT `bucket_script` alias among the arguments + // of a COALESCE / GREATEST / LEAST (`COALESCE(d, 0)`) or under a conversion (`CAST(d AS + // DOUBLE)`), which no group filter answers right (see there). There `c` stays a bare + // column, the predicate references + // no aggregate at all, and the rule above cannot see it -- it renders a context parameter + // no bucket pipeline binds (MEASURED before #389: dropped silently). Everywhere else the + // substitution reaches every depth (`NULLIF(c, 0)` IS `NULLIF(COUNT(*), 0)`), so no + // alias is left for this rule to find. // // ⚠️ The alias set is read from `Having.aggregateAliases`, the SAME map the substitution // uses, so the two cannot disagree about which bare names are aggregate references. A @@ -2259,6 +2303,26 @@ package object query { Right(()) } } + _ <- { + // A DATEDIFF-family call over a WINDOW function with no GROUP BY + // (`SELECT g, DATEDIFF(MAX(d) OVER (PARTITION BY g), '2024-01-01') AS x FROM t`): the + // window's values come from an aggregation of their own, merged into the rows, while the + // call over them is a per-group calculation with no group to run in. MEASURED on ES + // 8.18.3: every such statement failed in Elasticsearch ("bucket_script aggregation [x] + // must be declared inside of another aggregation"). Refused by name, as a window function + // under ISNULL is (the function-chain rule). With a GROUP BY it is computed per group, + // unchanged. + if (groupBy.isDefined) Right(()) + else + dateDiffOverWindow match { + case Some((call, window)) => + Left( + "DATEDIFF, DATE_DIFF and TIMESTAMPDIFF over a window function are not supported " + + s"yet (${window.sql} in ${call.sql}); select the window function on its own" + ) + case None => Right(()) + } + } } yield () } diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/transform/TransformAggregation.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/transform/TransformAggregation.scala index 17ee7980b..9701c7865 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/transform/TransformAggregation.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/transform/TransformAggregation.scala @@ -21,6 +21,8 @@ import app.softnetwork.elastic.sql.schema.{mapper, sqlConfig} import app.softnetwork.elastic.sql.{DdlToken, Identifier} import com.fasterxml.jackson.databind.JsonNode +import scala.collection.immutable.ListMap + sealed trait TransformAggregation extends DdlToken { def name: String @@ -66,6 +68,51 @@ case class CardinalityTransformAggregation(field: String) extends TransformAggre override def sql: String = s"COUNT(DISTINCT $field)" } +/** The per-group calculation of a SELECT item that is arithmetic over aggregates (`MAX(a) - MIN(b) + * AS d`), as a `bucket_script` in a materialized view's pivot -- the same calculation a search + * runs for it (`SingleSearch.transformBucketScripts`). + * + * It reads its operands from sibling aggregations of the pivot BY NAME: `bucketsPath` maps each + * parameter the script reads (`params.`) to the name of the pivot aggregation that computes + * it. The aggregation itself is named after the SELECT alias, so a view's HAVING over `d` reads it + * like any other metric. + * + * @param expression + * the SQL expression it computes, for display + * @param params + * the script parameters the script reads that are NOT bucket paths, in the order it first reads + * them -- `__now__`, the request clock `CURRENT_DATE` / `NOW()` render against. The pivot must + * bind each one ([[node]] carries none): every other parameter the script reads is a key of + * `bucketsPath`. + */ +case class BucketScriptTransformAggregation( + expression: String, + bucketsPath: ListMap[String, String], + script: String, + params: Seq[String] +) extends TransformAggregation { + override def name: String = "bucket_script" + + /** The pivot aggregations it reads. */ + override def field: String = bucketsPath.values.mkString(", ") + + override def sql: String = expression + + override def node: JsonNode = { + val node = mapper.createObjectNode() + val bucketScriptNode = mapper.createObjectNode() + val bucketsPathNode = mapper.createObjectNode() + bucketsPath.foreach { case (param, aggregation) => + bucketsPathNode.put(param, aggregation) + () + } + bucketScriptNode.set("buckets_path", bucketsPathNode) + bucketScriptNode.put("script", script) + node.set(name, bucketScriptNode) + node + } +} + case class TopHitsTransformAggregation( fields: Seq[String], size: Int = 1, diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingAliasResolutionSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingAliasResolutionSpec.scala new file mode 100644 index 000000000..2adff4078 --- /dev/null +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingAliasResolutionSpec.scala @@ -0,0 +1,304 @@ +/* + * Copyright 2025 SOFTNETWORK + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package app.softnetwork.elastic.sql.query + +import app.softnetwork.elastic.sql.function.aggregate.{MaxAgg, MinAgg} +import app.softnetwork.elastic.sql.parser.Parser +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** The three spellings of an aggregate in a `HAVING` -- bare `MAX(x)`, qualified `MAX(t.x)` and the + * SELECT alias `mx` -- are ONE shape: one verdict, one message, one filter. + * + * `Having.resolveAggregateAliases` replaces a SELECT alias by its aggregate at EVERY depth of a + * value expression -- a function argument, an arithmetic operand, a `CASE` condition, the value + * side of a comparison -- through the one walk that reaches all of them, `update`. And `ISNULL` / + * `ISNOTNULL` parse their operand like any value expression, so an aggregate there is the + * aggregate every other position builds. + * + * The whole function family is measured over the help-corpus population outside this suite; these + * rows pin the mechanism on every operand position the walk has to reach. + */ +class HavingAliasResolutionSpec extends AnyFlatSpec with Matchers { + + private def outcome(sql: String): Either[String, Option[String]] = Parser(sql) match { + case Right(s: SingleSearch) => + Right(s.having.flatMap(_.criteria).flatMap(MetricSelectorScript.nullAwareSelectorScript)) + case Right(other) => fail(s"[$sql] expected a SingleSearch, got $other") + case Left(e) => Left(e.msg) + } + + private def bare(having: String): String = + "SELECT g, MAX(x) AS mx, MIN(y) AS my FROM t GROUP BY g HAVING " + + having.replace("{A}", "MAX(x)").replace("{B}", "MIN(y)") + + private def qualified(having: String): String = + "SELECT r.g, MAX(r.x) AS mx, MIN(r.y) AS my FROM t AS r GROUP BY r.g HAVING " + + having.replace("{A}", "MAX(r.x)").replace("{B}", "MIN(r.y)") + + private def aliased(having: String): String = + "SELECT g, MAX(x) AS mx, MIN(y) AS my FROM t GROUP BY g HAVING " + + having.replace("{A}", "mx").replace("{B}", "my") + + /** Every operand position the substitution has to reach, accepted and refused shapes alike. */ + private val shapes: Seq[String] = Seq( + "{A} > 1", + "{A} > {B}", + "{A} BETWEEN 1 AND 5", + "{A} IN (1, 2)", + "NOT {A} = 1", + "COALESCE({A}, {B}) > 1", + "GREATEST({A}, {B}, 3) > 1", + "LEAST({A}, 1) > 0", + "SIGN({A}) > 0", + "{A} > COALESCE({B}, 1)", + "{A} > 1 AND COALESCE({B}, 0) > 1", + "COALESCE(COALESCE({A}, {B}), 5) > 1", + "NULLIF({A}, 0) > 1", + "ABS({A}) > 1", + "ROUND({A}, 2) > 1", + "CAST({A} AS DOUBLE) > 1", + "{A} - {B} > 1", + "{A} + 1 > 2", + "CASE WHEN {A} > 1 THEN 1 ELSE 0 END = 1", + "ISNULL({A})", + "ISNOTNULL({A})" + ) + + "the bare and the alias spellings" should "get the same verdict, message and filter" in { + var accepted, refused = 0 + shapes.foreach { shape => + withClue(s"[$shape] ") { + val b = outcome(bare(shape)) + outcome(aliased(shape)) shouldBe b + if (b.isRight) accepted += 1 else refused += 1 + } + } + // both outcomes occur, or the equality proved nothing + accepted should be > 5 + refused should be > 5 + } + + "the qualified spelling" should "get the same verdict and filter, and the message in its own spelling" in { + shapes.foreach { shape => + withClue(s"[$shape] ") { + outcome(qualified(shape)).left.map(_.replace("r.", "")) shouldBe outcome(bare(shape)) + } + } + } + + "ISNULL and ISNOTNULL" should "parse their operand as the aggregate every other position builds" in { + def nullTestOperand(sql: String) = Parser(sql) match { + case Right(s: SingleSearch) => + s.having.flatMap(_.criteria) match { + case Some(c: IsNullCriteria) => c.identifier.aggregateFunction + case Some(c: IsNotNullCriteria) => c.identifier.aggregateFunction + case other => fail(s"[$sql] expected a null test, got $other") + } + case other => fail(s"[$sql] expected a SingleSearch, got $other") + } + nullTestOperand(bare("ISNULL({B})")).get shouldBe a[MinAgg] + nullTestOperand(bare("ISNOTNULL({A})")).get shouldBe a[MaxAgg] + nullTestOperand(qualified("ISNULL({B})")).get shouldBe a[MinAgg] + } + + it should "no longer mistake the qualified spelling for a different aggregate" in { + // The bare `MIN` token rendered `MIN(y)` where the SELECT's `MinAgg` renders `MIN(r.y)`, so the + // alias `my` looked like it named two aggregates and the statement was refused. + outcome(qualified("ISNULL({B})")) shouldBe outcome(aliased("ISNULL({B})")) + outcome(qualified("ISNOTNULL({B})")) shouldBe outcome(aliased("ISNOTNULL({B})")) + } + + "DATEDIFF, DATE_DIFF and TIMESTAMPDIFF" should "parse an aggregate operand as the aggregate every other position builds" in { + // The remedy for inline arithmetic -- alias it in SELECT -- was refused on the qualified + // spelling: the bare `MAX` token rendered `MAX(d)` where the SELECT's `MaxAgg` renders + // `MAX(r.d)`, so the alias `max_d` looked like it named two aggregates. + Seq( + "DATEDIFF(MAX(r.d), MIN(r.d))", + "DATEDIFF(MAX(r.d), '2024-01-01')", + "DATE_DIFF(MAX(r.d), MIN(r.d), DAY)", + "DATE_DIFF(DAY, MAX(r.d), '2024-01-01')", + "TIMESTAMPDIFF(DAY, MAX(r.d), MIN(r.d))" + ).foreach { expr => + val sql = "SELECT r.g, MAX(r.d) AS max_d, MIN(r.d) AS min_d, " + + s"$expr AS x FROM t AS r GROUP BY r.g HAVING x > 1" + withClue(s"[$sql] ") { + Parser(sql) match { + case Right(s: SingleSearch) => + val item = s.select.fields.find(_.fieldAlias.exists(_.alias == "x")).get + val operands = item.identifier.referencedAggregates.flatMap(_.aggregateFunction) + operands should not be empty + operands.foreach { af => + withClue(s"[$af] ")( + (af.isInstanceOf[MaxAgg] || af.isInstanceOf[MinAgg]) shouldBe true + ) + } + case other => fail(s"expected a SingleSearch, got $other") + } + } + } + } + + "a cast or an interval after an aggregate operand" should "keep the verdict and the message it had" in { + // The aggregate reading stops at the call's `)`: there the alternatives these five functions + // read before read the operand, suffix included, exactly as they did -- not `end of input + // expected`. + val chain = Left("Aggregation function must be the first function in the chain") + def inline(expr: String) = Left( + s"HAVING cannot combine aggregates arithmetically inline ($expr); alias the expression in " + + "SELECT and reference the alias" + ) + val having = "SELECT g, MAX(a) AS ma, MAX(d) AS md FROM t GROUP BY g HAVING " + Seq( + having + "ISNULL(MAX(a)::DOUBLE)" -> chain, + having + "ISNOTNULL(MAX(a)::BIGINT)" -> chain, + having + "ISNULL(MAX(d) - INTERVAL 1 DAY)" -> chain, + having + "ISNOTNULL(MAX(d) + INTERVAL 1 DAY)" -> chain, + having + "ISNULL(COUNT(*)::DOUBLE)" -> chain, + having + "DATEDIFF(MAX(d)::DATE, '2024-01-01') > 1" -> inline( + "DATEDIFF(MAX(d)::DATE, '2024-01-01')" + ), + having + "DATEDIFF(MAX(d) - INTERVAL 1 DAY, '2024-01-01') > 1" -> inline( + "DATEDIFF(MAX(d) - INTERVAL 1 DAY, '2024-01-01')" + ), + having + "DATE_DIFF(DAY, MAX(d) - INTERVAL 1 DAY, '2024-01-01') > 1" -> inline( + "DATE_DIFF(DAY, MAX(d) - INTERVAL 1 DAY, '2024-01-01')" + ), + having + "TIMESTAMPDIFF(DAY, MAX(d)::DATE, '2024-01-01') > 1" -> inline( + "DATE_DIFF(DAY, MAX(d)::DATE, '2024-01-01')" + ), + "SELECT g, ISNULL(MAX(a)::DOUBLE) AS x FROM t GROUP BY g" -> chain, + "SELECT g, ISNULL(MAX(d) - INTERVAL 1 DAY) AS x FROM t GROUP BY g" -> chain, + "SELECT g, DATEDIFF(MAX(d)::DATE, '2024-01-01') AS x FROM t GROUP BY g" -> Right(()), + "SELECT g, DATEDIFF(MAX(d) - INTERVAL 1 DAY, CURRENT_DATE) AS x FROM t GROUP BY g" -> Right( + () + ) + ).foreach { case (sql, expected) => + withClue(s"[$sql] ")(Parser(sql).map(_ => ()).left.map(_.msg) shouldBe expected) + } + } + + "a window function operand of DATEDIFF, DATE_DIFF or TIMESTAMPDIFF" should + "be refused by name with no GROUP BY, and its remedy accepted" in { + // (the statement, the call as written, the window function as written, as rendered) + Seq( + ( + "SELECT g, DATEDIFF(MAX(d) OVER (PARTITION BY g), '2024-01-01') AS x FROM t", + "DATEDIFF(MAX(d) OVER (PARTITION BY g), '2024-01-01')", + "MAX(d) OVER (PARTITION BY g)", + "MAX(d) OVER (PARTITION BY g)" + ), + ( + "SELECT g, DATE_DIFF(DAY, '2024-01-01', MIN(ts) OVER (PARTITION BY g)) AS x FROM t", + "DATE_DIFF(DAY, '2024-01-01', MIN(ts) OVER (PARTITION BY g))", + "MIN(ts) OVER (PARTITION BY g)", + "MIN(ts) OVER (PARTITION BY g)" + ), + ( + "SELECT id, g, d, DATEDIFF(d, MIN(d) OVER (PARTITION BY g)) AS x FROM t", + "DATEDIFF(d, MIN(d) OVER (PARTITION BY g))", + "MIN(d) OVER (PARTITION BY g)", + "MIN(d) OVER (PARTITION BY g)" + ), + ( + "SELECT id, TIMESTAMPDIFF(DAY, FIRST_VALUE(d) OVER (PARTITION BY g ORDER BY d), d) AS x FROM t", + "TIMESTAMPDIFF(DAY, FIRST_VALUE(d) OVER (PARTITION BY g ORDER BY d), d)", + "FIRST_VALUE(d) OVER (PARTITION BY g ORDER BY d)", + "FIRST_VALUE(d) OVER (PARTITION BY g ORDER BY d ASC)" + ) + ).foreach { case (sql, call, window, rendered) => + withClue(s"[$sql] ") { + val message = Parser(sql).left.map(_.msg).left.toOption.getOrElse(fail("not refused")) + message should startWith( + s"DATEDIFF, DATE_DIFF and TIMESTAMPDIFF over a window function are not supported yet ($rendered in " + ) + message should endWith("); select the window function on its own") + Parser(sql.replace(call, window)).isRight shouldBe true + } + } + // With a GROUP BY it is computed per group, as it was. + Seq( + "SELECT g, DATEDIFF(MAX(d) OVER (), '2024-01-01') AS x FROM t GROUP BY g", + "SELECT g, DATEDIFF(MAX(d) OVER (PARTITION BY g), '2024-01-01') AS x FROM t GROUP BY g", + "SELECT g, DATEDIFF('2024-01-01', MAX(d) OVER (), DAY) AS x FROM t GROUP BY g", + "SELECT g, MAX(d) AS m, DATEDIFF(MAX(d) OVER (), '2024-01-01') AS x FROM t GROUP BY g" + ).foreach(sql => withClue(s"[$sql] ")(Parser(sql).isRight shouldBe true)) + } + + private def overBucketScript(having: String): String = + "SELECT g, MAX(x) AS mx, MIN(y) AS my, MAX(x) - MIN(y) AS d FROM t GROUP BY g HAVING " + having + + "a SELECT bucket_script alias" should "be read under a function that is NULL whenever it is" in { + Seq("SIGN(d) > 0", "mx > SIGN(d)", "SIGN(d) BETWEEN -1 AND 0").foreach { having => + withClue(s"[$having] ")(outcome(overBucketScript(having)).isRight shouldBe true) + } + } + + it should "stay refused under COALESCE, GREATEST, LEAST and a conversion, as before" in { + // Inside a NULL-deciding function the item is read through its OPERANDS, and an operand's NULL + // throws there instead of making the item NULL; under a conversion of the alias the comparison + // reads `params.d` and drops the conversion, and every other position reads undeclared + // operands. So the alias is left in place and keeps the refusal it always had. + Seq( + "COALESCE(d, 0) > 1" -> "(COALESCE(d, 0))", + "GREATEST(d, 1) > 1" -> "(GREATEST(d, 1))", + "LEAST(d, my) < 3" -> "(LEAST(d, my))", + "SIGN(COALESCE(d, 0)) > 0" -> "(SIGN(COALESCE(d, 0)))", + "COALESCE(SIGN(d), 0) = 0" -> "(COALESCE(SIGN(d), 0))", + "CAST(d AS DOUBLE) > 1" -> "(CAST(d AS DOUBLE))", + "CAST(d AS BIGINT) > 1" -> "(CAST(d AS BIGINT))", + "d::DOUBLE > 1" -> "(d::DOUBLE)", + "TRY_CAST(d AS DOUBLE) > 1" -> "(TRY_CAST(d AS DOUBLE))", + "ISNULL(CAST(d AS DOUBLE))" -> "(CAST(d AS DOUBLE))", + "SIGN(CAST(d AS DOUBLE)) > 0" -> "(SIGN(CAST(d AS DOUBLE)))" + ).foreach { case (having, operand) => + withClue(s"[$having] ") { + outcome(overBucketScript(having)).left.toOption.get shouldBe + s"HAVING cannot apply a function to the aggregate alias 'd' $operand; compare the aggregate itself" + } + } + // beside an aggregate, the refusals they had + outcome(overBucketScript("mx > COALESCE(d, 1)")).left.toOption.get should include( + "HAVING cannot be applied to MAX(x) > COALESCE(d, 1): its rendering reads 'arg0'" + ) + outcome(overBucketScript("COUNT(*) > CAST(d AS DOUBLE)")).left.toOption.get should include( + "HAVING cannot be applied to COUNT(*) > CAST(d AS DOUBLE): its rendering can evaluate to NULL" + ) + } + + "an alias named after its aggregate's own column" should "not be substituted into its operand" in { + // `MAX(x) AS x`: the `x` inside `MAX(x)` is the column, never the alias. + val sql = "SELECT g, MAX(x) AS x FROM t GROUP BY g HAVING x > 1 AND ABS(x) > 1" + outcome(sql).left.toOption.get should include("ABS(MAX(x)) > 1") + outcome("SELECT g, MAX(x) AS x FROM t GROUP BY g HAVING x > 1") shouldBe + outcome("SELECT g, MAX(x) AS x FROM t GROUP BY g HAVING MAX(x) > 1") + } + + "an alias the substitution leaves in place" should "still be refused by name inside a function" in { + // Relation predicates are not substituted; a function of an alias there stays a bare column, + // and the #389 rule names it rather than letting it render an unbound parameter. + outcome(aliased("NESTED(ABS({A}) > 1)")).left.toOption.get should include( + "HAVING cannot apply a function to the aggregate alias 'mx'" + ) + } + + "a MATCH over an alias" should "keep its own message" in { + outcome(aliased("MATCH ({A}) AGAINST ('z')")).left.toOption.get should include( + "the aggregate mx (MAX(x))" + ) + } +} diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingNullAwareSelectorSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingNullAwareSelectorSpec.scala index 8a2ba0abd..a9d778793 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingNullAwareSelectorSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingNullAwareSelectorSpec.scala @@ -85,11 +85,27 @@ class HavingNullAwareSelectorSpec extends AnyFlatSpec with Matchers with OptionV it should "leave a function of the aggregate its own NULL handling" in { // COALESCE decides what a NULL means: over a group with no value it answers 5, and 5 > 1. script(group + "COALESCE(MAX(v), 5) > 1") shouldBe s"($maxV != null ? $maxV : 5) > 1" - // GREATEST over a NULL aggregate is UNKNOWN in a HAVING, as before: the guard is the - // rendering's own. The cast keeps the read `def`: `Math.max(, 0)` does not - // compile otherwise (`Cannot cast null to a primitive type [double]`). + // GREATEST skips a NULL argument, as the docs state and WHERE does: over a group with no value + // it answers 0, and 0 > 1 is false. The cast keeps the read `def`: `Math.max(, + // 0)` does not compile otherwise (`Cannot cast null to a primitive type [double]`). script(group + "GREATEST(MAX(v), 0) > 1") shouldBe - s"($maxV == null ? false : (Math.max($maxV, 0) > 1))" + s"($maxV == null ? 0 : Math.max($maxV, 0)) > 1" + } + + it should "skip a NULL argument of GREATEST / LEAST, and guard them on their result" in { + val a = read("max_a") + val b = read("min_b") + def reduced(fn: String) = s"($a == null ? $b : ($b == null ? $a : Math.$fn($a, $b)))" + // UNKNOWN only when every argument is NULL: a group with `a` and no `b` compares its `MAX(a)` + script(group + "GREATEST(MAX(a), MIN(b)) > 1") shouldBe + s"(${reduced("max")} == null ? false : (${reduced("max")} > 1))" + script(group + "LEAST(MAX(a), MIN(b)) < 3") shouldBe + s"(${reduced("min")} == null ? false : (${reduced("min")} < 3))" + // a literal argument: never NULL, so no guard at all + script(group + "LEAST(MIN(b), 1) < 3") shouldBe s"($b == null ? 1 : Math.min($b, 1)) < 3" + // on the right of an aggregate, whose own guard stays + script(group + "MAX(a) > GREATEST(MIN(b), 1)") shouldBe + s"($a == null ? false : ($a > ($b == null ? 1 : Math.max($b, 1))))" } it should "guard a COALESCE on its result, never on its arguments" in { diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala index 267a335ed..49a78c8f4 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingOverAggregateFunctionSpec.scala @@ -140,15 +140,16 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt // compile in the bucket-pipeline script context -- see the disqualifier tests below. // ------------------------------------------------------------------------------------------- - "a null-PROPAGATING function of an aggregate" should "render guarded, exactly as a bare aggregate does" in { - // SQL says `GREATEST(NULL, 0) > 1` is UNKNOWN, so the group is excluded -- which is the `false` - // this guard supplies. Without it `Math.max(null, 0)` fails the whole search. + "a NULL-skipping function of an aggregate" should "skip a NULL argument, never guard on it" in { + // `GREATEST` / `LEAST` skip a NULL argument -- the docs, and WHERE: `GREATEST(NULL, 0)` is 0, + // never UNKNOWN. With a literal argument the value is never NULL, so nothing is guarded; and + // the skip is what keeps `Math.max(null, 0)` from failing the whole search. script(group + "GREATEST(COUNT(*), 0) > 1") shouldBe - "(params.c == null ? false : (Math.max(params.c, 0) > 1))" + "(params.c == null ? 0 : Math.max(params.c, 0)) > 1" script(group + "LEAST(COUNT(*), 99) > 1") shouldBe - "(params.c == null ? false : (Math.min(params.c, 99) > 1))" + "(params.c == null ? 99 : Math.min(params.c, 99)) > 1" script(group + "1 < GREATEST(COUNT(*), 0)") shouldBe - "(params.c == null ? false : (1 < Math.max(params.c, 0)))" + "1 < (params.c == null ? 0 : Math.max(params.c, 0))" } "a null-ABSORBING function of an aggregate" should "render UNguarded" in { @@ -159,14 +160,16 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt "BETWEEN and IN over a function of an aggregate" should "render as one guarded boolean" in { script(group + "GREATEST(COUNT(*), 0) BETWEEN 1 AND 5") shouldBe - "(params.c == null ? false : ((Math.max(params.c, 0) >= 1 && Math.max(params.c, 0) <= 5)))" + "((params.c == null ? 0 : Math.max(params.c, 0)) >= 1 && " + + "(params.c == null ? 0 : Math.max(params.c, 0)) <= 5)" script(group + "GREATEST(COUNT(*), 0) IN (1, 2)") shouldBe - "(params.c == null ? false : ((Math.max(params.c, 0) == 1 || Math.max(params.c, 0) == 2)))" + "((params.c == null ? 0 : Math.max(params.c, 0)) == 1 || " + + "(params.c == null ? 0 : Math.max(params.c, 0)) == 2)" } "NOT over a function of an aggregate" should "push the negation into the comparison" in { script(group + "NOT GREATEST(COUNT(*), 0) > 1") shouldBe - "(params.c == null ? false : (Math.max(params.c, 0) <= 1))" + "(params.c == null ? 0 : Math.max(params.c, 0)) <= 1" } "a conjunction of two expressible predicates" should "emit BOTH" in { @@ -174,14 +177,14 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt // wrong -- a partial filter is a wrong answer, not a lesser fix. script(group + "COUNT(*) > 1 AND GREATEST(COUNT(*), 0) > 2") shouldBe "(params.c == null ? false : (params.c > 1)) && " + - "(params.c == null ? false : (Math.max(params.c, 0) > 2))" + "(params.c == null ? 0 : Math.max(params.c, 0)) > 2" } "a disjunction of two expressible predicates" should "emit BOTH" in { // The OR form is WORSE than the AND form when a branch is dropped: the surviving filter is // STRICTER than what was written, so rows silently disappear. script(group + "GREATEST(COUNT(*), 0) > 1 OR COUNT(*) > 5") shouldBe - "(params.c == null ? false : (Math.max(params.c, 0) > 1)) || " + + "(params.c == null ? 0 : Math.max(params.c, 0)) > 1 || " + "(params.c == null ? false : (params.c > 5))" } @@ -200,8 +203,7 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt parsed(sql).sqlAggregations.keys.toList shouldBe List("c", "max_x") having(sql).extractAllMetricsPath shouldBe Map("c" -> "c", "max_x" -> "max_x") script(sql) shouldBe - "(params.c == null || params.max_x == null ? false : " + - "(params.c > Math.max(params.max_x, 0)))" + "(params.c == null ? false : (params.c > (params.max_x == null ? 0 : Math.max(params.max_x, 0))))" } // ------------------------------------------------------------------------------------------- @@ -256,12 +258,14 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt ) } - "an aggregate ALIAS used inside a function argument" should "be refused by name" in { - // `Having.resolveAggregateAliases` substitutes an OPERAND only, so `c` inside `NULLIF(c, 0)` - // stays a bare column and renders `arg0`, a context parameter nothing binds. - val msg = rejection(group + "NULLIF(c, 0) > 1") - msg should include("aggregate alias 'c'") - msg should include("NULLIF(c, 0)") + "an aggregate ALIAS used inside a function argument" should "be read as the aggregate it names" in { + // `Having.resolveAggregateAliases` substitutes an alias at EVERY depth, so `c` inside + // `NULLIF(c, 0)` is `COUNT(*)`: the alias spelling gets the bare spelling's verdict and message + // -- here the honest refusal of a rendering that can evaluate to NULL. It used to stay a bare + // column rendering `arg0`, a context parameter nothing binds, refused by name. + rejection(group + "NULLIF(c, 0) > 1") shouldBe rejection(group + "NULLIF(COUNT(*), 0) > 1") + rejection(group + "NULLIF(c, 0) > 1") should include("can evaluate to NULL") + script(group + "COALESCE(c, 0) > 1") shouldBe script(group + "COALESCE(COUNT(*), 0) > 1") } "a conjunction mixing an expressible and an un-expressible predicate" should "refuse the whole statement" in { @@ -279,7 +283,7 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt "the whole-table HAVING" should "follow the same rule" in { script("SELECT COUNT(*) AS c FROM t HAVING GREATEST(COUNT(*), 0) > 1") shouldBe - "(params.c == null ? false : (Math.max(params.c, 0) > 1))" + "(params.c == null ? 0 : Math.max(params.c, 0)) > 1" rejection("SELECT COUNT(*) AS c FROM t HAVING NULLIF(COUNT(*), 0) > 1") should include( "can evaluate to NULL" ) @@ -290,7 +294,7 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt "HAVING GREATEST(COUNT(e.address), 0) > 1" parsed(sql).sqlAggregations.keys.toList shouldBe List("e.filtered_agg.count_e_address") script(sql) shouldBe - "(params.count_e_address == null ? false : (Math.max(params.count_e_address, 0) > 1))" + "(params.count_e_address == null ? 0 : Math.max(params.count_e_address, 0)) > 1" } it should "be REFUSED inside the relation too, not only at the flat level" in { diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/MaterializedViewHavingSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/MaterializedViewHavingSpec.scala index 5e4add4a8..6b214a87b 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/MaterializedViewHavingSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/MaterializedViewHavingSpec.scala @@ -21,14 +21,13 @@ import org.scalatest.matchers.should.Matchers * `AggregateConversion.toTransformAggregation` answers `None`, while * `Stage.extractAggregatePaths` still names it: a `buckets_path` pointing at an aggregation * that does not exist; - * - a SELECT `bucket_script` alias (`MAX(x) - MIN(x) AS d`) -- a transform's pivot has no + * - a SELECT `bucket_script` alias (`MAX(x) - MIN(x) AS d`) -- a transform's pivot had no * `bucket_script` channel and such an item is not an aggregate, so `buckets_path` came back - * EMPTY and the clause was dropped; - * - an aggregate on the RIGHT of a comparison -- `Stage.extractAggregatePaths` walks - * `expr.identifier` ONLY and never `expr.maybeValue`, while core's own - * `Expression.extractAllMetricsPath` walks BOTH, so publishing it puts it in - * `buildAggregations` and NEVER in `buckets_path`: the selector IS built, reads a null - * parameter, the guard short-circuits and EVERY bucket is rejected -- an EMPTY view at 200; + * EMPTY and the clause was dropped. A view now computes the item with the `bucket_script` of + * `SingleSearch.transformBucketScripts`; what that channel does not serve stays refused; + * - an aggregate on the RIGHT of a comparison -- the view's filter declared only the left-hand + * metric of each comparison. It now declares `Having.metricNames`, every metric the clause + * reads, so two aggregates in one condition are checked like one; * - an aggregate a transform COULD compute but the SELECT list does not publish -- the selector * reads a metric the view never creates: `buckets_path` empty (dropped) or, beside a published * metric, PARTIAL, so the script reads a `params.*` the path never declares. @@ -62,8 +61,8 @@ class MaterializedViewHavingSpec extends AnyFlatSpec with Matchers { val GroupKey = "grouping key" val NoTransformAgg = "computes only MIN, MAX, SUM, AVG" val BucketScript = "expression over aggregates" - val TwoAggregates = "compare two aggregates" val Unpublished = "to the SELECT list" + val Relation = "nested, child or parent predicate" val NoMatchingAgg = "matches none of the aggregations this view creates" val all: Seq[String] = Seq( @@ -71,8 +70,8 @@ class MaterializedViewHavingSpec extends AnyFlatSpec with Matchers { GroupKey, NoTransformAgg, BucketScript, - TwoAggregates, Unpublished, + Relation, NoMatchingAgg ) } @@ -262,11 +261,42 @@ class MaterializedViewHavingSpec extends AnyFlatSpec with Matchers { "SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING STDDEV(amount) > 1", Some(Rule.NoTransformAgg) ), + // A SELECT `bucket_script` item is computed by the view's own `bucket_script` + // (`SingleSearch.transformBucketScripts`), so a HAVING over its alias reads it like any metric + // -- whether or not the SELECT list publishes its operands. Cell( "N7 bucket_script alias", "SELECT city, MAX(amount) - MIN(amount) AS d FROM customers GROUP BY city HAVING d > 3", + None + ), + Cell( + "N7b bucket_script alias, operands published", + "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn, MAX(amount) - MIN(amount) AS d " + + "FROM customers GROUP BY city HAVING d > 3", + None + ), + // …and what that channel does not serve: an operand no transform computes, and a null test + // over the alias, whose rendering reads the operands rather than `d`. + Cell( + "N7c bucket_script over an aggregate no transform computes", + "SELECT city, STDDEV(amount) - MIN(amount) AS d FROM customers GROUP BY city HAVING d > 3", + Some(Rule.BucketScript) + ), + Cell( + "N7d null test over a bucket_script alias", + "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn, MAX(amount) - MIN(amount) AS d " + + "FROM customers GROUP BY city HAVING ISNULL(d)", Some(Rule.BucketScript) ), + // A function of the alias reads the item's operands (`SIGN(d)`), which the view creates once the + // SELECT list publishes them. A conversion of it (`CAST(d AS DOUBLE)`) is refused in both venues + // by the search rules (`HavingAliasResolutionSpec`). + Cell( + "N7e a function of the alias, its operands published", + "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn, MAX(amount) - MIN(amount) AS d " + + "FROM customers GROUP BY city HAVING SIGN(d) > 0", + None + ), // πŸ”΄ The non-over-reach row: an aggregate no transform can compute is fine in the SELECT list // as long as the HAVING does not read it. Without this, rule 3 could refuse the whole family. Cell( @@ -284,70 +314,59 @@ class MaterializedViewHavingSpec extends AnyFlatSpec with Matchers { "SELECT city, COUNT(DISTINCT id) AS cd FROM customers GROUP BY city HAVING COUNT(DISTINCT id) > 1", None ), - // (R) an aggregate on the RIGHT of the comparison. πŸ”΄ The population had NO row comparing two - // aggregates -- every earlier cell is ` ` or ` ` -- which - // is why a whole rule could be added without one assertion moving. - // - // extensions' `Stage.extractAggregatePaths` walks `expr.identifier` ONLY and never - // `expr.maybeValue`, while core's own `Expression.extractAllMetricsPath` walks BOTH. So - // publishing the right-hand aggregate puts it in `buildAggregations` and NEVER in - // `buckets_path`. MEASURED: the selector IS built, reads `params.` which is null, the - // null guard short-circuits, and EVERY bucket is rejected -- an EMPTY view at HTTP 200. + // (R) an aggregate on the RIGHT of the comparison. The view's filter declared only the + // left-hand metric of each comparison, so the right-hand one was read as an undeclared + // parameter and every group was rejected (an EMPTY view at HTTP 200); the old rule 5 refused + // the shape. The filter now declares `Having.metricNames` -- every metric the clause reads -- so + // these deploy, and an unpublished right-hand aggregate gets rule 6's remedy like any other. Cell( "R1 MAX > MIN, both published", "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city " + "HAVING MAX(amount) > MIN(amount)", - Some(Rule.TwoAggregates) + None ), Cell( "R2 same, alias spelling", "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city HAVING mx > mn", - Some(Rule.TwoAggregates) + None ), Cell( "R3 reversed operand order", "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city " + "HAVING MIN(amount) < MAX(amount)", - Some(Rule.TwoAggregates) + None ), Cell( "R4 inequality", "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city " + "HAVING MAX(amount) <> MIN(amount)", - Some(Rule.TwoAggregates) + None ), Cell( "R5 SUM > AVG", "SELECT city, SUM(amount) AS sm, AVG(amount) AS av FROM customers GROUP BY city " + "HAVING SUM(amount) > AVG(amount)", - Some(Rule.TwoAggregates) + None ), Cell( "R6 beside a well-formed metric, AND", "SELECT city, COUNT(*) AS c, MAX(amount) AS mx, MIN(amount) AS mn FROM customers " + "GROUP BY city HAVING COUNT(*) > 1 AND MAX(amount) > MIN(amount)", - Some(Rule.TwoAggregates) + None ), Cell( "R7 beside a well-formed metric, OR", "SELECT city, COUNT(*) AS c, MAX(amount) AS mx, MIN(amount) AS mn FROM customers " + "GROUP BY city HAVING COUNT(*) > 1 OR MAX(amount) > MIN(amount)", - Some(Rule.TwoAggregates) + None ), - // πŸ”΄ The other direction: the right-hand aggregate is NOT published either. Both rules apply; - // the ordering test below pins which one speaks, and it must be this one -- rule 5's remedy - // ("add it to the SELECT list") is MEASURED to produce R1. Cell( "R8 right-hand aggregate NOT published", "SELECT city, MAX(amount) AS mx FROM customers GROUP BY city HAVING MAX(amount) > MIN(amount)", - Some(Rule.TwoAggregates) + Some(Rule.Unpublished) ), - // (V) πŸ”΄ A right-hand aggregate is undeclared ONLY when its name appears on no leaf's LEFT - // side anywhere in the clause -- `Stage.extractAggregatePaths` declares a parameter for the - // left identifier of EVERY leaf in the whole tree. A sibling conjunct that names the same - // aggregate therefore declares it, and the transform deploys and runs correctly. - // MEASURED on the control: UNDECLARED = none, selector built, both aggregations created. - // These MUST stay accepted; refusing them is a regression against main. + // (V) a sibling conjunct naming the right-hand aggregate on its left -- accepted before, and + // still: the filter declares every metric the clause reads. Cell( "V1 sibling conjunct declares it, AND", "SELECT city, SUM(amount) AS m, MAX(amount) AS m2 FROM customers GROUP BY city " + @@ -396,25 +415,9 @@ class MaterializedViewHavingSpec extends AnyFlatSpec with Matchers { "SELECT city, MAX(amount) AS m2 FROM customers GROUP BY city HAVING m2 > m2", None ), - // …and the negatives that keep rule 5 alive: nothing declares the right-hand name. - Cell( - "V9 no sibling declares it", - "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city " + - "HAVING MAX(amount) > MIN(amount)", - Some(Rule.TwoAggregates) - ), - Cell( - "V10 right-hand aggregate not published at all", - "SELECT city, MAX(amount) AS mx FROM customers GROUP BY city HAVING MAX(amount) > MIN(amount)", - Some(Rule.TwoAggregates) - ), - // (W) πŸ”΄ Which of rule 5 and rule 6 speaks when BOTH apply -- the right-hand aggregate is - // undeclared AND the SELECT list does not publish it. The discriminator is the COUNTERFACTUAL: - // would publishing it make the selector declare it? `Stage.extractAggregatePaths` declares the - // LEFT identifier of EVERY leaf, so the answer is yes exactly when some leaf names it on the - // left. When it does, rule 6's remedy ends the journey (W5 / W8 are W1 / W3 with the remedy - // applied literally, and they are ACCEPTED); when nothing names it on the left, publishing it - // lands on V9 and rule 5 must speak instead (W6, R8/V10 above). + // (W) an unpublished aggregate anywhere in the condition -- on the left, on the right, named by + // a sibling or not -- is rule 6's, and its remedy ends the journey (W5 / W8 are W1 / W3 with + // the remedy applied literally, and they are ACCEPTED). Cell( "W1 a sibling leaf names it on the left, unpublished", "SELECT city, MAX(amount) AS mx FROM customers GROUP BY city " + @@ -445,16 +448,16 @@ class MaterializedViewHavingSpec extends AnyFlatSpec with Matchers { None ), Cell( - "W6 nothing names it on the left: publishing it would not help", + "W6 nothing names it on the left", "SELECT city, MAX(amount) AS mx FROM customers GROUP BY city " + "HAVING MIN(amount) > 1 AND MIN(amount) > MAX(amount)", - Some(Rule.TwoAggregates) + Some(Rule.Unpublished) ), Cell( "W7 both published, alias spelling on the right", "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city " + "HAVING MAX(amount) > mn", - Some(Rule.TwoAggregates) + None ), Cell( "W8 W3 with rule 6's remedy applied literally", @@ -462,6 +465,71 @@ class MaterializedViewHavingSpec extends AnyFlatSpec with Matchers { "HAVING MIN(amount) > MIN(amount)", None ), + // (M) several aggregates through a FUNCTION, in every spelling: the filter reads each of them, + // so each must be published -- and then the view deploys. + Cell( + "M1 COALESCE of two aggregates", + "SELECT city, MAX(amount) AS mx, MIN(qty) AS mq FROM customers GROUP BY city " + + "HAVING COALESCE(MAX(amount), MIN(qty)) > 1", + None + ), + Cell( + "M2 same, alias spelling", + "SELECT city, MAX(amount) AS mx, MIN(qty) AS mq FROM customers GROUP BY city " + + "HAVING COALESCE(mx, mq) > 1", + None + ), + Cell( + "M3 GREATEST of two aggregates and a literal", + "SELECT city, MAX(amount) AS mx, MIN(qty) AS mq FROM customers GROUP BY city " + + "HAVING GREATEST(MAX(amount), MIN(qty), 3) > 1", + None + ), + Cell( + "M4 a function of an aggregate on the value side", + "SELECT city, MAX(amount) AS mx, MIN(qty) AS mq FROM customers GROUP BY city " + + "HAVING MAX(amount) > COALESCE(MIN(qty), 1)", + None + ), + Cell( + "M5 one aggregate and a literal", + "SELECT city, MIN(qty) AS mq FROM customers GROUP BY city HAVING COALESCE(MIN(qty), 1) > 1", + None + ), + Cell( + "M6 one of the two aggregates NOT published", + "SELECT city, MAX(amount) AS mx FROM customers GROUP BY city " + + "HAVING COALESCE(MAX(amount), MIN(qty)) > 1", + Some(Rule.Unpublished) + ), + // (I) ISNULL / ISNOTNULL over an aggregate: its operand is the aggregate every other position + // builds, so the bare and qualified spellings deploy exactly as the alias spelling does. + Cell( + "I1 ISNULL, bare spelling", + "SELECT city, MIN(amount) AS mn FROM customers GROUP BY city HAVING ISNULL(MIN(amount))", + None + ), + Cell( + "I2 ISNOTNULL, bare spelling", + "SELECT city, MIN(amount) AS mn FROM customers GROUP BY city HAVING ISNOTNULL(MIN(amount))", + None + ), + Cell( + "I3 ISNULL, qualified spelling", + "SELECT c.city, MIN(c.amount) AS mn FROM customers AS c GROUP BY c.city " + + "HAVING ISNULL(MIN(c.amount))", + None + ), + Cell( + "I4 ISNULL, alias spelling", + "SELECT city, MIN(amount) AS mn FROM customers GROUP BY city HAVING ISNULL(mn)", + None + ), + Cell( + "I5 ISNULL over an aggregate the SELECT list does not publish", + "SELECT city, MIN(amount) AS mn FROM customers GROUP BY city HAVING ISNULL(MAX(amount))", + Some(Rule.Unpublished) + ), // (U) the BACKSTOP population: a HAVING leaf whose selector parameter names no aggregation the // pivot creates. Two families, both PRE-EXISTING (accepted on the control too) and both still // correct as a plain SELECT. @@ -520,15 +588,21 @@ class MaterializedViewHavingSpec extends AnyFlatSpec with Matchers { "SELECT city, SUM(amount) FROM customers GROUP BY city HAVING SUM(amount) BETWEEN 1 AND 9", Some(Rule.NoMatchingAgg) ), - // U-2: a relation predicate BESIDE a metric conjunct. `extractAggregatePaths` falls to its - // `case _ => acc` for an `ElasticRelation`, and `havingLeaves` deliberately excludes relation - // predicates -- so the grouping-key and two-aggregate rules are STRUCTURALLY blind to it. The - // selector IS built and reads the metric only: a PARTIAL filter answering 200 with the wrong - // groups, which is exactly the harm the grouping-key rule's own docstring names. + // U-2: a relation predicate BESIDE a metric conjunct, refused by its own arm. A condition + // reading no metric has no form in a `bucket_selector`, so the selector reads the metric only: + // a PARTIAL filter answering 200 with the wrong groups, which is exactly the harm the + // grouping-key rule's own docstring names. Cell( "U10 relation predicate beside a metric", "SELECT city, SUM(amount) AS s FROM customers GROUP BY city HAVING s > 5 AND child(c.x = 1)", - Some(Rule.NoMatchingAgg) + Some(Rule.Relation) + ), + // A relation predicate is never alias-substituted, so `s` inside it stays a column and the + // condition reads no metric. It was accepted by name before, and the view dropped it. + Cell( + "U11 an alias comparison inside a relation predicate", + "SELECT city, SUM(amount) AS s FROM customers GROUP BY city HAVING nested(s > 5)", + Some(Rule.Relation) ), // (P) the ONE materialized-view example in the shipped documentation that carries a HAVING -- // `documentation/sql/materialized_views.md` "Materialized View with Aggregations", verbatim. @@ -588,18 +662,17 @@ class MaterializedViewHavingSpec extends AnyFlatSpec with Matchers { ), Rule.BucketScript -> refusalOf( asView( - "SELECT city, MAX(amount) - MIN(amount) AS d FROM customers GROUP BY city HAVING d > 3" - ) - ), - Rule.TwoAggregates -> refusalOf( - asView( - "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city " + - "HAVING MAX(amount) > MIN(amount)" + "SELECT city, STDDEV(amount) - MIN(amount) AS d FROM customers GROUP BY city HAVING d > 3" ) ), Rule.Unpublished -> refusalOf( asView("SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING MAX(amount) > 1") ), + Rule.Relation -> refusalOf( + asView( + "SELECT city, SUM(amount) AS s FROM customers GROUP BY city HAVING s > 5 AND child(c.x = 1)" + ) + ), Rule.NoMatchingAgg -> refusalOf( asView("SELECT city, SUM(amount) FROM customers GROUP BY city HAVING SUM(amount) > 5") ) @@ -838,18 +911,23 @@ class MaterializedViewHavingSpec extends AnyFlatSpec with Matchers { ) should include(Rule.NoTransformAgg) } - "the two-aggregate rule" should "win over the unpublished-aggregate rule" in { - // πŸ”΄ Same reason as rule 3's precedence, one operand over: rule 5's remedy is "add it to the - // SELECT list", and MEASURED, applying it to this statement produces R1 -- a selector that - // reads an undeclared parameter and rejects every group. A refusal whose remedy leads - // somewhere worse is a defect, not a nicety. + "an unpublished aggregate on the value side" should "get the unpublished-aggregate remedy, which ends the journey" in { + // The old rule 5 spoke here, because publishing the right-hand aggregate used to land on R1 -- + // a selector reading an undeclared parameter. With the filter declaring every metric it reads, + // R1 deploys, so rule 6's remedy is the one that ends the journey: applied literally, ACCEPTED. val reason = refusalOf( asView( "SELECT city, MAX(amount) AS mx FROM customers GROUP BY city HAVING MAX(amount) > MIN(amount)" ) ) - reason should include(Rule.TwoAggregates) - reason should not include Rule.Unpublished + reason should include(Rule.Unpublished) + reason should include("MIN(amount) AS ") + verdict( + asView( + "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city " + + "HAVING MAX(amount) > MIN(amount)" + ) + ) shouldBe Accepted } // ───────────────────────────────────────────────────────────────────────────────────────────── @@ -897,9 +975,6 @@ class MaterializedViewHavingSpec extends AnyFlatSpec with Matchers { "R2 filter the key in WHERE" -> ("SELECT city, SUM(amount) AS s FROM customers GROUP BY city HAVING city = 'Paris'", "SELECT city, SUM(amount) AS s FROM customers WHERE city = 'Paris' GROUP BY city HAVING SUM(amount) > 5"), - "R5 compare with a constant" -> - ("SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city HAVING MAX(amount) > MIN(amount)", - "SELECT city, MAX(amount) AS mx FROM customers GROUP BY city HAVING MAX(amount) > 5"), "R6 add it to the SELECT list" -> ("SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING MIN(qty) > 5", "SELECT city, COUNT(*) AS c, MIN(qty) AS mq FROM customers GROUP BY city HAVING MIN(qty) > 5") @@ -921,7 +996,7 @@ class MaterializedViewHavingSpec extends AnyFlatSpec with Matchers { } } - "the backstop message" should "teach the alias rule and name the relation case" in { + "the backstop message" should "teach the alias rule, and the relation arm name its case" in { // πŸ”΄ Same weakness M14 exposed one rule over: the population keys on "matches none of the // aggregations this view creates", which survives deleting everything that TEACHES. The // backstop is the message most users will see for the commonest mistake (no SELECT alias), @@ -993,12 +1068,8 @@ class MaterializedViewHavingSpec extends AnyFlatSpec with Matchers { } } - /** πŸ”΄ Why the `isAggregation` guard on `havingValueSideAggs` has a GREEN mutation. - * - * A value side that is NOT an aggregate never reaches the view rules: the shared rules inside - * `dql.validate()` refuse it first. These rows pin THAT, which is what makes the guard dead from - * SQL -- if a shared rule ever stopped firing, the guard would become live and its mutation - * would start to matter. Recording the reason beats leaving a green cell unexplained. + /** A value side that is NOT an aggregate never reaches the view rules: the shared rules inside + * `dql.validate()` refuse it first, in both venues alike. */ "a non-aggregate on the value side" should "be refused by the shared rules, before the view rules" in { Seq( @@ -1016,13 +1087,13 @@ class MaterializedViewHavingSpec extends AnyFlatSpec with Matchers { refusalOf(body) shouldBe reason } } - // …and a SELECT alias of an aggregate IS substituted before the guard sees it, so the guard - // admits it as the aggregate it is rather than excluding it. - refusalOf( + // …and a SELECT alias of an aggregate IS substituted, so it is read as the aggregate it is -- + // published here, so the view deploys. + verdict( asView( "SELECT city, COUNT(*) AS c, MAX(amount) AS mx FROM customers GROUP BY city HAVING MAX(amount) > c" ) - ) should include(Rule.TwoAggregates) + ) shouldBe Accepted } "the shared HAVING rules" should "still fire inside a view, unchanged" in { @@ -1078,4 +1149,176 @@ class MaterializedViewHavingSpec extends AnyFlatSpec with Matchers { "CREATE TABLE t2 AS SELECT city, COUNT(*) AS c FROM customers GROUP BY city HAVING city = 'Paris'" ) shouldBe Accepted } + + // ───────────────────────────────────────────────────────────────────────────────────────────── + // What a view's filter declares (#292), and the per-group calculation channel. + // ───────────────────────────────────────────────────────────────────────────────────────────── + + private val ParamRead = """params\.([A-Za-z_][A-Za-z0-9_]*)""".r + + private def viewSearch(body: String): SingleSearch = Parser(asView(body)) match { + case Right(cr: CreateMaterializedView) => cr.search + case other => fail(s"expected a view, got $other: [$body]") + } + + "every view-accepted cell" should "declare exactly the metrics its filter reads, each one the pivot creates" in { + // The contract `softclient4es-extensions` builds the view's filter on: it declares + // `Having.metricNames` and nothing else, so those names must be EXACTLY the parameters the + // script reads, and every one of them an aggregation the pivot creates. Asked of the + // population, never of a hand list. + val accepted = population.filter(c => c.mvRefusedBy.isEmpty && c.body.contains("HAVING")) + accepted.size should be > 20 + accepted.foreach { cell => + withClue(s"[${cell.id}] ") { + val search = viewSearch(cell.body) + val names = search.having.toSeq.flatMap(_.metricNames) + val read = search.having + .flatMap(_.criteria) + .flatMap(MetricSelectorScript.nullAwareSelectorScript) + .toSeq + .flatMap(script => ParamRead.findAllMatchIn(script).map(_.group(1))) + .distinct + names should not be empty + names.toSet shouldBe read.toSet + names.foreach(n => search.transformAggregationNames should contain(n)) + } + } + } + + "the per-group calculation channel" should "compute a SELECT bucket_script item, its operands published or not" in { + import app.softnetwork.elastic.sql.transform.BucketScriptTransformAggregation + val published = viewSearch( + "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn, MAX(amount) - MIN(amount) AS d " + + "FROM customers GROUP BY city HAVING d > 3" + ) + published.transformBucketScripts.keys.toSeq shouldBe Seq("d") + published.transformBucketScripts("d") shouldBe a[BucketScriptTransformAggregation] + published.transformBucketScripts("d").node.toString shouldBe + """{"bucket_script":{"buckets_path":{"mx":"mx","mn":"mn"},"script":"params.mx - params.mn"}}""" + published.transformBucketScriptOperands shouldBe empty + published.having.toSeq.flatMap(_.metricNames) shouldBe Seq("d") + + // The operands the SELECT list does not publish are created under the key the script reads. + val unpublished = viewSearch( + "SELECT city, MAX(amount) - MIN(amount) AS d FROM customers GROUP BY city HAVING d > 3" + ) + unpublished.transformBucketScripts("d").node.toString shouldBe + """{"bucket_script":{"buckets_path":{"max_amount":"max_amount","min_amount":"min_amount"},""" + + """"script":"params.max_amount - params.min_amount"}}""" + unpublished.transformBucketScriptOperands.map(f => + s"${f.identifier.sql} AS ${f.fieldAlias.map(_.alias).getOrElse("")}" + ) shouldBe Seq("MAX(amount) AS max_amount", "MIN(amount) AS min_amount") + + // With no HAVING too: the column the view declares is the one its pivot computes. + viewSearch( + "SELECT city, MAX(amount) - MIN(amount) AS d FROM customers GROUP BY city" + ).transformBucketScripts.keySet shouldBe Set("d") + + // An operand no transform computes: the channel does not serve the item. + viewSearch( + "SELECT city, STDDEV(amount) - MIN(amount) AS d FROM customers GROUP BY city" + ).transformBucketScripts shouldBe empty + } + + it should "compute a DATEDIFF over aggregates exactly as the search's bucket_script does" in { + // A date aggregate reaches a bucket_script as the epoch millis Elasticsearch computed and a + // string literal as a String: each operand is converted to a date first, an aggregate ONCE (it + // is the instant in UTC already). Both venues render ONE calculation (the item's context-free + // rendering); the RUN is GroupByCompletenessSpec's. + def date(metric: String) = + s"Instant.ofEpochMilli(((long) params.$metric)).atZone(ZoneId.of('Z')).toLocalDate()" + val literal = + """LocalDate.parse(("2024-01-01").replace("/", "-"), DateTimeFormatter.ofPattern("yyyy-MM-dd"))""" + Seq( + // MySQL's DATEDIFF is `first - second`, every other spelling `end - start` + "DATEDIFF(MAX(d), '2024-01-01')" -> s"Long.valueOf(ChronoUnit.DAYS.between($literal, ${date("mx")}))", + "DATE_DIFF(MAX(d), MIN(d), DAY)" -> s"Long.valueOf(ChronoUnit.DAYS.between(${date("mx")}, ${date("mn")}))", + "TIMESTAMPDIFF(DAY, MAX(d), '2024-01-01')" -> + s"Long.valueOf(ChronoUnit.DAYS.between(${date("mx")}, $literal))" + ).foreach { case (expression, script) => + val body = + s"SELECT city, MAX(d) AS mx, MIN(d) AS mn, $expression AS x FROM customers GROUP BY city HAVING x > 1" + withClue(s"[$expression] ") { + val view = viewSearch(body) + view.transformBucketScripts("x").script shouldBe script + view.transformBucketScripts("x").bucketsPath.keySet shouldBe + ParamRead.findAllMatchIn(script).map(_.group(1)).toSet + view.transformBucketScripts("x").params shouldBe empty + view.having.toSeq.flatMap(_.metricNames) shouldBe Seq("x") + Parser(body) match { + case Right(search: SingleSearch) => + search.select.fields + .find(_.fieldAlias.exists(_.alias == "x")) + .map(_.identifier.painless(None)) shouldBe Some(script) + case other => fail(s"expected a search, got $other") + } + } + } + } + + it should "declare the script parameters it reads beyond its bucket paths: the request clock" in { + // CURRENT_DATE / NOW() render against `params.__now__`, which no bucket path binds: the model + // carries it, for the pivot to bind, and the script reads nothing else undeclared. + Seq( + "DATEDIFF(MAX(d), CURRENT_DATE)", + "DATE_DIFF(MAX(d), NOW(), HOUR)", + "TIMESTAMPDIFF(DAY, MIN(d), CURRENT_TIMESTAMP)" + ).foreach { expression => + withClue(s"[$expression] ") { + val channel = viewSearch( + s"SELECT city, $expression AS x FROM customers GROUP BY city HAVING x > 1" + ).transformBucketScripts("x") + channel.params shouldBe Seq("__now__") + ParamRead.findAllMatchIn(channel.script).map(_.group(1)).toSet shouldBe + channel.bucketsPath.keySet ++ channel.params + } + } + } + + "rule 4's refusals" should "each be accepted once their remedy is applied literally" in { + // An operand no transform computes: "apply the condition when querying the view". + verdict( + asView("SELECT city, STDDEV(amount) - MIN(amount) AS d FROM customers GROUP BY city") + ) shouldBe Accepted + // A null test over the alias: "compare d itself". + val nullTest = refusalOf( + asView( + "SELECT city, MAX(amount) - MIN(amount) AS d FROM customers GROUP BY city HAVING ISNULL(d)" + ) + ) + nullTest should include("Compare d itself (HAVING d > 10)") + verdict( + asView( + "SELECT city, MAX(amount) - MIN(amount) AS d FROM customers GROUP BY city HAVING d > 10" + ) + ) shouldBe Accepted + } + + "a condition on an UNNEST aggregate" should "keep the verdict it always had" in { + // `update` wraps it in `ElasticNested` itself; it reads its metric like any other condition, so + // it is not the relation arm's. + verdict( + asView( + "SELECT e.name, COUNT(e.address) AS c FROM t JOIN UNNEST(t.emails) AS e GROUP BY e.name HAVING c > 1" + ) + ) shouldBe Accepted + } + + "the inline-arithmetic refusal" should "stay shared, and its remedy reach an accepted view" in { + // Search is unchanged: inline arithmetic over aggregates is refused in every venue. Its remedy + // -- alias the expression in SELECT and reference the alias -- now ends the journey in a view + // too, through the per-group calculation channel. + val body = + "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn FROM customers GROUP BY city " + + "HAVING MAX(amount) - MIN(amount) > 3" + val reason = refusalOf(asView(body)) + reason should include("alias the expression in SELECT and reference the alias") + refusalOf(body) shouldBe reason + verdict( + asView( + "SELECT city, MAX(amount) AS mx, MIN(amount) AS mn, MAX(amount) - MIN(amount) AS d " + + "FROM customers GROUP BY city HAVING d > 3" + ) + ) shouldBe Accepted + } } diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/DateFunctionExecutionSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/DateFunctionExecutionSpec.scala index 4b2f7e8f6..86f825cc6 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/DateFunctionExecutionSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/DateFunctionExecutionSpec.scala @@ -23,16 +23,19 @@ import app.softnetwork.elastic.client.bulk._ import app.softnetwork.elastic.client.result._ import app.softnetwork.elastic.client.spi.ElasticClientFactory import app.softnetwork.elastic.scalatest.ElasticDockerTestKit +import app.softnetwork.elastic.sql.parser.Parser import app.softnetwork.elastic.sql.query.SelectStatement import app.softnetwork.persistence.generateUUID import org.scalatest.flatspec.AnyFlatSpecLike import org.scalatest.matchers.should.Matchers import org.slf4j.{Logger, LoggerFactory} +import java.time.{Instant, LocalDate, ZoneOffset} import scala.collection.immutable.ListMap import scala.concurrent.Await import scala.concurrent.duration._ import scala.language.implicitConversions +import scala.util.Try /** Issue #368 -- `LAST_DAY` and `EPOCHDAY` over a `date` column EXECUTE and return the right value. * @@ -111,6 +114,7 @@ trait DateFunctionExecutionSpec extends AnyFlatSpecLike with ElasticDockerTestKi override def afterAll(): Unit = { client.deleteIndex(index) + if (diffIndexCreated) client.deleteIndex(diffIndex) super.afterAll() } @@ -201,4 +205,310 @@ trait DateFunctionExecutionSpec extends AnyFlatSpecLike with ElasticDockerTestKi // change must not have moved it. byId(s"SELECT id, YEAR(d) AS y FROM $index", "y") shouldBe expectedYear } + + // ----------------------------------------------------------------------------------------------- + // The DATEDIFF family against its documented semantics, computed HERE: + // + // - HOUR, MINUTE and SECOND count the ELAPSED whole units between two instants (UTC), truncated + // toward zero; a DATE operand is the start of its day; + // - DAY and every larger unit compare two CALENDAR dates (UTC): days, weeks (days / 7), months + // (complete once the day of month is reached), quarters (months / 3), years (months / 12), + // truncated toward zero; + // - the answer is `second - first`, except MySQL's two-argument DATEDIFF: `first - second`; + // - a string literal is the temporal it spells, a DATE alone or a TIMESTAMP (UTC unless it names + // a zone); NULL when an operand has no value. + // + // The population is every spelling (DATE_DIFF / DATEDIFF with the unit last or first, + // TIMESTAMPDIFF, DATE_DIFF without a unit, MySQL's DATEDIFF), every unit, every operand kind (a + // date column, a timestamp column, a DATE literal, two TIMESTAMP literals, a date and a timestamp + // aggregate) and every venue: a SELECT item per document, a WHERE, a SELECT item per group and a + // HAVING through that item's alias. Before, HOUR / MINUTE / SECOND over a column or an aggregate + // failed with `Unsupported unit: Hours`, QUARTER did not compile, and a literal failed at row + // level and, with a time of day, per group. + // ----------------------------------------------------------------------------------------------- + + private val diffIndex = "date_diff_semantics" + + @volatile private var diffIndexCreated = false + + private type DiffDoc = (String, String, Option[String], Option[String]) + + /** (id, group, `d` a calendar date, `ts` an instant); `None`: the document lacks it. Group g1 has + * no value at all, g4 has some. + */ + private val diffDocs: Seq[DiffDoc] = Seq( + ("r1", "g1", None, None), + ("r2", "g1", None, None), + ("r3", "g2", Some("2024-01-02"), Some("2024-01-01T23:30:00Z")), + ("r4", "g3", Some("2023-11-15"), Some("2024-01-01T10:30:15Z")), + ("r5", "g3", Some("2024-02-29"), Some("2024-03-31T00:00:45Z")), + ("r6", "g4", Some("2024-01-31"), None), + ("r7", "g4", None, Some("2023-12-31T23:59:59Z")), + ("r8", "g4", Some("2025-06-15"), Some("2024-06-14T12:00:00Z")) + ) + + private lazy val diffIndexLoaded: Unit = { + client + .createIndex(diffIndex, settings = """{"number_of_shards": 1, "number_of_replicas": 0}""") + .get shouldBe true + diffIndexCreated = true + client + .setMapping( + diffIndex, + """{"properties": {"id": {"type": "keyword"}, "g": {"type": "keyword"}, "d": {"type": "date"}, "ts": {"type": "date"}}}""" + ) + .get shouldBe true + val docs = diffDocs.toList.map { case (id, g, d, ts) => + (Seq(s""""id":"$id"""", s""""g":"$g"""") ++ d.map(x => s""""d":"$x"""") ++ + ts.map(x => s""""ts":"$x"""")).mkString("{", ",", "}") + } + implicit val bulkOptions: BulkOptions = BulkOptions(defaultIndex = diffIndex, logEvery = 10) + implicit def listToSource[T](list: List[T]): Source[T, NotUsed] = + Source.fromIterator(() => list.iterator) + client.bulk[String](docs, identity, idKey = Some(Set("id"))) match { + case ElasticSuccess(_) => client.refresh(diffIndex) + case ElasticFailure(error) => + fail(s"Bulk indexing into $diffIndex failed: ${error.message}") + } + } + + /** An operand's value: a calendar date, or an instant. */ + private sealed trait DiffValue + private final case class OnDate(date: LocalDate) extends DiffValue + private final case class AtInstant(instant: Instant) extends DiffValue + + /** An operand of the population, and its value over a document or over a group's documents. */ + private final case class DiffOperand(sql: String, aggregate: Boolean)( + val value: Seq[DiffDoc] => Option[DiffValue] + ) + + private def constant(sql: String, value: DiffValue): DiffOperand = + DiffOperand(sql, aggregate = false)(_ => Some(value)) + + private val diffLiterals: Seq[DiffOperand] = Seq( + constant("'2024-01-01'", OnDate(LocalDate.parse("2024-01-01"))), + constant("'2024-01-01 10:00:00'", AtInstant(Instant.parse("2024-01-01T10:00:00Z"))), + constant("'2023-12-31T22:15:30Z'", AtInstant(Instant.parse("2023-12-31T22:15:30Z"))) + ) + + private val diffRowOperands: Seq[DiffOperand] = Seq( + DiffOperand("d", aggregate = false)(_.head._3.map(x => OnDate(LocalDate.parse(x)))), + DiffOperand("ts", aggregate = false)(_.head._4.map(x => AtInstant(Instant.parse(x)))) + ) ++ diffLiterals + + private val diffGroupOperands: Seq[DiffOperand] = Seq( + DiffOperand("MAX(d)", aggregate = true)( + _.flatMap(_._3) + .map(LocalDate.parse(_)) + .reduceOption((x, y) => if (x.isAfter(y)) x else y) + .map(date => OnDate(date)) + ), + DiffOperand("MIN(ts)", aggregate = true)( + _.flatMap(_._4) + .map(Instant.parse(_)) + .reduceOption((x, y) => if (x.isBefore(y)) x else y) + .map(instant => AtInstant(instant)) + ) + ) ++ diffLiterals + + private def utcDate(value: DiffValue): LocalDate = value match { + case OnDate(date) => date + case AtInstant(instant) => instant.atZone(ZoneOffset.UTC).toLocalDate + } + + private def utcSeconds(value: DiffValue): Long = value match { + case OnDate(date) => date.toEpochDay * 86400L + case AtInstant(instant) => instant.getEpochSecond + } + + /** The complete months from `start` to `end`: a month counts once its day of month is reached. */ + private def wholeMonths(start: LocalDate, end: LocalDate): Long = { + def packed(date: LocalDate): Long = + (date.getYear * 12L + date.getMonthValue - 1) * 32L + date.getDayOfMonth + (packed(end) - packed(start)) / 32L + } + + /** `end - start` in `unit`, truncated toward zero (Scala's `Long` division). */ + private def documentedDiff(unit: String, start: DiffValue, end: DiffValue): Long = unit match { + case "SECOND" => utcSeconds(end) - utcSeconds(start) + case "MINUTE" => (utcSeconds(end) - utcSeconds(start)) / 60L + case "HOUR" => (utcSeconds(end) - utcSeconds(start)) / 3600L + case "DAY" => utcDate(end).toEpochDay - utcDate(start).toEpochDay + case "WEEK" => (utcDate(end).toEpochDay - utcDate(start).toEpochDay) / 7L + case "MONTH" => wholeMonths(utcDate(start), utcDate(end)) + case "QUARTER" => wholeMonths(utcDate(start), utcDate(end)) / 3L + case "YEAR" => wholeMonths(utcDate(start), utcDate(end)) / 12L + } + + /** A spelling of the family: its SQL over two operands, its unit, and whether it is MySQL's + * two-argument DATEDIFF (`first - second`). + */ + private final case class DiffSpelling(unit: String, mysql: Boolean)( + val sql: (String, String) => String + ) + + private val diffUnits: Seq[String] = + Seq("YEAR", "QUARTER", "MONTH", "WEEK", "DAY", "HOUR", "MINUTE", "SECOND") + + private val diffSpellings: Seq[DiffSpelling] = diffUnits.flatMap { u => + Seq( + DiffSpelling(u, mysql = false)((a, b) => s"DATE_DIFF($a, $b, $u)"), + DiffSpelling(u, mysql = false)((a, b) => s"DATEDIFF($a, $b, $u)"), + DiffSpelling(u, mysql = false)((a, b) => s"DATE_DIFF($u, $a, $b)"), + DiffSpelling(u, mysql = false)((a, b) => s"DATEDIFF($u, $a, $b)"), + DiffSpelling(u, mysql = false)((a, b) => s"TIMESTAMPDIFF($u, $a, $b)") + ) + } ++ Seq( + DiffSpelling("DAY", mysql = false)((a, b) => s"DATE_DIFF($a, $b)"), + DiffSpelling("DAY", mysql = true)((a, b) => s"DATEDIFF($a, $b)") + ) + + /** A statement, the venue it runs in, and the documented answer for each document or group. */ + private final case class DiffStatement( + sql: String, + venue: String, + unit: String, + expected: Map[String, Option[Long]] + ) { + def filters: Boolean = venue == "WHERE" || venue == "HAVING" + def kept: Set[String] = expected.collect { case (k, Some(v)) if v > 0 => k }.toSet + } + + private lazy val diffPopulation: Seq[DiffStatement] = { + val perDocument: Seq[(String, Seq[DiffDoc])] = diffDocs.map(doc => doc._1 -> Seq(doc)) + val perGroup: Seq[(String, Seq[DiffDoc])] = diffDocs.groupBy(_._2).toSeq.sortBy(_._1) + val rowPairs = for (a <- diffRowOperands; b <- diffRowOperands) yield (a, b) + // A WHERE over two literals reads no column: it is a constant predicate, rendered as a bare + // comparison of the function's boxed result, which fails to compile whatever the function is + // (`WHERE ABS(-3) > 0` too). It is not a DATEDIFF question, so it is left out. + val wherePairs = rowPairs.filterNot { case (a, b) => + diffLiterals.contains(a) && diffLiterals.contains(b) + } + val groupPairs = + for (a <- diffGroupOperands; b <- diffGroupOperands if a.aggregate || b.aggregate) + yield (a, b) + for { + spelling <- diffSpellings + (venue, pairs, scopes) <- Seq( + ("SELECT", rowPairs, perDocument), + ("WHERE", wherePairs, perDocument), + ("GROUP", groupPairs, perGroup), + ("HAVING", groupPairs, perGroup) + ) + (a, b) <- pairs + } yield { + val e = spelling.sql(a.sql, b.sql) + val sql = venue match { + case "SELECT" => s"SELECT id, $e AS x FROM $diffIndex" + case "WHERE" => s"SELECT id FROM $diffIndex WHERE $e > 0" + case "GROUP" => s"SELECT g, $e AS x FROM $diffIndex GROUP BY g" + case _ => s"SELECT g, $e AS x FROM $diffIndex GROUP BY g HAVING x > 0" + } + val expected = scopes.map { case (key, docs) => + key -> (for { + first <- a.value(docs) + second <- b.value(docs) + } yield + if (spelling.mysql) documentedDiff(spelling.unit, second, first) + else documentedDiff(spelling.unit, first, second)) + }.toMap + DiffStatement(sql, venue, spelling.unit, expected) + } + } + + /** The rows of a statement through the gateway, or why there are none: a streamed result fails + * while it is consumed, so the whole read is guarded. + */ + private def diffRows(sql: String): Either[String, Seq[ListMap[String, Any]]] = + Try { + Await.result(client.run(sql), 60.seconds) match { + case ElasticSuccess(QueryRows(rows, _)) => Right(rows) + case ElasticSuccess(QueryStructured(response, _)) => Right(response.results) + case ElasticSuccess(QueryStream(stream, _)) => + Right(Await.result(stream.map(_._1).runWith(Sink.seq), 60.seconds)) + case ElasticSuccess(other) => Left(s"unexpected result $other") + case ElasticFailure(error) => Left(error.message) + } + }.toEither.left.map(t => s"${t.getClass.getSimpleName}: ${t.getMessage}").joinRight + + /** A whole number, from a `script_fields` array, a bucket value or NULL. */ + private def whole(value: Any): Either[String, Option[Long]] = value match { + case null => Right(None) + case s: Seq[_] => s.headOption.map(whole).getOrElse(Right(None)) + case c: java.util.Collection[_] => whole(c.toArray.toSeq) + case other => + Try(BigDecimal(other.toString)).toOption match { + case Some(n) if n.isWhole => Right(Some(n.toLong)) + case _ => Left(s"not a whole number: $other") + } + } + + "the DATEDIFF family" should "answer its documented semantics for every spelling, unit, operand and venue" in { + diffIndexLoaded + val population = diffPopulation + // Non-vacuity, computed over the material: every spelling in every venue over every operand + // pair, every unit answering a negative, a zero and a positive somewhere, NULL in every venue, + // and the filters keeping some and dropping some. + population.size shouldBe diffSpellings.size * (25 + 16 + 16 + 16) + population.foreach(st => withClue(s"[${st.sql}] ")(Parser(st.sql).isRight shouldBe true)) + diffUnits.foreach { u => + val answers = population.filter(_.unit == u).flatMap(_.expected.values.flatten) + withClue(s"$u: ") { + Seq(answers.exists(_ < 0), answers.contains(0L), answers.exists(_ > 0)) shouldBe + Seq(true, true, true) + } + } + Seq("SELECT", "WHERE", "GROUP", "HAVING").foreach { venue => + withClue(s"$venue: ") { + population.filter(_.venue == venue).exists(_.expected.values.exists(_.isEmpty)) shouldBe + true + } + } + population + .filter(_.filters) + .exists(st => st.kept.nonEmpty && st.kept != st.expected.keySet) shouldBe + true + + val outcomes: Seq[(DiffStatement, Either[String, String])] = population.map { st => + st -> diffRows(st.sql).flatMap { rows => + val key = if (st.venue == "SELECT" || st.venue == "WHERE") "id" else "g" + if (st.filters) { + val actual = rows.map(_.getOrElse(key, "?").toString).toSet + Right( + if (actual == st.kept) "" + else + s"kept ${actual.toSeq.sorted.mkString(",")}, expected ${st.kept.toSeq.sorted.mkString(",")}" + ) + } else { + val values = + rows.map(r => r.getOrElse(key, "?").toString -> whole(r.getOrElse("x", null))) + values.collectFirst { case (_, Left(error)) => error } match { + case Some(error) => Left(error) + case None => + val actual = values.collect { case (k, Right(v)) => k -> v }.toMap + Right( + if (actual == st.expected) "" + else + s"answered ${actual.toSeq.sortBy(_._1).mkString(" ")}, expected " + + st.expected.toSeq.sortBy(_._1).mkString(" ") + ) + } + } + } + } + val wrong = outcomes.collect { case (st, Right(diff)) if diff.nonEmpty => s"[${st.sql}] $diff" } + val errors = outcomes.collect { case (st, Left(error)) => s"[${st.sql}] $error" } + info( + s"DATEDIFF family: ${population.size} statements; wrong: ${wrong.size}; errors: ${errors.size}" + ) + wrong.foreach(info(_)) + errors.foreach(info(_)) + withClue( + s"${wrong.size} wrong statements, ${errors.size} errors:\n" + + (wrong.take(20) ++ errors.take(10)).mkString("\n") + "\n" + ) { + wrong shouldBe empty + errors shouldBe empty + } + } } diff --git a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala index ceb425688..3def597fc 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala @@ -37,6 +37,8 @@ import org.scalatest.flatspec.AnyFlatSpecLike import org.scalatest.matchers.should.Matchers import org.slf4j.{Logger, LoggerFactory} +import java.time.LocalDate +import java.time.temporal.ChronoUnit import scala.collection.immutable.ListMap import scala.concurrent.Await import scala.concurrent.duration._ @@ -121,6 +123,7 @@ trait GroupByCompletenessSpec extends AnyFlatSpecLike with ElasticDockerTestKit client.deleteIndex(index) client.deleteIndex(nullIndex) client.deleteIndex(coalesceIndex) + client.deleteIndex(dateDiffIndex) super.afterAll() } @@ -1803,6 +1806,473 @@ trait GroupByCompletenessSpec extends AnyFlatSpecLike with ElasticDockerTestKit } } + // --------------------------------------------------------------------------------------------- + // The spellings a HAVING used to refuse and now answers: a SELECT alias inside a function, read + // as the aggregate it names at every depth (`COALESCE(ma, mb) > 1` is `COALESCE(MAX(a), MIN(b)) + // > 1`), and ISNULL / ISNOTNULL over a QUALIFIED aggregate, whose operand is now the aggregate + // every other position builds -- it was refused as "a different aggregate" than its own SELECT + // item. + // + // The population is DERIVED from the COALESCE one above: every operand of it in its alias + // spelling, under every form, plus the same operands on the VALUE side of a published aggregate, + // over the same fixture and the same three-valued oracle -- and, under the same forms, the alias + // of a SELECT bucket_script item (`MAX(a) - MIN(b) AS d`) under SIGN, NULL whenever an operand + // is. Every statement runs through the gateway; every one of them was refused before. + // --------------------------------------------------------------------------------------------- + + private val coalesceSelect = "g, MAX(a) AS ma, MIN(b) AS mb, MAX(b) AS xb, SUM(a) AS sa" + + /** An operand in its SELECT-alias spelling. */ + private def aliasSql(operand: CoalesceOperand): String = operand match { + case Aggregate(sql) => + Map("MAX(a)" -> "ma", "MIN(b)" -> "mb", "MAX(b)" -> "xb", "SUM(a)" -> "sa") + .getOrElse(sql, fail(s"no SELECT alias for $sql")) + case Literal(k) => k.toString + case CoalesceOf(as @ _*) => as.map(aliasSql).mkString("COALESCE(", ", ", ")") + case SignOf(argument) => s"SIGN(${aliasSql(argument)})" + case GreatestOf(as @ _*) => as.map(aliasSql).mkString("GREATEST(", ", ", ")") + case LeastOf(as @ _*) => as.map(aliasSql).mkString("LEAST(", ", ", ")") + } + + private lazy val respelledPopulation: Seq[NullStatement] = { + val oracle: Seq[(String, CoalesceGroup)] = coalesceGroups.map { case (g, rows) => + g -> CoalesceGroup(rows.size, rows.flatMap(_._1), rows.flatMap(_._2)) + } + def keep(eval: CoalesceGroup => Option[Boolean]): Set[String] = + oracle.collect { case (g, group) if eval(group).contains(true) => g }.toSet + // the alias spelling of every COALESCE-family statement above + val aliasInFunction = for { + operand <- coalesceOperands + c <- coalesceForms + if !(operand.isInstanceOf[SignOf] && readsNullTest(c)) + } yield NullStatement( + s"SELECT $coalesceSelect FROM $coalesceIndex GROUP BY g HAVING ${c.sql(aliasSql(operand))}", + keep(group => c.eval(operand.value(group), group.rows)) + ) + // the same operands on the VALUE side of a published aggregate, alias spelling on both sides + val valueSide = for { + operand <- coalesceOperands ++ Seq(CoalesceOf(minB, Literal(1)), SignOf(minB)) + op <- Seq(">", "<=") + } yield NullStatement( + s"SELECT $coalesceSelect FROM $coalesceIndex GROUP BY g HAVING ma $op ${aliasSql(operand)}", + keep { group => + for { + left <- maxA.value(group) + right <- operand.value(group) + } yield compares(left, op, right) + } + ) + // the alias of a SELECT bucket_script item under SIGN, NULL whenever an operand is: the filter + // reads the operands and declares them + val bucketScriptInFunction = for { + c <- coalesceForms + // the null test of SIGN, as above + if !readsNullTest(c) + } yield { + val having = c.sql("SIGN(d)") + NullStatement( + s"SELECT g, MAX(a) AS ma, MIN(b) AS mb, MAX(a) - MIN(b) AS d FROM $coalesceIndex GROUP BY g HAVING $having", + keep { group => + c.eval( + for { + x <- maxA.value(group) + y <- minB.value(group) + } yield math.signum(x - y), + group.rows + ) + } + ) + } + // ISNULL / ISNOTNULL over a qualified aggregate + val qualifiedNullTests = for { + aggregate <- Seq(maxA, minB, maxB) + isNull <- Seq(true, false) + } yield { + val qualified = aggregate.sql.replace("(", "(r.") + val test = if (isNull) s"ISNULL($qualified)" else s"ISNOTNULL($qualified)" + NullStatement( + s"SELECT r.g, $qualified AS m FROM $coalesceIndex AS r GROUP BY r.g HAVING $test", + keep(group => Some(aggregate.value(group).isEmpty == isNull)) + ) + } + aliasInFunction ++ valueSide ++ bucketScriptInFunction ++ qualifiedNullTests + } + + "a HAVING spelling that used to be refused" should + "keep exactly the groups SQL's three-valued logic keeps" in { + coalesceGroupsLoaded + val population = respelledPopulation + // Non-vacuity: every statement parses now, and the oracle keeps and drops a group whose `a` + // or `b` has no value. + population.size should be >= 200 + population.foreach(st => withClue(s"[${st.sql}] ")(Parser(st.sql).isRight shouldBe true)) + Seq("g1", "g2", "g4").foreach { g => + withClue(s"[$g] ") { + population.exists(_.expected.contains(g)) shouldBe true + population.exists(st => !st.expected.contains(g)) shouldBe true + } + } + + val outcomes = population.map(st => st -> keptGroups(st)) + val wrong = outcomes.collect { + case (st, Right(actual)) if actual != st.expected => (st, actual) + } + val errors = outcomes.collect { case (st, Left(error)) => s"[${st.sql}] $error" } + val wrongGroups = wrong.map { case (st, actual) => + ((st.expected -- actual) ++ (actual -- st.expected)).size + }.sum + info( + s"HAVING spellings that used to be refused: ${population.size} statements; wrong: " + + s"${wrong.size} statements / $wrongGroups groups; errors: ${errors.size}" + ) + val report = wrong.map { case (st, actual) => + s"[${st.sql}] kept ${actual.toSeq.sorted.mkString(",")}, expected ${st.expected.toSeq.sorted.mkString(",")}" + } + report.foreach(info(_)) + errors.foreach(info(_)) + withClue( + s"${wrong.size} wrong statements ($wrongGroups wrong groups), ${errors.size} errors:\n" + + (report.take(20) ++ errors.take(10)).mkString("\n") + "\n" + ) { + wrong shouldBe empty + errors shouldBe empty + } + } + + // --------------------------------------------------------------------------------------------- + // GREATEST / LEAST over aggregates in HAVING skip a NULL argument -- what the docs state and + // WHERE does: the result is NULL only when every argument is. The group filter emitted + // `Math.max(params.m, k)` with no NULL skip, behind a guard on EVERY argument, so a group was + // dropped as soon as ONE argument was NULL: MEASURED on Elasticsearch 8.18.3, 5 of the 6 shapes + // below kept the wrong groups (`LEAST(MIN(b), 1) < 3` dropped every group with no `b`). + // + // The population is DERIVED from the COALESCE one above -- the same fixture, the same forms and + // the same three-valued oracle, NULL arguments skipped -- over several aggregates and an + // aggregate with a literal, in the bare, alias and qualified spellings, on the value side of an + // aggregate, and in the implicit whole-table group. Every statement runs through the gateway. + // --------------------------------------------------------------------------------------------- + + private final case class GreatestOf(arguments: CoalesceOperand*) extends CoalesceOperand { + def sql: String = arguments.map(_.sql).mkString("GREATEST(", ", ", ")") + def value(group: CoalesceGroup): Option[Double] = + arguments.flatMap(_.value(group)).reduceOption(_ max _) + } + + private final case class LeastOf(arguments: CoalesceOperand*) extends CoalesceOperand { + def sql: String = arguments.map(_.sql).mkString("LEAST(", ", ", ")") + def value(group: CoalesceGroup): Option[Double] = + arguments.flatMap(_.value(group)).reduceOption(_ min _) + } + + private val reducerOperands: Seq[CoalesceOperand] = Seq( + GreatestOf(maxA, minB), + LeastOf(maxA, minB), + GreatestOf(minB, Literal(1)), + LeastOf(minB, Literal(1)), + GreatestOf(maxA, minB, maxB), + LeastOf(Literal(5), maxA, minB), + // SUM is never NULL (#336) + GreatestOf(Aggregate("SUM(a)"), minB), + LeastOf(GreatestOf(maxA, minB), maxB), + CoalesceOf(GreatestOf(maxA, minB), Literal(5)), + GreatestOf(CoalesceOf(maxA, Literal(0)), minB), + SignOf(LeastOf(maxA, minB)) + ) + + /** The measured shapes with the reducer on the LEFT, in the form they were measured in; the two + * on the value side (`MAX(a) > GREATEST(MIN(b), 1)`, `... LEAST(...)`) are in `valueSide`. + */ + private val reducerMeasured: Seq[(CoalesceOperand, NullCond)] = Seq( + GreatestOf(maxA, minB) -> Cmp(">", 1), + GreatestOf(minB, Literal(1)) -> Cmp(">", 1), + LeastOf(maxA, minB) -> Cmp("<", 3), + LeastOf(minB, Literal(1)) -> Cmp("<", 3) + ) + + private val reducerForms: Seq[NullCond] = coalesceForms :+ Cmp("<", 3) + + /** A GREATEST / LEAST with a literal argument: never NULL, and its rendering is a primitive. */ + private def primitiveReducer(operand: CoalesceOperand): Boolean = operand match { + case GreatestOf(as @ _*) => as.exists(_.isInstanceOf[Literal]) + case LeastOf(as @ _*) => as.exists(_.isInstanceOf[Literal]) + case _ => false + } + + private lazy val reducerPopulation: Seq[NullStatement] = { + val oracle: Seq[(String, CoalesceGroup)] = coalesceGroups.map { case (g, rows) => + g -> CoalesceGroup(rows.size, rows.flatMap(_._1), rows.flatMap(_._2)) + } + def keep(eval: CoalesceGroup => Option[Boolean]): Set[String] = + oracle.collect { case (g, group) if eval(group).contains(true) => g }.toSet + def qualified(operand: CoalesceOperand): String = + operand.sql.replace("(a)", "(r.a)").replace("(b)", "(r.b)") + val qualifiedSelect = "r.g, MAX(r.a) AS ma, MIN(r.b) AS mb, MAX(r.b) AS xb, SUM(r.a) AS sa" + val grouped = for { + operand <- reducerOperands + c <- reducerForms + // As for COALESCE: the null test of a function rendering a primitive fails to compile in + // Elasticsearch on main already (`Cannot cast from [double] to [java.lang.Object]`) -- SIGN, + // and a GREATEST / LEAST with a literal argument, which is never NULL. + if !((operand.isInstanceOf[SignOf] || primitiveReducer(operand)) && readsNullTest(c)) + (select, from, key, spelled) <- Seq( + ("g, COUNT(*) AS c", coalesceIndex, "g", operand.sql), + (coalesceSelect, coalesceIndex, "g", operand.sql), + (coalesceSelect, coalesceIndex, "g", aliasSql(operand)), + (qualifiedSelect, s"$coalesceIndex AS r", "r.g", qualified(operand)) + ) + } yield NullStatement( + s"SELECT $select FROM $from GROUP BY $key HAVING ${c.sql(spelled)}", + keep(group => c.eval(operand.value(group), group.rows)) + ) + // on the VALUE side of an aggregate, bare and alias spellings + val valueSide = for { + operand <- reducerOperands + op <- Seq(">", "<=") + (left, spelled) <- Seq(("MAX(a)", operand.sql), ("ma", aliasSql(operand))) + } yield NullStatement( + s"SELECT $coalesceSelect FROM $coalesceIndex GROUP BY g HAVING $left $op $spelled", + keep { group => + for { + l <- maxA.value(group) + r <- operand.value(group) + } yield compares(l, op, r) + } + ) + // The implicit whole-table group, one group at a time. + val wholeTable = for { + (operand, c) <- reducerMeasured + (g, group) <- oracle + } yield NullStatement( + s"SELECT COUNT(*) AS c FROM $coalesceIndex WHERE g = '$g' HAVING ${c.sql(operand.sql)}", + if (c.eval(operand.value(group), group.rows).contains(true)) Set(g) else Set.empty, + wholeTable = Some(g) + ) + grouped ++ valueSide ++ wholeTable + } + + "a HAVING over GREATEST / LEAST of aggregates" should + "keep exactly the groups SQL's three-valued logic keeps, NULL arguments skipped" in { + coalesceGroupsLoaded + val population = reducerPopulation + // Non-vacuity, computed over the material: the measured shapes are in it, in both spellings, + // and the oracle keeps and drops each group whose `a` or `b` has no value. + reducerMeasured.foreach { case (operand, c) => + Seq( + s"SELECT g, COUNT(*) AS c FROM $coalesceIndex GROUP BY g HAVING ${c.sql(operand.sql)}", + s"SELECT $coalesceSelect FROM $coalesceIndex GROUP BY g HAVING ${c.sql(aliasSql(operand))}" + ).foreach(sql => withClue(s"[$sql] ")(population.exists(_.sql == sql) shouldBe true)) + } + Seq("g1", "g2", "g4", "g5", "g6").foreach { g => + withClue(s"[$g] ") { + population.exists(st => st.wholeTable.isEmpty && st.expected.contains(g)) shouldBe true + population.exists(st => st.wholeTable.isEmpty && !st.expected.contains(g)) shouldBe true + } + } + population.foreach(st => withClue(s"[${st.sql}] ")(Parser(st.sql).isRight shouldBe true)) + + val outcomes = population.map(st => st -> keptGroups(st)) + val wrong = outcomes.collect { + case (st, Right(actual)) if actual != st.expected => (st, actual) + } + val errors = outcomes.collect { case (st, Left(error)) => s"[${st.sql}] $error" } + val wrongGroups = wrong.map { case (st, actual) => + ((st.expected -- actual) ++ (actual -- st.expected)).size + }.sum + info( + s"HAVING over GREATEST / LEAST of aggregates: ${population.size} statements; wrong: " + + s"${wrong.size} statements / $wrongGroups groups; errors: ${errors.size}" + ) + val report = wrong.map { case (st, actual) => + s"[${st.sql}] kept ${actual.toSeq.sorted.mkString(",")}, expected ${st.expected.toSeq.sorted.mkString(",")}" + } + report.foreach(info(_)) + errors.foreach(info(_)) + withClue( + s"${wrong.size} wrong statements ($wrongGroups wrong groups), ${errors.size} errors:\n" + + (report.take(20) ++ errors.take(10)).mkString("\n") + "\n" + ) { + wrong shouldBe empty + errors shouldBe empty + } + } + + // --------------------------------------------------------------------------------------------- + // DATEDIFF / DATE_DIFF / TIMESTAMPDIFF over aggregates, computed per group. The calculation is a + // bucket_script, which reads a date aggregate as the epoch millis Elasticsearch computed: it failed + // the search for every statement below -- `java.lang.Double cannot be cast to ... Temporal` over + // two aggregates, `Cannot cast from [java.lang.String] to [java.time.temporal.Temporal]` over an + // aggregate and a literal. + // + // The population is every spelling of the family (MySQL's DATEDIFF with and without a unit, + // DATEDIFF / DATE_DIFF / TIMESTAMPDIFF unit-first, DATE_DIFF date-first), over an aggregate and a + // literal and over two aggregates, bare and table-qualified: as a SELECT item, whose value is + // checked in every group, and through its SELECT alias in HAVING -- the route the "alias it in + // SELECT" remedy of an inline DATEDIFF leads to. The fixture holds a group with no date, a group + // with some and groups with all of them; the expected values are computed HERE, NULL when an + // operand has no value. + // --------------------------------------------------------------------------------------------- + + private val dateDiffIndex = "having_datediff_groups" + + /** Each group and the `d` of each of its documents (`None`: the document lacks it). */ + private val dateDiffGroups: Seq[(String, Seq[Option[String]])] = Seq( + "s1" -> Seq(None, None), + "s2" -> Seq(Some("2023-12-01")), + "s3" -> Seq(Some("2024-03-01"), Some("2024-01-01")), + "s4" -> Seq(Some("2023-06-01"), None, Some("2024-02-15")), + "s5" -> Seq(Some("2024-01-02")), + "s6" -> Seq(Some("2024-01-01"), Some("2024-01-02")) + ) + + private lazy val dateDiffGroupsLoaded: Unit = { + client + .createIndex(dateDiffIndex, settings = """{"number_of_shards": 1, "number_of_replicas": 0}""") + .get shouldBe true + client + .setMapping( + dateDiffIndex, + """{"properties": {"id": {"type": "keyword"}, "sensor_id": {"type": "keyword"}, "d": {"type": "date"}}}""" + ) + .get shouldBe true + val docs = for { + (s, ds) <- dateDiffGroups.toList + (d, offset) <- ds.zipWithIndex + } yield (Seq(s""""id":"$s-$offset"""", s""""sensor_id":"$s"""") ++ d.map(x => s""""d":"$x"""")) + .mkString("{", ",", "}") + implicit val bulkOptions: BulkOptions = + BulkOptions(defaultIndex = dateDiffIndex, logEvery = 100) + implicit def listToSource[T](list: List[T]): Source[T, NotUsed] = + Source.fromIterator(() => list.iterator) + client.bulk[String](docs, identity, idKey = Some(Set("id"))) match { + case ElasticSuccess(_) => client.refresh(dateDiffIndex) + case ElasticFailure(error) => + fail(s"Bulk indexing into $dateDiffIndex failed: ${error.message}") + } + } + + /** A spelling of the family over two operands, and the days it answers for them: `end - start`, + * except MySQL's two-argument DATEDIFF, which is `first - second`. + */ + private final case class DateDiffForm( + sql: (String, String) => String, + days: (LocalDate, LocalDate) => Long + ) + + private val dateDiffForms: Seq[DateDiffForm] = { + def between(start: LocalDate, end: LocalDate): Long = ChronoUnit.DAYS.between(start, end) + Seq( + DateDiffForm((a, b) => s"DATEDIFF($a, $b)", (a, b) => between(b, a)), + DateDiffForm((a, b) => s"DATEDIFF($a, $b, DAY)", between), + DateDiffForm((a, b) => s"DATEDIFF(DAY, $a, $b)", between), + DateDiffForm((a, b) => s"DATE_DIFF($a, $b, DAY)", between), + DateDiffForm((a, b) => s"DATE_DIFF(DAY, $a, $b)", between), + DateDiffForm((a, b) => s"TIMESTAMPDIFF(DAY, $a, $b)", between) + ) + } + + /** A statement of the population: its value of `x_inline` in each group (`None`: NULL) for a + * SELECT, or the groups it keeps for a HAVING. + */ + private final case class DateDiffStatement( + sql: String, + values: Option[Map[String, Option[Long]]], + kept: Option[Set[String]] + ) + + private lazy val dateDiffPopulation: Seq[DateDiffStatement] = { + val literal = LocalDate.parse("2024-01-01") + val groups: Seq[(String, Seq[LocalDate])] = dateDiffGroups.map { case (s, ds) => + s -> ds.flatten.map(x => LocalDate.parse(x)) + } + def maxOf(ds: Seq[LocalDate]) = ds.reduceOption((x, y) => if (x.isAfter(y)) x else y) + def minOf(ds: Seq[LocalDate]) = ds.reduceOption((x, y) => if (x.isBefore(y)) x else y) + for { + spelling <- dateDiffForms + (second, readsMin, secondValue) <- Seq( + ("'2024-01-01'", false, (_: Seq[LocalDate]) => Option(literal)), + ("MIN(d)", true, minOf _) + ) + qualified <- Seq(false, true) + (form, filter) <- Seq[(String, Option[Long => Boolean])]( + ("", None), + (" HAVING x_inline > 1", Some((v: Long) => v > 1)), + (" HAVING x_inline < -1", Some((v: Long) => v < -1)) + ) + } yield { + val q = if (qualified) "r." else "" + val items = s"MAX(${q}d) AS max_d" + (if (readsMin) s", MIN(${q}d) AS min_d" else "") + val expr = spelling.sql(s"MAX(${q}d)", second.replace("(d)", s"(${q}d)")) + val from = if (qualified) s"$dateDiffIndex AS r" else dateDiffIndex + val sql = + s"SELECT ${q}sensor_id, $items, $expr AS x_inline FROM $from GROUP BY ${q}sensor_id$form" + val values = groups.map { case (s, ds) => + s -> (for { + first <- maxOf(ds) + other <- secondValue(ds) + } yield spelling.days(first, other)) + }.toMap + filter match { + case None => DateDiffStatement(sql, Some(values), None) + case Some(f) => + DateDiffStatement( + sql, + None, + Some(values.collect { case (s, Some(v)) if f(v) => s }.toSet) + ) + } + } + } + + "DATEDIFF / DATE_DIFF / TIMESTAMPDIFF over aggregates" should + "answer per group, NULL where an operand has no value" in { + dateDiffGroupsLoaded + val population = dateDiffPopulation + // Non-vacuity, computed over the material: every spelling in both venues, the group with no + // date is NULL in every SELECT and kept by no HAVING, and the HAVINGs keep and drop groups. + population.size shouldBe dateDiffForms.size * 2 * 2 * 3 + population.foreach(st => withClue(s"[${st.sql}] ")(Parser(st.sql).isRight shouldBe true)) + population.flatMap(_.values).foreach(values => values("s1") shouldBe None) + population.flatMap(_.kept).foreach(kept => kept should not contain "s1") + population.flatMap(_.kept).exists(_.nonEmpty) shouldBe true + population.flatMap(_.kept).exists(_.size < dateDiffGroups.size - 1) shouldBe true + + def number(v: Any): Option[Double] = Option(v).map(_.toString.toDouble) + val outcomes: Seq[(DateDiffStatement, Either[String, String])] = population.map { st => + st -> gatewayRows(st.sql).map { rows => + (st.values, st.kept) match { + case (Some(expected), _) => + val actual = rows + .map(r => r.getOrElse("sensor_id", "?").toString -> r.get("x_inline").flatMap(number)) + .toMap + val wanted = expected.map { case (s, v) => s -> v.map(_.toDouble) } + if (actual == wanted) "" + else s"answered ${actual.toSeq.sortBy(_._1)}, expected ${wanted.toSeq.sortBy(_._1)}" + case (_, Some(expected)) => + val actual = rows.map(_.getOrElse("sensor_id", "?").toString).toSet + if (actual == expected) "" + else + s"kept ${actual.toSeq.sorted.mkString(",")}, expected ${expected.toSeq.sorted.mkString(",")}" + case _ => "no expectation" + } + } + } + val wrong = outcomes.collect { case (st, Right(diff)) if diff.nonEmpty => s"[${st.sql}] $diff" } + val errors = outcomes.collect { case (st, Left(error)) => s"[${st.sql}] $error" } + info( + s"DATEDIFF family over aggregates: ${population.size} statements; wrong: ${wrong.size}; " + + s"errors: ${errors.size}" + ) + wrong.foreach(info(_)) + errors.foreach(info(_)) + withClue( + s"${wrong.size} wrong statements, ${errors.size} errors:\n" + + (wrong.take(20) ++ errors.take(10)).mkString("\n") + "\n" + ) { + wrong shouldBe empty + errors shouldBe empty + } + } + // --------------------------------------------------------------------------------------------- // F2 -- a MATCH in HAVING over an aggregate, or over a column that is neither an aggregate nor a // GROUP BY key, is refused by name. Before, it was DROPPED: the group filter has no MATCH, the