diff --git a/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala b/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala index 29baa821..d441615c 100644 --- a/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala +++ b/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala @@ -759,7 +759,11 @@ object ElasticAggregation { // No filtering at this level is `None`, never a placeholder to strip out of a script: a // rendered comparison can hold the placeholder's text (`params.max_c1 == 1`). - val fullScript = MetricSelectorScript.selectorScript(criteria).map(_.trim).getOrElse("") + // + // F1 -- the NULL-AWARE script: an aggregate of a group that has none of its values reads as + // NULL, as SELECT returns it. The ONE function `Having.script` reads too (#292). + val fullScript = + MetricSelectorScript.nullAwareSelectorScript(criteria).map(_.trim).getOrElse("") // println(s"[DEBUG] currentNestedPath = $currentNestedPath") // println(s"[DEBUG] fullScript (complete) = $fullScript") diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala index b42a3a49..71131563 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala @@ -77,7 +77,14 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { """doc['createdAt'].value.toInstant().atZone(ZoneId.of('Z')).get(ChronoField.YEAR)); param1"}}},""", """"count_x":{"value_count":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"y":"y","count_x":"count_x"},""", - """"script":{"source":"(params.y == null ? false : (params.y > 2020)) && """, + """"script":{"source":"""", + """(((def) (params.y == null""", + """ || Double.isNaN(params.y)""", + """ || Double.isInfinite(params.y) ? null : params.y)) == null ? false : """, + """(((def) (params.y == null""", + """ || Double.isNaN(params.y)""", + """ || Double.isInfinite(params.y) ? null : params.y)) > 2020))""", + """ && """, """(params.count_x == null ? false : (params.count_x > 1))"}}}}}}}""" ).mkString } @@ -90,7 +97,12 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { ""","aggs":{"count_x":{"value_count":{"field":"x"}},"max_x":{"max":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x","max_x":"max_x"},""", """"script":{"source":"(params.count_x == null ? false : (params.count_x > 5)) && """, - """(params.max_x == null ? false : (params.max_x > 3))"}}}}}}}""" + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == null ? false : """, + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) > 3))"}}}}}}}""" ).mkString } @@ -104,7 +116,13 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { """"aggs":{"c":{"value_count":{"field":"x"}},"max_x":{"max":{"field":"x"}},""", """"min_x":{"min":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", - """"script":{"source":"(params.max_x == null ? false : (params.max_x > 3))"}}}}}}}""" + """"script":{"source":"""", + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == null ? false : """, + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) > 3))"}}}}}}}""" ).mkString } @@ -129,7 +147,13 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"max_profile_age":{"max":{"field":"profile.age"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_profile_age":"max_profile_age"},""", - """"script":{"source":"(params.max_profile_age == null ? false : (params.max_profile_age > 30))"}}}}}}}""" + """"script":{"source":"""", + """(((def) (params.max_profile_age == null""", + """ || Double.isNaN(params.max_profile_age)""", + """ || Double.isInfinite(params.max_profile_age) ? null : params.max_profile_age)) == null ? false : """, + """(((def) (params.max_profile_age == null""", + """ || Double.isNaN(params.max_profile_age)""", + """ || Double.isInfinite(params.max_profile_age) ? null : params.max_profile_age)) > 30))"}}}}}}}""" ).mkString } @@ -183,7 +207,13 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { ""","aggs":{"c":{"value_count":{"field":"x"}},""", maxYearCreatedAt, ""","having_filter":{"bucket_selector":{"buckets_path":{"max_year_createdat":"max_year_createdat"},""", - """"script":{"source":"(params.max_year_createdat == null ? false : (params.max_year_createdat > 2020))"}}}}}}}""" + """"script":{"source":"""", + """(((def) (params.max_year_createdat == null""", + """ || Double.isNaN(params.max_year_createdat)""", + """ || Double.isInfinite(params.max_year_createdat) ? null : params.max_year_createdat)) == null ? false : """, + """(((def) (params.max_year_createdat == null""", + """ || Double.isNaN(params.max_year_createdat)""", + """ || Double.isInfinite(params.max_year_createdat) ? null : params.max_year_createdat)) > 2020))"}}}}}}}""" ).mkString } @@ -198,7 +228,14 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { ""","aggs":{"c":{"value_count":{"field":"x"}},""", maxYearCreatedAt, ""","having_filter":{"bucket_selector":{"buckets_path":{"max_year_createdat":"max_year_createdat","c":"c"},""", - """"script":{"source":"(params.max_year_createdat == null ? false : (params.max_year_createdat > 2020)) || """, + """"script":{"source":"""", + """(((def) (params.max_year_createdat == null""", + """ || Double.isNaN(params.max_year_createdat)""", + """ || Double.isInfinite(params.max_year_createdat) ? null : params.max_year_createdat)) == null ? false : """, + """(((def) (params.max_year_createdat == null""", + """ || Double.isNaN(params.max_year_createdat)""", + """ || Double.isInfinite(params.max_year_createdat) ? null : params.max_year_createdat)) > 2020))""", + """ || """, """(params.c == null ? false : (params.c > 5))"}}}}}}}""" ).mkString } @@ -209,7 +246,18 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"max_a":{"max":{"field":"a"}},"min_b":{"min":{"field":"b"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_a":"max_a","min_b":"min_b"},""", - """"script":{"source":"(params.max_a == null || params.min_b == null ? false : (params.max_a > params.min_b))"}}}}}}}""" + """"script":{"source":"""", + """(((def) (params.max_a == null""", + """ || Double.isNaN(params.max_a)""", + """ || Double.isInfinite(params.max_a) ? null : params.max_a)) == null""", + """ || ((def) (params.min_b == null""", + """ || Double.isNaN(params.min_b)""", + """ || Double.isInfinite(params.min_b) ? null : params.min_b)) == null ? false : """, + """(((def) (params.max_a == null""", + """ || Double.isNaN(params.max_a)""", + """ || Double.isInfinite(params.max_a) ? null : params.max_a)) > ((def) (params.min_b == null""", + """ || Double.isNaN(params.min_b)""", + """ || Double.isInfinite(params.min_b) ? null : params.min_b))))"}}}}}}}""" ).mkString } @@ -224,8 +272,14 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { """"source":"def param1 = (doc['createdAt'].size() == 0 ? null : """, """doc['createdAt'].value.toInstant().atZone(ZoneId.of('Z')).truncatedTo(ChronoUnit.DAYS)); param1"}}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_date_trunc_createdat_day":"max_date_trunc_createdat_day"},""", - """"script":{"source":"(params.max_date_trunc_createdat_day == null ? false : """, - """(params.max_date_trunc_createdat_day > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z'))""", + """"script":{"source":"""", + """(((def) (params.max_date_trunc_createdat_day == null""", + """ || Double.isNaN(params.max_date_trunc_createdat_day)""", + """ || Double.isInfinite(params.max_date_trunc_createdat_day) ? null : params.max_date_trunc_createdat_day)) == null ? false : """, + """(((def) """, + """(params.max_date_trunc_createdat_day == null""", + """ || Double.isNaN(params.max_date_trunc_createdat_day)""", + """ || Double.isInfinite(params.max_date_trunc_createdat_day) ? null : params.max_date_trunc_createdat_day)) > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z'))""", """.minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))","params":{"__now__":1767139200000}}}}}}}}""" ).mkString } @@ -257,7 +311,16 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"max_x":{"max":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", - """"script":{"source":"(params.max_x == null ? false : (params.max_x == 1 || params.max_x == 2))"}}}}}}}""" + """"script":{"source":"""", + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == null ? false : """, + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == 1""", + """ || ((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == 2))"}}}}}}}""" ).mkString } @@ -267,7 +330,16 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"max_x":{"max":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", - """"script":{"source":"(params.max_x == null ? false : !(params.max_x == 1 || params.max_x == 2))"}}}}}}}""" + """"script":{"source":"""", + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == null ? false : !""", + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == 1""", + """ || ((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == 2))"}}}}}}}""" ).mkString } @@ -306,7 +378,12 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { ""","aggs":{"count_x":{"value_count":{"field":"x"}},"max_x":{"max":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x","max_x":"max_x"},""", """"script":{"source":"((params.count_x == null ? false : (params.count_x > 5))) && """, - """(params.max_x == null ? false : (params.max_x <= 3))"}}}}}}}""" + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == null ? false : """, + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) <= 3))"}}}}}}}""" ).mkString } @@ -316,7 +393,13 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"max_x":{"max":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", - """"script":{"source":"(params.max_x == null ? false : (params.max_x <= 3))"}}}}}}}""" + """"script":{"source":"""", + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == null ? false : """, + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) <= 3))"}}}}}}}""" ).mkString } @@ -329,7 +412,13 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { """"script":"params.max_x - params.min_x"}},""", """"max_x":{"max":{"field":"x"}},"min_x":{"min":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"d":"d"},""", - """"script":{"source":"(params.d == null ? false : (params.d > 3))"}}}}}}}""" + """"script":{"source":"""", + """(((def) (params.d == null""", + """ || Double.isNaN(params.d)""", + """ || Double.isInfinite(params.d) ? null : params.d)) == null ? false : """, + """(((def) (params.d == null""", + """ || Double.isNaN(params.d)""", + """ || Double.isInfinite(params.d) ? null : params.d)) > 3))"}}}}}}}""" ).mkString } @@ -344,7 +433,13 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { """"aggs":{"e.domain":{"terms":{"field":"emails.domain","size":65536,"min_doc_count":1},""", """"aggs":{"max_e_sent":{"max":{"field":"emails.sent"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_e_sent":"max_e_sent"},""", - """"script":{"source":"(params.max_e_sent == null ? false : (params.max_e_sent > """, + """"script":{"source":"""", + """(((def) (params.max_e_sent == null""", + """ || Double.isNaN(params.max_e_sent)""", + """ || Double.isInfinite(params.max_e_sent) ? null : params.max_e_sent)) == null ? false : """, + """(((def) (params.max_e_sent == null""", + """ || Double.isNaN(params.max_e_sent)""", + """ || Double.isInfinite(params.max_e_sent) ? null : params.max_e_sent)) > """, """ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS)""", """.toInstant().toEpochMilli()))","params":{"__now__":1767139200000}}}}}}}}}}""" ).mkString 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 9d46e30c..7dcc6d8b 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/HavingFunctionEmissionSpec.scala @@ -357,10 +357,13 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { // base: `= 1` read `params.max_c`, `= 10` read `params.max_c0`, and `IN (1, 2)` lost its first // member; on Elasticsearch 8.18.3 all three searches failed (`Cannot invoke // "Object.getClass()" because "value" is null`). + // F1: `max_c1` is read as NULL when it is null, NaN or infinite (an empty group). + val m = "((def) (params.max_c1 == null || Double.isNaN(params.max_c1) || " + + "Double.isInfinite(params.max_c1) ? null : params.max_c1))" Seq( - "MAX(c1) = 1" -> "(params.max_c1 == null ? false : (params.max_c1 == 1))", - "MAX(c1) = 10" -> "(params.max_c1 == null ? false : (params.max_c1 == 10))", - "MAX(c1) IN (1, 2)" -> "(params.max_c1 == null ? false : (params.max_c1 == 1 || params.max_c1 == 2))" + "MAX(c1) = 1" -> s"($m == null ? false : ($m == 1))", + "MAX(c1) = 10" -> s"($m == null ? false : ($m == 10))", + "MAX(c1) IN (1, 2)" -> s"($m == null ? false : ($m == 1 || $m == 2))" ).foreach { case (condition, script) => withClue(s"[$condition] ") { queryOf(s"SELECT g, COUNT(*) AS cnt FROM t GROUP BY g HAVING $condition") should include( @@ -420,8 +423,14 @@ 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 || params.max_x == null ? false : """, - """(params.c > Math.max(params.max_x, 0)))"}}}}}}}""" + """"script":{"source":"(params.c == null""", + """ || ((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.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)), 0)))"}}}}}}}""" ).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 88786ec5..d90e767f 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala +++ b/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala @@ -862,7 +862,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "max_price": "max_price" | }, | "script": { - | "source": "(params.min_price == null ? false : (params.min_price > 5.0)) && (params.max_price == null ? false : (params.max_price < 50.0))" + | "source": "(((def) (params.min_price == null || Double.isNaN(params.min_price) || Double.isInfinite(params.min_price) ? null : params.min_price)) == null ? false : (((def) (params.min_price == null || Double.isNaN(params.min_price) || Double.isInfinite(params.min_price) ? null : params.min_price)) > 5.0)) && (((def) (params.max_price == null || Double.isNaN(params.max_price) || Double.isInfinite(params.max_price) ? null : params.max_price)) == null ? false : (((def) (params.max_price == null || Double.isNaN(params.max_price) || Double.isInfinite(params.max_price) ? null : params.max_price)) < 50.0))" | } | } | } @@ -883,6 +883,14 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("<(\\d)", " < $1") .replaceAll(">(\\d)", " > $1") .replaceAll("\\?false:", " ? false : ") + .replace( + "((def)(params.min_price == null||Double.isNaN(params.min_price)||Double.isInfinite(params.min_price)?null:params.min_price))", + "((def) (params.min_price == null || Double.isNaN(params.min_price) || Double.isInfinite(params.min_price) ? null : params.min_price))" + ) + .replace( + "((def)(params.max_price == null||Double.isNaN(params.max_price)||Double.isInfinite(params.max_price)?null:params.max_price))", + "((def) (params.max_price == null || Double.isNaN(params.max_price) || Double.isInfinite(params.max_price) ? null : params.max_price))" + ) } @@ -1094,7 +1102,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "lastSeen": "lastSeen" | }, | "script": { - | "source": "(params.lastSeen == null ? false : (params.lastSeen > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))", + | "source": "(((def) (params.lastSeen == null || Double.isNaN(params.lastSeen) || Double.isInfinite(params.lastSeen) ? null : params.lastSeen)) == null ? false : (((def) (params.lastSeen == null || Double.isNaN(params.lastSeen) || Double.isInfinite(params.lastSeen) ? null : params.lastSeen)) > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))", | "params": { | "__now__": 1767139200000 | } @@ -1116,6 +1124,10 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll(">", " > ") .replaceAll(",ZoneId.of", ", ZoneId.of") .replaceAll("\\?false:", " ? false : ") + .replace( + "((def)(params.lastSeen == null||Double.isNaN(params.lastSeen)||Double.isInfinite(params.lastSeen)?null:params.lastSeen))", + "((def) (params.lastSeen == null || Double.isNaN(params.lastSeen) || Double.isInfinite(params.lastSeen) ? null : params.lastSeen))" + ) } it should "handle group by with having and date time functions" in { @@ -1167,7 +1179,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "lastSeen": "lastSeen" | }, | "script": { - | "source": "(params.cnt == null ? false : (params.cnt > 1)) && (params.lastSeen == null ? false : (params.lastSeen > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))", + | "source": "(params.cnt == null ? false : (params.cnt > 1)) && (((def) (params.lastSeen == null || Double.isNaN(params.lastSeen) || Double.isInfinite(params.lastSeen) ? null : params.lastSeen)) == null ? false : (((def) (params.lastSeen == null || Double.isNaN(params.lastSeen) || Double.isInfinite(params.lastSeen) ? null : params.lastSeen)) > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))", | "params": { | "__now__": 1767139200000 | } @@ -1191,6 +1203,10 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll(">", " > ") .replaceAll(",ZoneId.of", ", ZoneId.of") .replaceAll("\\?false:", " ? false : ") + .replace( + "((def)(params.lastSeen == null||Double.isNaN(params.lastSeen)||Double.isInfinite(params.lastSeen)?null:params.lastSeen))", + "((def) (params.lastSeen == null || Double.isNaN(params.lastSeen) || Double.isInfinite(params.lastSeen) ? null : params.lastSeen))" + ) } it should "handle group by index" in { @@ -1244,7 +1260,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "lastSeen": "lastSeen" | }, | "script": { - | "source": "(params.cnt == null ? false : (params.cnt > 1)) && (params.lastSeen == null ? false : (params.lastSeen > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))", + | "source": "(params.cnt == null ? false : (params.cnt > 1)) && (((def) (params.lastSeen == null || Double.isNaN(params.lastSeen) || Double.isInfinite(params.lastSeen) ? null : params.lastSeen)) == null ? false : (((def) (params.lastSeen == null || Double.isNaN(params.lastSeen) || Double.isInfinite(params.lastSeen) ? null : params.lastSeen)) > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))", | "params": { | "__now__": 1767139200000 | } @@ -1268,6 +1284,10 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll(">", " > ") .replaceAll(",ZoneId.of", ", ZoneId.of") .replaceAll("\\?false:", " ? false : ") + .replace( + "((def)(params.lastSeen == null||Double.isNaN(params.lastSeen)||Double.isInfinite(params.lastSeen)?null:params.lastSeen))", + "((def) (params.lastSeen == null || Double.isNaN(params.lastSeen) || Double.isInfinite(params.lastSeen) ? null : params.lastSeen))" + ) } it should "handle date_parse function" in { @@ -4278,7 +4298,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "avg_age": "avg_age" | }, | "script": { - | "source": "(params.__c2 == null ? false : (params.__c2 >= 1)) && (params.avg_age == null ? false : (params.avg_age > 25))" + | "source": "(params.__c2 == null ? false : (params.__c2 >= 1)) && (((def) (params.avg_age == null || Double.isNaN(params.avg_age) || Double.isInfinite(params.avg_age) ? null : params.avg_age)) == null ? false : (((def) (params.avg_age == null || Double.isNaN(params.avg_age) || Double.isInfinite(params.avg_age) ? null : params.avg_age)) > 25))" | } | } | } @@ -4295,6 +4315,10 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll(">=", " >= ") .replaceAll("(?)>(?!=)", " > ") .replaceAll("\\?false:", " ? false : ") + .replace( + "((def)(params.avg_age == null||Double.isNaN(params.avg_age)||Double.isInfinite(params.avg_age)?null:params.avg_age))", + "((def) (params.avg_age == null || Double.isNaN(params.avg_age) || Double.isInfinite(params.avg_age) ? null : params.avg_age))" + ) } it should "handle HAVING COUNT(*) only in HAVING clause not in SELECT" in { diff --git a/core/src/test/scala/app/softnetwork/elastic/client/HavingNullAwareReadSpec.scala b/core/src/test/scala/app/softnetwork/elastic/client/HavingNullAwareReadSpec.scala new file mode 100644 index 00000000..d6790d76 --- /dev/null +++ b/core/src/test/scala/app/softnetwork/elastic/client/HavingNullAwareReadSpec.scala @@ -0,0 +1,108 @@ +/* + * 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.client + +import app.softnetwork.elastic.sql.parser.Parser +import app.softnetwork.elastic.sql.query.{MetricSelectorScript, SingleSearch} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** F1 -- the null-aware `HAVING` selector and the rule the response parser answers NULL with. + * + * `ClientAggregation.nullOverEmptyInput` decides which aggregates SELECT returns as NULL over a + * group with no value; `MetricSelectorScript.nullAwareSelectorScript` decides which ones the + * `HAVING` selector reads as NULL when Elasticsearch hands it `NaN`. They are two answers to one + * question and live in two modules, so this spec -- the one place that sees both -- asserts they + * agree over EVERY aggregation type. + */ +class HavingNullAwareReadSpec extends AnyFlatSpec with Matchers { + + private def parsed(sql: String): SingleSearch = Parser(sql) match { + case Right(s: SingleSearch) => s + case other => fail(s"[$sql] expected a SingleSearch, got $other") + } + + /** How a SELECT publishes an aggregate of each type over `v` as `a`, and how a HAVING names it. + * `None`: a multi-valued type, which no comparison reads. + */ + private def spelling(t: AggregationType.AggregationType): Option[(String, String)] = { + def plain(fn: String) = Some(s"$fn(v) AS a" -> s"$fn(v)") + t match { + case AggregationType.Count => plain("COUNT") + case AggregationType.Min => plain("MIN") + case AggregationType.Max => plain("MAX") + case AggregationType.Avg => plain("AVG") + case AggregationType.Sum => plain("SUM") + case AggregationType.Stddev => plain("STDDEV") + case AggregationType.StddevSamp => plain("STDDEV_SAMP") + case AggregationType.StddevPop => plain("STDDEV_POP") + case AggregationType.Variance => plain("VARIANCE") + case AggregationType.VarSamp => plain("VAR_SAMP") + case AggregationType.VarPop => plain("VAR_POP") + case AggregationType.PercentileCont => + val p = "PERCENTILE_CONT(0.5) WITHIN GROUP (ORDER BY v)" + Some(s"$p AS a" -> p) + case AggregationType.PercentileDisc => + val p = "PERCENTILE_DISC(0.5) WITHIN GROUP (ORDER BY v)" + Some(s"$p AS a" -> p) + case AggregationType.FirstValue => + val f = "FIRST_VALUE(v) OVER (PARTITION BY g ORDER BY v)" + Some(s"$f AS a" -> f) + case AggregationType.LastValue => + val f = "LAST_VALUE(v) OVER (PARTITION BY g ORDER BY v)" + Some(s"$f AS a" -> f) + case AggregationType.BucketScript => Some("MAX(v) - MIN(v) AS a" -> "a") + case AggregationType.ArrayAgg | AggregationType.RowNumber | AggregationType.Rank | + AggregationType.DenseRank => + None + case other => fail(s"$other has no spelling here: classify it") + } + } + + private def nullOverEmptyInput(t: AggregationType.AggregationType): Boolean = + ClientAggregation( + aggName = "a", + aggType = t, + distinct = false, + sourceField = "v", + windowing = false, + bucketPath = "", + bucketRoot = "" + ).nullOverEmptyInput + + "the null-aware HAVING selector" should + "read as NULL exactly the aggregates SELECT answers NULL over no value" in { + val checked = AggregationType.values.toSeq.flatMap(t => spelling(t).map(t -> _)) + checked.map(_._1).filter(nullOverEmptyInput) should not be empty + checked.map(_._1).filterNot(nullOverEmptyInput) should not be empty + checked.foreach { case (t, (selectItem, operand)) => + val sql = s"SELECT g, $selectItem FROM t GROUP BY g HAVING $operand > 1" + withClue(s"[$t] [$sql] ") { + val search = parsed(sql) + // the spelling IS the type: the published column converts to it + implicitly[ClientAggregation](search.sqlAggregations("a")).aggType shouldBe t + val criteria = search.having.flatMap(_.criteria).get + val plain = MetricSelectorScript.selectorScript(criteria) + val nullAware = MetricSelectorScript.nullAwareSelectorScript(criteria) + plain.map(_.contains("params.a")) shouldBe Some(true) + val readAsNull = nullAware != plain + readAsNull shouldBe nullOverEmptyInput(t) + if (readAsNull) nullAware.get should include("Double.isNaN(params.a)") + } + } + } +} diff --git a/documentation/sql/dql_statements.md b/documentation/sql/dql_statements.md index f3c99c98..08f8aab7 100644 --- a/documentation/sql/dql_statements.md +++ b/documentation/sql/dql_statements.md @@ -845,8 +845,11 @@ ORDER BY COUNT(*) DESC; - Rejected with an explicit error: arithmetic over aggregates written inline in `HAVING` (`HAVING MAX(price) - MIN(price) > 10` โ€” alias it in `SELECT` and reference the alias), an aggregate function inside `WHERE` (use `HAVING`, and this covers a wrapped one such as - `WHERE ABS(COUNT(*)) > 1`), and an alias that names one aggregate in `SELECT` and a different one - in `HAVING` / `ORDER BY`. + `WHERE ABS(COUNT(*)) > 1`), an alias that names one aggregate in `SELECT` and a different one + in `HAVING` / `ORDER BY`, and a full-text `MATCH ... AGAINST` in `HAVING` over an aggregate + (`HAVING MATCH (MAX(title)) AGAINST ('x')`, or over its `SELECT` alias) or over a column that is + neither an aggregate nor a `GROUP BY` key (`GROUP BY city HAVING MATCH (title) AGAINST ('x')`) + โ€” put the `MATCH` in `WHERE`. - A **function of an aggregate** in `HAVING` is applied to the group, or the statement is rejected by name โ€” it is never ignored. `COALESCE`, `GREATEST`, `LEAST` and `SIGN` over an aggregate filter the groups (`HAVING COALESCE(COUNT(*), 0) > 30`, `HAVING GREATEST(MAX(price), 0) > 100`), on @@ -938,7 +941,14 @@ SELECT city, COUNT(*) AS cnt FROM dql_users GROUP BY city HAVING city <> 'Lyon'; - An `AND` across mechanisms is fine โ€” each stage applies its own half. - A group whose compared metric has no value (for instance `MAX(age)` over a group whose documents all lack `age`) never passes a `HAVING` comparison, in either direction: the generated filter - script null-checks every metric before comparing it. + script null-checks every metric before comparing it. For such a group `IS NULL` is true and + `IS NOT NULL` is false (written `ISNULL(MAX(age))` and `ISNOTNULL(MAX(age))` in `HAVING`). + +> ๐Ÿ”ด **Changed in 0.24.0 โ€” an empty group's aggregate is NULL in `HAVING`.** Before 0.24.0, `<>`, a +> negated comparison (`NOT MAX(age) = 30`, `NOT BETWEEN`, `NOT IN`), `IS [NOT] NULL` and `COALESCE` +> over the `MIN`, `MAX`, `AVG` or a percentile of a group with no value could keep or drop the wrong groups: +> the group filter was handed Elasticsearch's placeholder for an aggregate over no value (`NaN`), +> not NULL. #### GROUP BY without an aggregate diff --git a/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala b/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala index 232aaf99..5eccb1ea 100644 --- a/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala +++ b/es6/bridge/src/main/scala/app/softnetwork/elastic/sql/bridge/ElasticAggregation.scala @@ -755,7 +755,11 @@ object ElasticAggregation { // No filtering at this level is `None`, never a placeholder to strip out of a script: a // rendered comparison can hold the placeholder's text (`params.max_c1 == 1`). - val fullScript = MetricSelectorScript.selectorScript(criteria).map(_.trim).getOrElse("") + // + // F1 -- the NULL-AWARE script: an aggregate of a group that has none of its values reads as + // NULL, as SELECT returns it. The ONE function `Having.script` reads too (#292). + val fullScript = + MetricSelectorScript.nullAwareSelectorScript(criteria).map(_.trim).getOrElse("") // println(s"[DEBUG] currentNestedPath = $currentNestedPath") // println(s"[DEBUG] fullScript (complete) = $fullScript") diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala index dae03ac6..fe6dc810 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala +++ b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/AggregationNamingSpec.scala @@ -77,7 +77,14 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { """doc['createdAt'].value.toInstant().atZone(ZoneId.of('Z')).get(ChronoField.YEAR)); param1"}}},""", """"count_x":{"value_count":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"y":"y","count_x":"count_x"},""", - """"script":{"source":"(params.y == null ? false : (params.y > 2020)) && """, + """"script":{"source":"""", + """(((def) (params.y == null""", + """ || Double.isNaN(params.y)""", + """ || Double.isInfinite(params.y) ? null : params.y)) == null ? false : """, + """(((def) (params.y == null""", + """ || Double.isNaN(params.y)""", + """ || Double.isInfinite(params.y) ? null : params.y)) > 2020))""", + """ && """, """(params.count_x == null ? false : (params.count_x > 1))"}}}}}}}""" ).mkString } @@ -90,7 +97,12 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { ""","aggs":{"count_x":{"value_count":{"field":"x"}},"max_x":{"max":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x","max_x":"max_x"},""", """"script":{"source":"(params.count_x == null ? false : (params.count_x > 5)) && """, - """(params.max_x == null ? false : (params.max_x > 3))"}}}}}}}""" + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == null ? false : """, + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) > 3))"}}}}}}}""" ).mkString } @@ -104,7 +116,13 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { """"aggs":{"c":{"value_count":{"field":"x"}},"max_x":{"max":{"field":"x"}},""", """"min_x":{"min":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", - """"script":{"source":"(params.max_x == null ? false : (params.max_x > 3))"}}}}}}}""" + """"script":{"source":"""", + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == null ? false : """, + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) > 3))"}}}}}}}""" ).mkString } @@ -129,7 +147,13 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"max_profile_age":{"max":{"field":"profile.age"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_profile_age":"max_profile_age"},""", - """"script":{"source":"(params.max_profile_age == null ? false : (params.max_profile_age > 30))"}}}}}}}""" + """"script":{"source":"""", + """(((def) (params.max_profile_age == null""", + """ || Double.isNaN(params.max_profile_age)""", + """ || Double.isInfinite(params.max_profile_age) ? null : params.max_profile_age)) == null ? false : """, + """(((def) (params.max_profile_age == null""", + """ || Double.isNaN(params.max_profile_age)""", + """ || Double.isInfinite(params.max_profile_age) ? null : params.max_profile_age)) > 30))"}}}}}}}""" ).mkString } @@ -183,7 +207,13 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { ""","aggs":{"c":{"value_count":{"field":"x"}},""", maxYearCreatedAt, ""","having_filter":{"bucket_selector":{"buckets_path":{"max_year_createdat":"max_year_createdat"},""", - """"script":{"source":"(params.max_year_createdat == null ? false : (params.max_year_createdat > 2020))"}}}}}}}""" + """"script":{"source":"""", + """(((def) (params.max_year_createdat == null""", + """ || Double.isNaN(params.max_year_createdat)""", + """ || Double.isInfinite(params.max_year_createdat) ? null : params.max_year_createdat)) == null ? false : """, + """(((def) (params.max_year_createdat == null""", + """ || Double.isNaN(params.max_year_createdat)""", + """ || Double.isInfinite(params.max_year_createdat) ? null : params.max_year_createdat)) > 2020))"}}}}}}}""" ).mkString } @@ -198,7 +228,14 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { ""","aggs":{"c":{"value_count":{"field":"x"}},""", maxYearCreatedAt, ""","having_filter":{"bucket_selector":{"buckets_path":{"max_year_createdat":"max_year_createdat","c":"c"},""", - """"script":{"source":"(params.max_year_createdat == null ? false : (params.max_year_createdat > 2020)) || """, + """"script":{"source":"""", + """(((def) (params.max_year_createdat == null""", + """ || Double.isNaN(params.max_year_createdat)""", + """ || Double.isInfinite(params.max_year_createdat) ? null : params.max_year_createdat)) == null ? false : """, + """(((def) (params.max_year_createdat == null""", + """ || Double.isNaN(params.max_year_createdat)""", + """ || Double.isInfinite(params.max_year_createdat) ? null : params.max_year_createdat)) > 2020))""", + """ || """, """(params.c == null ? false : (params.c > 5))"}}}}}}}""" ).mkString } @@ -209,7 +246,18 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"max_a":{"max":{"field":"a"}},"min_b":{"min":{"field":"b"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_a":"max_a","min_b":"min_b"},""", - """"script":{"source":"(params.max_a == null || params.min_b == null ? false : (params.max_a > params.min_b))"}}}}}}}""" + """"script":{"source":"""", + """(((def) (params.max_a == null""", + """ || Double.isNaN(params.max_a)""", + """ || Double.isInfinite(params.max_a) ? null : params.max_a)) == null""", + """ || ((def) (params.min_b == null""", + """ || Double.isNaN(params.min_b)""", + """ || Double.isInfinite(params.min_b) ? null : params.min_b)) == null ? false : """, + """(((def) (params.max_a == null""", + """ || Double.isNaN(params.max_a)""", + """ || Double.isInfinite(params.max_a) ? null : params.max_a)) > ((def) (params.min_b == null""", + """ || Double.isNaN(params.min_b)""", + """ || Double.isInfinite(params.min_b) ? null : params.min_b))))"}}}}}}}""" ).mkString } @@ -224,8 +272,14 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { """"source":"def param1 = (doc['createdAt'].size() == 0 ? null : """, """doc['createdAt'].value.toInstant().atZone(ZoneId.of('Z')).truncatedTo(ChronoUnit.DAYS)); param1"}}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_date_trunc_createdat_day":"max_date_trunc_createdat_day"},""", - """"script":{"source":"(params.max_date_trunc_createdat_day == null ? false : """, - """(params.max_date_trunc_createdat_day > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z'))""", + """"script":{"source":"""", + """(((def) (params.max_date_trunc_createdat_day == null""", + """ || Double.isNaN(params.max_date_trunc_createdat_day)""", + """ || Double.isInfinite(params.max_date_trunc_createdat_day) ? null : params.max_date_trunc_createdat_day)) == null ? false : """, + """(((def) """, + """(params.max_date_trunc_createdat_day == null""", + """ || Double.isNaN(params.max_date_trunc_createdat_day)""", + """ || Double.isInfinite(params.max_date_trunc_createdat_day) ? null : params.max_date_trunc_createdat_day)) > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z'))""", """.minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))","params":{"__now__":1767139200000}}}}}}}}""" ).mkString } @@ -257,7 +311,16 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"max_x":{"max":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", - """"script":{"source":"(params.max_x == null ? false : (params.max_x == 1 || params.max_x == 2))"}}}}}}}""" + """"script":{"source":"""", + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == null ? false : """, + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == 1""", + """ || ((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == 2))"}}}}}}}""" ).mkString } @@ -267,7 +330,16 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"max_x":{"max":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", - """"script":{"source":"(params.max_x == null ? false : !(params.max_x == 1 || params.max_x == 2))"}}}}}}}""" + """"script":{"source":"""", + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == null ? false : !""", + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == 1""", + """ || ((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == 2))"}}}}}}}""" ).mkString } @@ -306,7 +378,12 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { ""","aggs":{"count_x":{"value_count":{"field":"x"}},"max_x":{"max":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"count_x":"count_x","max_x":"max_x"},""", """"script":{"source":"((params.count_x == null ? false : (params.count_x > 5))) && """, - """(params.max_x == null ? false : (params.max_x <= 3))"}}}}}}}""" + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == null ? false : """, + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) <= 3))"}}}}}}}""" ).mkString } @@ -316,7 +393,13 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { terms, ""","aggs":{"max_x":{"max":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_x":"max_x"},""", - """"script":{"source":"(params.max_x == null ? false : (params.max_x <= 3))"}}}}}}}""" + """"script":{"source":"""", + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) == null ? false : """, + """(((def) (params.max_x == null""", + """ || Double.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)) <= 3))"}}}}}}}""" ).mkString } @@ -329,7 +412,13 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { """"script":"params.max_x - params.min_x"}},""", """"max_x":{"max":{"field":"x"}},"min_x":{"min":{"field":"x"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"d":"d"},""", - """"script":{"source":"(params.d == null ? false : (params.d > 3))"}}}}}}}""" + """"script":{"source":"""", + """(((def) (params.d == null""", + """ || Double.isNaN(params.d)""", + """ || Double.isInfinite(params.d) ? null : params.d)) == null ? false : """, + """(((def) (params.d == null""", + """ || Double.isNaN(params.d)""", + """ || Double.isInfinite(params.d) ? null : params.d)) > 3))"}}}}}}}""" ).mkString } @@ -344,7 +433,13 @@ class AggregationNamingSpec extends AnyFlatSpec with Matchers { """"aggs":{"e.domain":{"terms":{"field":"emails.domain","size":65536,"min_doc_count":1},""", """"aggs":{"max_e_sent":{"max":{"field":"emails.sent"}},""", """"having_filter":{"bucket_selector":{"buckets_path":{"max_e_sent":"max_e_sent"},""", - """"script":{"source":"(params.max_e_sent == null ? false : (params.max_e_sent > """, + """"script":{"source":"""", + """(((def) (params.max_e_sent == null""", + """ || Double.isNaN(params.max_e_sent)""", + """ || Double.isInfinite(params.max_e_sent) ? null : params.max_e_sent)) == null ? false : """, + """(((def) (params.max_e_sent == null""", + """ || Double.isNaN(params.max_e_sent)""", + """ || Double.isInfinite(params.max_e_sent) ? null : params.max_e_sent)) > """, """ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS)""", """.toInstant().toEpochMilli()))","params":{"__now__":1767139200000}}}}}}}}}}""" ).mkString 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 77805bfe..006af302 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 @@ -371,10 +371,13 @@ class HavingFunctionEmissionSpec extends AnyFlatSpec with Matchers { // base: `= 1` read `params.max_c`, `= 10` read `params.max_c0`, and `IN (1, 2)` lost its first // member; on Elasticsearch 8.18.3 all three searches failed (`Cannot invoke // "Object.getClass()" because "value" is null`). + // F1: `max_c1` is read as NULL when it is null, NaN or infinite (an empty group). + val m = "((def) (params.max_c1 == null || Double.isNaN(params.max_c1) || " + + "Double.isInfinite(params.max_c1) ? null : params.max_c1))" Seq( - "MAX(c1) = 1" -> "(params.max_c1 == null ? false : (params.max_c1 == 1))", - "MAX(c1) = 10" -> "(params.max_c1 == null ? false : (params.max_c1 == 10))", - "MAX(c1) IN (1, 2)" -> "(params.max_c1 == null ? false : (params.max_c1 == 1 || params.max_c1 == 2))" + "MAX(c1) = 1" -> s"($m == null ? false : ($m == 1))", + "MAX(c1) = 10" -> s"($m == null ? false : ($m == 10))", + "MAX(c1) IN (1, 2)" -> s"($m == null ? false : ($m == 1 || $m == 2))" ).foreach { case (condition, script) => withClue(s"[$condition] ") { queryOf(s"SELECT g, COUNT(*) AS cnt FROM t GROUP BY g HAVING $condition") should include( @@ -435,8 +438,14 @@ 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 || params.max_x == null ? false : """, - """(params.c > Math.max(params.max_x, 0)))"}}}}}}}""" + """"script":{"source":"(params.c == null""", + """ || ((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.isNaN(params.max_x)""", + """ || Double.isInfinite(params.max_x) ? null : params.max_x)), 0)))"}}}}}}}""" ).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 345c6d96..30b47208 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 @@ -862,7 +862,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "max_price": "max_price" | }, | "script": { - | "source": "(params.min_price == null ? false : (params.min_price > 5.0)) && (params.max_price == null ? false : (params.max_price < 50.0))" + | "source": "(((def) (params.min_price == null || Double.isNaN(params.min_price) || Double.isInfinite(params.min_price) ? null : params.min_price)) == null ? false : (((def) (params.min_price == null || Double.isNaN(params.min_price) || Double.isInfinite(params.min_price) ? null : params.min_price)) > 5.0)) && (((def) (params.max_price == null || Double.isNaN(params.max_price) || Double.isInfinite(params.max_price) ? null : params.max_price)) == null ? false : (((def) (params.max_price == null || Double.isNaN(params.max_price) || Double.isInfinite(params.max_price) ? null : params.max_price)) < 50.0))" | } | } | } @@ -883,6 +883,14 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll("<(\\d)", " < $1") .replaceAll(">(\\d)", " > $1") .replaceAll("\\?false:", " ? false : ") + .replace( + "((def)(params.min_price == null||Double.isNaN(params.min_price)||Double.isInfinite(params.min_price)?null:params.min_price))", + "((def) (params.min_price == null || Double.isNaN(params.min_price) || Double.isInfinite(params.min_price) ? null : params.min_price))" + ) + .replace( + "((def)(params.max_price == null||Double.isNaN(params.max_price)||Double.isInfinite(params.max_price)?null:params.max_price))", + "((def) (params.max_price == null || Double.isNaN(params.max_price) || Double.isInfinite(params.max_price) ? null : params.max_price))" + ) } @@ -1094,7 +1102,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "lastSeen": "lastSeen" | }, | "script": { - | "source": "(params.lastSeen == null ? false : (params.lastSeen > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))", + | "source": "(((def) (params.lastSeen == null || Double.isNaN(params.lastSeen) || Double.isInfinite(params.lastSeen) ? null : params.lastSeen)) == null ? false : (((def) (params.lastSeen == null || Double.isNaN(params.lastSeen) || Double.isInfinite(params.lastSeen) ? null : params.lastSeen)) > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))", | "params": { | "__now__": 1767139200000 | } @@ -1116,6 +1124,10 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll(">", " > ") .replaceAll(",ZoneId.of", ", ZoneId.of") .replaceAll("\\?false:", " ? false : ") + .replace( + "((def)(params.lastSeen == null||Double.isNaN(params.lastSeen)||Double.isInfinite(params.lastSeen)?null:params.lastSeen))", + "((def) (params.lastSeen == null || Double.isNaN(params.lastSeen) || Double.isInfinite(params.lastSeen) ? null : params.lastSeen))" + ) } it should "handle group by with having and date time functions" in { @@ -1167,7 +1179,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "lastSeen": "lastSeen" | }, | "script": { - | "source": "(params.cnt == null ? false : (params.cnt > 1)) && (params.lastSeen == null ? false : (params.lastSeen > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))", + | "source": "(params.cnt == null ? false : (params.cnt > 1)) && (((def) (params.lastSeen == null || Double.isNaN(params.lastSeen) || Double.isInfinite(params.lastSeen) ? null : params.lastSeen)) == null ? false : (((def) (params.lastSeen == null || Double.isNaN(params.lastSeen) || Double.isInfinite(params.lastSeen) ? null : params.lastSeen)) > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))", | "params": { | "__now__": 1767139200000 | } @@ -1191,6 +1203,10 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll(">", " > ") .replaceAll(",ZoneId.of", ", ZoneId.of") .replaceAll("\\?false:", " ? false : ") + .replace( + "((def)(params.lastSeen == null||Double.isNaN(params.lastSeen)||Double.isInfinite(params.lastSeen)?null:params.lastSeen))", + "((def) (params.lastSeen == null || Double.isNaN(params.lastSeen) || Double.isInfinite(params.lastSeen) ? null : params.lastSeen))" + ) } it should "handle group by index" in { @@ -1244,7 +1260,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "lastSeen": "lastSeen" | }, | "script": { - | "source": "(params.cnt == null ? false : (params.cnt > 1)) && (params.lastSeen == null ? false : (params.lastSeen > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))", + | "source": "(params.cnt == null ? false : (params.cnt > 1)) && (((def) (params.lastSeen == null || Double.isNaN(params.lastSeen) || Double.isInfinite(params.lastSeen) ? null : params.lastSeen)) == null ? false : (((def) (params.lastSeen == null || Double.isNaN(params.lastSeen) || Double.isInfinite(params.lastSeen) ? null : params.lastSeen)) > ZonedDateTime.ofInstant(Instant.ofEpochMilli(params.__now__), ZoneId.of('Z')).minus(7, ChronoUnit.DAYS).toInstant().toEpochMilli()))", | "params": { | "__now__": 1767139200000 | } @@ -1268,6 +1284,10 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll(">", " > ") .replaceAll(",ZoneId.of", ", ZoneId.of") .replaceAll("\\?false:", " ? false : ") + .replace( + "((def)(params.lastSeen == null||Double.isNaN(params.lastSeen)||Double.isInfinite(params.lastSeen)?null:params.lastSeen))", + "((def) (params.lastSeen == null || Double.isNaN(params.lastSeen) || Double.isInfinite(params.lastSeen) ? null : params.lastSeen))" + ) } it should "handle date_parse function" in { @@ -4262,7 +4282,7 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { | "avg_age": "avg_age" | }, | "script": { - | "source": "(params.__c2 == null ? false : (params.__c2 >= 1)) && (params.avg_age == null ? false : (params.avg_age > 25))" + | "source": "(params.__c2 == null ? false : (params.__c2 >= 1)) && (((def) (params.avg_age == null || Double.isNaN(params.avg_age) || Double.isInfinite(params.avg_age) ? null : params.avg_age)) == null ? false : (((def) (params.avg_age == null || Double.isNaN(params.avg_age) || Double.isInfinite(params.avg_age) ? null : params.avg_age)) > 25))" | } | } | } @@ -4279,6 +4299,10 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers { .replaceAll(">=", " >= ") .replaceAll("(?)>(?!=)", " > ") .replaceAll("\\?false:", " ? false : ") + .replace( + "((def)(params.avg_age == null||Double.isNaN(params.avg_age)||Double.isInfinite(params.avg_age)?null:params.avg_age))", + "((def) (params.avg_age == null || Double.isNaN(params.avg_age) || Double.isInfinite(params.avg_age) ? null : params.avg_age))" + ) } it should "handle HAVING COUNT(*) only in HAVING clause not in SELECT" in { 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 b9497f92..3544194f 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 @@ -17,6 +17,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.matching.Regex @@ -390,8 +391,124 @@ object MetricSelectorScript { import MetricSelector._ + /** The `bucket_selector` source of a `HAVING` criterion that reads every aggregate exactly as + * SELECT returns it, or `None` when there is nothing to filter. Throws exactly where + * [[metricSelector]] does. + * + * ๐Ÿ”ด ONE function for every consumer (#292): the bridge's `metricSelectorForBucket` (both + * bridges) and `Having.script` -- which a materialized view's transform `bucket_selector` is + * built from -- read it, so a group filter and a view's filter cannot disagree about the value + * of an empty group's aggregate. + * + * ๐Ÿ”ด Why [[selectorScript]] is not enough (F1). In a group where NO document has the aggregated + * column, SELECT answers NULL for every aggregate of `ClientAggregation.nullOverEmptyInput` + * (rule #336: `MIN`, `MAX`, `AVG`, the percentiles, the STDDEV / VARIANCE family, `FIRST_VALUE` + * / `LAST_VALUE`, and `bucket_script` arithmetic over them), but a `bucket_selector` never + * receives that NULL: under the default gap policy Elasticsearch hands it `NaN` in place of the + * metric's `-Infinity` / `+Infinity` / `NaN` sentinel. So the `params.x == null` guards never + * fired and every comparison ran on `NaN`. MEASURED on Elasticsearch 8.18.3: `MAX(v) <> 1`, `NOT + * MAX(v) = 1`, `MAX(v) NOT IN (1, 2)` and `ISNOTNULL(MAX(v))` KEPT the group, `ISNULL(MAX(v))` + * dropped it, and so did `COALESCE(MAX(v), 5) > 1`. + * + * Every read of such a metric therefore becomes [[nullAwareRead]]: NULL when the value is null, + * `NaN` or infinite. Exact, not a heuristic: Elasticsearch 6.8 to 9.0 index only finite numbers, + * so no group's real aggregate is `NaN` or infinite, and SELECT answers NULL for exactly those + * values. COUNT and SUM are read as before -- 0 and `0.0` over no value, never NULL. Nothing is + * added to the request: no hidden aggregation, and no `buckets_path` variable -- the read uses + * the metric's own `params.`, so a consumer that derives its `buckets_path` from the + * clause (the materialized view's `extractAggregatePaths`) stays valid. + * + * 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. + */ + def nullAwareSelectorScript(expr: Criteria): Option[String] = + selectorScript(expr).map { script => + val nullable = bucketMetricsOf(expr).filter(nullOverEmptyInput).map(_.metricPathKey).toSet + if (nullable.isEmpty) script else readAsSelectReturns(script, nullable) + } + + /** How the selector reads the metric it is handed as `params.` when SELECT answers NULL for + * that metric over a group with no value: NULL when the value is null, `NaN` or infinite. + * + * The `(def)` cast is load-bearing: without it Painless types the conditional from its position, + * and `Math.max((... ? null : params.max_v), 0)` fails to compile (`Cannot cast null to a + * primitive type [double]`, MEASURED). `Double.isNaN` and `Double.isInfinite` are whitelisted on + * every supported major (6.8 to 9.0), and the null test comes first: unboxing a null throws. + */ + private def nullAwareRead(name: String): String = { + val param = s"params.$name" + 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 + * aggregation type. + */ + private def nullOverEmptyInput(metric: Identifier): Boolean = + metric.aggregateFunction match { + case Some(af) => nullOverEmptyInput(af) + // `Identifier.bucketMetrics`' second arm: the alias of a SELECT `bucket_script` item, + // arithmetic over aggregates -- NULL whenever an operand is + case None => metric.hasAggregation + } + + /** Deliberately a total match, with no default arm: a new aggregate must be classified here. */ + private def nullOverEmptyInput(af: AggregateFunction): Boolean = af match { + // 0 and 0.0 over no value (#336), never NULL + case COUNT | SUM | _: CountAgg | _: SumAgg => false + // multi-valued: no comparison reads them + case _: ArrayAgg | _: RankingWindow => false + case MIN | MAX | AVG | _: MinAgg | _: MaxAgg | _: AvgAgg => true + case STDDEV | STDDEV_POP | STDDEV_SAMP | VARIANCE | VAR_POP | VAR_SAMP | _: ExtendedStatsAgg => + true + case PERCENTILE_CONT | PERCENTILE_DISC | _: PercentileAgg => true + case _: FirstValue | _: LastValue => true + case _: BucketScriptAggregation => true + } + + /** `script` with every read of a `nullable` metric turned into its [[nullAwareRead]]. String + * literals are skipped. + */ + private def readAsSelectReturns(script: String, nullable: Set[String]): String = { + val out = new java.lang.StringBuilder(script.length * 2) + var last = 0 + MetricRead.findAllMatchIn(blankStringLiterals(script)).foreach { m => + if (nullable.contains(m.group(1))) { + out.append(script, last, m.start).append(nullAwareRead(m.group(1))) + last = m.end + } + } + out.append(script, last, script.length).toString + } + + /** One `params.` read, the name taken whole. */ + private val MetricRead: Regex = """(? if (unrepresentable.nonEmpty) None else { // `None` when there is nothing to filter -- never a placeholder stripped out of the script, // which also ate `params.max_c1 == 1` (see `MetricSelectorScript.selectorScript`). - MetricSelectorScript.selectorScript(criteria).map(_.trim).filter(_.nonEmpty) + MetricSelectorScript.nullAwareSelectorScript(criteria).map(_.trim).filter(_.nonEmpty) } } } 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 29986131..0cb891ea 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,7 @@ 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.{ConditionalFunction, IsNotNull, IsNull} +import app.softnetwork.elastic.sql.function.cond.{Coalesce, ConditionalFunction, IsNotNull, IsNull} import app.softnetwork.elastic.sql.function.convert.Conversion import app.softnetwork.elastic.sql.function.geo.Distance import app.softnetwork.elastic.sql.parser.Validator @@ -1355,16 +1355,17 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { // name), kept total so a grammar widening cannot ship it. case IS_NULL => s"$param == null" case IS_NOT_NULL => s"$param != null" - case _ => - val guard = bucketMetrics.map(id => s"${id.metricParam} == null").mkString(" || ") + case _ => + // Never empty: the left operand is a metric, and nothing here handles its NULL. + val guard = bucketPipelineGuard(None).mkString(" || ") s"($guard ? false : $painlessNot(${bucketPipelineCheck(param)}))" } } /** Every metric THIS predicate reads, left operand and right operand alike, deduplicated and in - * order -- the guard set of [[bucketPipelinePainless]] and of [[functionBucketPipelinePainless]] - * alike, and the same derivation `Criteria.extractAggregationFields` creates the aggregations - * from and `extractAllMetricsPath` publishes. + * order -- 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 @@ -1390,7 +1391,8 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { * proved to be a single boolean expression ([[MetricSelectorScript.representable]]) and once * every metric it dereferences is null-guarded. * - * ๐Ÿ”ด The guard is added only for a metric the rendering does not already test. `COALESCE` exists + * ๐Ÿ”ด The guard is the one [[bucketPipelineGuard]] derives: a metric the rendering does not + * already test, and a `COALESCE` on its RESULT, never on its arguments. `COALESCE` exists * precisely to decide what a null means, and forcing `false` on it would make `COALESCE(MAX(x), * 99) > 1` answer `false` where SQL says `true`. A rendering that does NOT test the metric * (`Math.abs(params.c)`) would throw on a null instead, and SQL's answer for it is UNKNOWN -- @@ -1399,13 +1401,115 @@ sealed trait Expression extends FunctionChain with ElasticFilter with Criteria { */ private[query] def functionBucketPipelinePainless: String = { val rendering = painless(None) - val unguarded = bucketMetrics.filterNot { id => - rendering.contains(s"${id.metricParam} == null") || - rendering.contains(s"${id.metricParam} != null") + val guard = bucketPipelineGuard(Some(rendering)) + if (guard.isEmpty) rendering + else s"(${guard.mkString(" || ")} ? false : ($rendering))" + } + + /** The terms of THIS predicate's null guard, in order. The bucket-pipeline rendering is `( + * ? false : ())`, the terms joined by `||`, so the comparison is UNKNOWN -- `false` + * to a group filter -- as soon as one term holds. The ONE derivation behind + * [[bucketPipelinePainless]] and [[functionBucketPipelinePainless]]. + * + * A metric is guarded on ITSELF, `params. == null`: its NULL makes the comparison + * UNKNOWN, and a rendering that dereferences it (`Math.abs(params.c)`) would throw on it. Given + * the context-free `rendering` the guard is placed around, a term that rendering already carries + * is not added again: `ISNULL(MAX(x))` IS the test of its metric, and the rendering of a + * predicate with an aggregate on its left is the one [[bucketPipelinePainless]] renders, guarded + * already. + * + * ๐Ÿ”ด A `COALESCE` is guarded on its RESULT ([[nullGuardTerms]]). It decides what a NULL argument + * means, so the comparison is UNKNOWN only when its VALUE is NULL -- when every argument is. + * 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. + */ + private[query] def bucketPipelineGuard(rendering: Option[String]): Seq[String] = { + val operands = identifier +: maybeValue.toSeq.collect { case id: Identifier => id } + if (operands.exists(readsCoalesce)) { + 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 + nullGuardTerms(operand, nullHandled = nullTest && i == 0) + }.distinct + rendering.fold(terms)(r => terms.filterNot(r.contains)) + } else + rendering + .fold(bucketMetrics)(r => + bucketMetrics.filterNot { id => + r.contains(s"${id.metricParam} == null") || r.contains(s"${id.metricParam} != null") + } + ) + .map(id => s"${id.metricParam} == null") + } + + /** 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. + * + * 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. + */ + 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(single: Identifier) => nullGuardTerms(single, nullHandled) + case values => + val arguments = values + .collect { case argument: Identifier => argument } + .flatMap(nullGuardTerms(_, nullHandled = true)) + val result = + if (nullHandled || values.exists(nonNullLiteral)) Nil + else Seq(s"${operand.painless(None)} == null") + arguments ++ result + } + case List(f: FunctionN[_, _]) if readsCoalesce(operand) => + f.args + .collect { case argument: Identifier => argument } + .flatMap(nullGuardTerms(_, nullHandled = false)) + case _ if nullHandled && isMetric(operand) => Nil + case _ => operand.bucketMetrics.map(m => s"${m.metricParam} == null") + } + + /** An operand a bucket pipeline reads as ONE `params.`: an aggregate, or the alias of a + * SELECT `bucket_script` item. + */ + private def isMetric(operand: Identifier): Boolean = operand.bucketMetrics match { + case Seq(metric) => metric eq operand + case _ => false + } + + /** Does this operand read a `COALESCE` -- its own, or one among its functions' arguments? */ + private def readsCoalesce(operand: Identifier): Boolean = + !isMetric(operand) && operand.functions.exists { + case _: Coalesce => true + case f: FunctionN[_, _] => + f.args.exists { + case argument: Identifier => readsCoalesce(argument) + case _ => false + } + case _ => false } - if (unguarded.isEmpty) rendering - else - s"(${unguarded.map(id => s"${id.metricParam} == null").mkString(" || ")} ? false : ($rendering))" + + /** A `COALESCE` argument that is a literal other than `NULL`: that `COALESCE` 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) => + wrapped.functions match { + case List(literal: Value[_]) => !literal.nullable + case _ => false + } + case _ => false } /** The comparison body of the bucket-pipeline rendering, `param` (= `params.`) against 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 bd19f889..a831a5c3 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 @@ -1869,6 +1869,62 @@ package object query { } } } + _ <- { + // F2 -- a full-text MATCH in HAVING over an aggregate (`HAVING MATCH (MAX(title)) + // AGAINST ('x')`) or over a column that is neither an aggregate nor a GROUP BY key + // (`GROUP BY g HAVING MATCH (title) AGAINST ('x')`) has no group-level form: the group + // filter has no MATCH, the key filter reads only the key, and such a column is not + // constant within a group. The condition was DROPPED and every group came back, HTTP + // 200. Refused by name: the aggregate written inline or named by its SELECT alias (the + // same alias map the substitution above reads), the column classified by the same + // `keyBucketOf` as rule (a) below. Left as they are: a MATCH over the GROUP BY key, and + // a MATCH the nested filter emits -- inside a relation, or over nested columns only, + // exactly the predicates `havingLeaves` leaves to that filter. + def matchesOf(c: Criteria, nestedFilter: Boolean): Seq[(MultiMatchCriteria, Boolean)] = + c match { + case Predicate(l, _, r, _, _) => + matchesOf(l, nestedFilter) ++ matchesOf(r, nestedFilter) + case relation: ElasticRelation => matchesOf(relation.criteria, nestedFilter = true) + case m: MultiMatchCriteria => Seq(m -> (nestedFilter || m.nested)) + case _ => Nil + } + val allMatches = having.flatMap(_.criteria).toSeq.flatMap(matchesOf(_, false)) + val matches = allMatches.map(_._1) + lazy val aliases = Having.aggregateAliases(this) + // An aggregate is named first, wherever it stands among the MATCH's columns. + def overAggregate: Option[String] = matches.view.flatMap { m => + m.identifiers.collect { + // `bucketMetrics`, not `hasAggregation`: a function never looks inside its own + // arguments, so `MATCH (UPPER(MAX(t)))` hides its aggregate from the latter (#389) + case id if id.bucketMetrics.nonEmpty => + s"the aggregate ${id.bucketMetrics.head.sql} in ${m.sql}: a full-text match " + + "reads documents, and HAVING filters groups." + case id if id.functions.isEmpty && aliases.contains(id.name) => + s"the aggregate ${id.name} (${aliases(id.name).sql}) in ${m.sql}: a full-text " + + "match reads documents, and HAVING filters groups." + } + }.headOption + def overColumn: Option[String] = allMatches.view + .collect { case (m, false) => + m + } + .flatMap { m => + m.identifiers + .flatMap(FunctionUtils.funIdentifiers(_)) + .filter(_.name.nonEmpty) + .find(keyBucketOf(_).isEmpty) + .map(column => + s"the column ${column.name} in ${m.sql}: ${column.name} is neither an aggregate " + + "nor a GROUP BY key, and HAVING filters groups." + ) + } + .headOption + overAggregate.orElse(overColumn) match { + case Some(reason) => + Left(s"HAVING cannot apply MATCH to $reason Put the MATCH in WHERE.") + case None => Right(()) + } + } _ <- { // ๐Ÿ”ด Issue #389 -- (a) a predicate that reads neither an aggregate NOR a grouping key is // not constant within a bucket, so NO mechanism can honour it: the terms filter cannot 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 new file mode 100644 index 00000000..8a2ba0ab --- /dev/null +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/HavingNullAwareSelectorSpec.scala @@ -0,0 +1,300 @@ +/* + * 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.parser.Parser +import org.scalatest.OptionValues +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** F1 -- an aggregate reads in `HAVING` exactly as SELECT returns it; 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. + * + * The `sql` half: what `MetricSelectorScript.nullAwareSelectorScript` renders, and the refusal. + * Whether Elasticsearch keeps the right groups is EXECUTED in the testkit's + * `GroupByCompletenessSpec`, through the gateway, on every major; the emitted request is pinned in + * the bridge suites. + */ +class HavingNullAwareSelectorSpec extends AnyFlatSpec with Matchers with OptionValues { + + private def parsed(sql: String): SingleSearch = Parser(sql) match { + case Right(s: SingleSearch) => s + case other => fail(s"[$sql] expected a SingleSearch, got $other") + } + + private def rejection(sql: String): String = Parser(sql) match { + case Left(e) => e.msg + case Right(s) => fail(s"[$sql] expected a rejection, got $s") + } + + private def having(sql: String): Criteria = + parsed(sql).having.flatMap(_.criteria).getOrElse(fail(s"[$sql] has no HAVING criteria")) + + private def script(sql: String): String = + MetricSelectorScript.nullAwareSelectorScript(having(sql)).value + + private val group = "SELECT g, COUNT(*) AS c FROM t GROUP BY g HAVING " + + /** The null-aware read of `params.`, spelled out here: the function under test is never its + * own oracle. + */ + private def read(name: String): String = + s"((def) (params.$name == null || Double.isNaN(params.$name) || " + + s"Double.isInfinite(params.$name) ? null : params.$name))" + + private val maxV = read("max_v") + + private val Param = """params\.([A-Za-z_][A-Za-z0-9_]*)""".r + + "the null-aware selector" should "read an aggregate as NULL when it is null, NaN or infinite" in { + script(group + "MAX(v) <> 5") shouldBe s"($maxV == null ? false : ($maxV != 5))" + } + + it should "keep the guard OUTSIDE the negation, so NOT of UNKNOWN stays UNKNOWN" in { + script(group + "NOT MAX(v) = 5") shouldBe s"($maxV == null ? false : ($maxV != 5))" + script(group + "MAX(v) NOT BETWEEN 4 AND 6") shouldBe + s"($maxV == null ? false : !($maxV >= 4 && $maxV <= 6))" + script(group + "MAX(v) NOT IN (5, 8)") shouldBe + s"($maxV == null ? false : !($maxV == 5 || $maxV == 8))" + script(group + "COUNT(*) > 2 AND NOT MAX(v) > 6") shouldBe + s"((params.c == null ? false : (params.c > 2))) && ($maxV == null ? false : ($maxV <= 6))" + } + + it should "test the value itself for ISNULL and ISNOTNULL" in { + script(group + "ISNULL(MAX(v))") shouldBe s"$maxV == null" + script(group + "ISNOTNULL(MAX(v))") shouldBe s"$maxV != null" + script(group + "COUNT(*) > 2 AND NOT ISNULL(MAX(v))") shouldBe + s"((params.c == null ? false : (params.c > 2))) && $maxV != null" + } + + 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]`). + script(group + "GREATEST(MAX(v), 0) > 1") shouldBe + s"($maxV == null ? false : (Math.max($maxV, 0) > 1))" + } + + it should "guard a COALESCE on its result, never on its arguments" in { + val a = read("max_a") + val b = read("min_b") + val coalesce = s"($a != null ? $a : $b)" + // UNKNOWN only when every argument is NULL: a group with `a` and no `b` compares its `MAX(a)` + script(group + "COALESCE(MAX(a), MIN(b)) > 1") shouldBe + s"($coalesce == null ? false : ($coalesce > 1))" + script(group + "COUNT(*) > 0 AND NOT COALESCE(MAX(a), MIN(b)) > 1") shouldBe + s"((params.c == null ? false : (params.c > 0))) && " + + s"($coalesce == null ? false : ($coalesce <= 1))" + // on the right of an aggregate, whose own guard stays + script(group + "COUNT(*) > COALESCE(MAX(a), MIN(b))") shouldBe + s"(params.c == null || $coalesce == null ? false : (params.c > $coalesce))" + // a function of the COALESCE reads its value + script(group + "SIGN(COALESCE(MAX(a), MIN(b))) > 0") shouldBe + s"($coalesce == null ? false : (($coalesce > 0 ? 1 : ($coalesce < 0 ? -1 : 0)) > 0))" + // a null test reads the value itself, NULL included + script(group + "ISNULL(COALESCE(MAX(a), MIN(b)))") shouldBe s"$coalesce == null" + script(group + "ISNOTNULL(COALESCE(MAX(a), MIN(b)))") shouldBe s"$coalesce != null" + // never NULL -- a literal among the arguments: nothing to guard + script(group + "COALESCE(MAX(a), MIN(b), 5) > 1") shouldBe + s"($a != null ? $a : ($b != null ? $b : 5)) > 1" + script(group + "COUNT(*) > COALESCE(SUM(a), 0)") shouldBe + "(params.c == null ? false : (params.c > (params.sum_a != null ? params.sum_a : 0)))" + } + + it should "read COUNT and SUM as before, beside a nullable aggregate" in { + script(group + "COUNT(*) > MAX(v)") shouldBe + s"(params.c == null || $maxV == null ? false : (params.c > $maxV))" + script(group + "SUM(v) > 1 AND MIN(v) < 9") shouldBe + s"(params.sum_v == null ? false : (params.sum_v > 1)) && " + + s"(${read("min_v")} == null ? false : (${read("min_v")} < 9))" + } + + it should "read as NULL every aggregate SELECT answers NULL over no value, and no other" in { + // F1's list (`ClientAggregation.nullOverEmptyInput`; the core suite asserts the two rules + // agree over every aggregation type) + Seq( + "MIN(v)", + "MAX(v)", + "AVG(v)", + "STDDEV(v)", + "STDDEV_POP(v)", + "STDDEV_SAMP(v)", + "VARIANCE(v)", + "VAR_POP(v)", + "VAR_SAMP(v)", + "PERCENTILE_CONT(0.5) WITHIN GROUP (ORDER BY v)", + "PERCENTILE_DISC(0.5) WITHIN GROUP (ORDER BY v)", + "FIRST_VALUE(v) OVER (PARTITION BY g ORDER BY v)", + "LAST_VALUE(v) OVER (PARTITION BY g ORDER BY v)", + "MAX(ABS(v))" + ).foreach { a => + withClue(s"[$a] ") { + val criteria = having(group + s"$a > 1") + val plain = MetricSelectorScript.selectorScript(criteria).value + val name = Param.findFirstMatchIn(plain).value.group(1) + script(group + s"$a > 1") shouldBe plain.replace(s"params.$name", read(name)) + } + } + // arithmetic over aggregates, read by its SELECT alias: a `bucket_script` result + script("SELECT g, MAX(v) - MIN(v) AS d FROM t GROUP BY g HAVING d > 1") shouldBe + s"(${read("d")} == null ? false : (${read("d")} > 1))" + } + + it should "leave a clause with no such aggregate exactly as rendered before" in { + Seq( + group + "COUNT(*) > 1", + group + "COUNT(v) > 1", + group + "COUNT(DISTINCT v) > 1", + group + "SUM(v) > 1 AND COUNT(*) < 9", + group + "SUM(v) NOT BETWEEN 1 AND 3 OR ISNULL(SUM(v))" + ).foreach { sql => + withClue(s"[$sql] ") { + val criteria = having(sql) + MetricSelectorScript.nullAwareSelectorScript(criteria) shouldBe + MetricSelectorScript.selectorScript(criteria) + } + } + } + + it should "read no variable the clause does not already publish" in { + Seq( + group + "MAX(v) <> 5", + group + "COUNT(*) > 2 OR NOT MIN(v) > 6", + group + "COALESCE(AVG(v), 5) > 1 AND ISNULL(MAX(v))", + "SELECT g, MAX(v) AS m FROM t GROUP BY g HAVING m <> 5", + "SELECT g, MAX(v) - MIN(v) AS d FROM t GROUP BY g HAVING d > 1", + group + "MAX(d) > now - interval 7 day" + ).foreach { sql => + withClue(s"[$sql] ") { + val criteria = having(sql) + def reads(s: Option[String]): Set[String] = + s.toSeq.flatMap(Param.findAllMatchIn(_).map(_.group(1))).toSet + reads(MetricSelectorScript.nullAwareSelectorScript(criteria)) shouldBe + reads(MetricSelectorScript.selectorScript(criteria)) + } + } + } + + "Having.script" should "carry the null-aware read the bridge's selector carries (#292)" in { + Seq( + group + "MAX(v) <> 5", + group + "ISNULL(MAX(v))", + group + "COUNT(*) > 2 OR NOT MIN(v) > 6", + group + "COALESCE(AVG(v), 5) > 1", + "SELECT g, MAX(v) AS m FROM t GROUP BY g HAVING m <> 5", + group + "COUNT(*) > 1" + ).foreach { sql => + withClue(s"[$sql] ") { + val search = parsed(sql) + val criteria = search.having.flatMap(_.criteria).value + @annotation.nowarn("cat=deprecation") + val viewScript = search.having.flatMap(_.script) + viewScript shouldBe MetricSelectorScript.nullAwareSelectorScript(criteria).map(_.trim) + } + } + } + + "MetricSelectorScript.metricSelector" should "read NULL as the bridge's selector does (#292)" in { + metricSelectorOf(group + "MAX(v) <> 5") shouldBe s"($maxV == null ? false : ($maxV != 5))" + Seq( + group + "ISNULL(MAX(v))", + group + "COALESCE(MAX(a), MIN(b)) > 1", + "SELECT g, MAX(v) AS m FROM t GROUP BY g HAVING m <> 5", + group + "COUNT(*) > 1" + ).foreach { sql => + withClue(s"[$sql] ") { + metricSelectorOf(sql) shouldBe + MetricSelectorScript.nullAwareSelectorScript(having(sql)).value + } + } + } + + private def metricSelectorOf(sql: String): String = + MetricSelectorScript.metricSelector(having(sql)) + + // --------------------------------------------------------------------------------------------- + // F2 -- a MATCH in HAVING over an aggregate, or over a column that is neither an aggregate nor + // a GROUP BY key + // --------------------------------------------------------------------------------------------- + + "a MATCH over an aggregate in HAVING" should "be refused by name, with the remedy" in { + Seq( + group + "MATCH (MAX(t)) AGAINST ('x')" -> "MAX(t)", + group + "COUNT(*) > 1 AND MATCH (MAX(t)) AGAINST ('x')" -> "MAX(t)", + group + "COUNT(*) > 1 OR MATCH (MAX(t)) AGAINST ('x')" -> "MAX(t)", + group + "COUNT(*) > 1 AND NOT MATCH (MAX(t)) AGAINST ('x')" -> "MAX(t)", + group + "MATCH (t, MAX(t)) AGAINST ('x')" -> "MAX(t)", + group + "MATCH (COUNT(t)) AGAINST ('x')" -> "COUNT(t)", + group + "MATCH (UPPER(MAX(t))) AGAINST ('x')" -> "MAX(t)", + "SELECT g, MAX(t) AS mt FROM t GROUP BY g HAVING MATCH (mt) AGAINST ('x')" -> "mt (MAX(t))", + "SELECT MAX(t) AS mt FROM t HAVING MATCH (MAX(t)) AGAINST ('x')" -> "MAX(t)" + ).foreach { case (sql, aggregate) => + withClue(s"[$sql] ") { + val msg = rejection(sql) + msg should startWith(s"HAVING cannot apply MATCH to the aggregate $aggregate in MATCH (") + msg should include("Put the MATCH in WHERE.") + msg should not startWith Parser.InternalParseFailure + } + } + } + + "a MATCH over a column that is neither an aggregate nor a GROUP BY key" should + "be refused by name, with the remedy" in { + Seq( + group + "MATCH (t) AGAINST ('x')", + group + "COUNT(*) > 1 AND MATCH (t) AGAINST ('x')", + group + "COUNT(*) > 1 OR MATCH (t) AGAINST ('x')", + group + "COUNT(*) > 1 AND NOT MATCH (t) AGAINST ('x')", + group + "MATCH (g, t) AGAINST ('x')", + group + "MATCH (UPPER(t)) AGAINST ('x')", + "SELECT g, COUNT(*) AS c FROM t GROUP BY g, h HAVING MATCH (t) AGAINST ('x')", + // no GROUP BY: there is no key at all + "SELECT COUNT(*) AS c FROM t HAVING MATCH (t) AGAINST ('x')" + ).foreach { sql => + withClue(s"[$sql] ") { + val msg = rejection(sql) + msg should startWith("HAVING cannot apply MATCH to the column t in MATCH (") + msg should include("t is neither an aggregate nor a GROUP BY key") + msg should include("Put the MATCH in WHERE.") + msg should not startWith Parser.InternalParseFailure + } + } + } + + it should "leave a MATCH over the GROUP BY key as it is" in { + parsed(group + "MATCH (g) AGAINST ('x')") + parsed("SELECT g, COUNT(*) AS c FROM t GROUP BY g, h HAVING MATCH (g, h) AGAINST ('x')") + } + + it should "leave a MATCH the nested filter applies as it is" in { + // over nested columns only: emitted as a match inside the nested filter, never dropped + parsed( + "SELECT p.category AS cat, MIN(p.price) AS min_price FROM stores s JOIN UNNEST(s.products) " + + "AS p GROUP BY p.category HAVING MATCH (p.name, p.description) AGAINST ('lasagnes') AND " + + "MIN(p.price) > 5.0" + ) + } + + it should "leave the same MATCH in WHERE untouched" in { + parsed( + "SELECT g, COUNT(*) AS c FROM t WHERE MATCH (t) AGAINST ('x') GROUP BY g HAVING COUNT(*) > 1" + ) + } +} 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 c268b269..267a335e 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 @@ -59,8 +59,10 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt private def havingOf(s: SingleSearch, sql: String): Criteria = s.having.flatMap(_.criteria).getOrElse(fail(s"[$sql] has no HAVING criteria")) + // The rendering BEFORE the null-aware read (`metricSelector` reads through it): what this spec + // pins is the function rendering and its guard, which the read does not touch. private def script(sql: String): String = - MetricSelectorScript.metricSelector(having(sql)) + MetricSelectorScript.selectorScript(having(sql)).getOrElse("1 == 1") /** The pattern the `terms` filter would carry, asked of the REAL derivation. */ private def includeOf(sql: String): Option[String] = { @@ -377,7 +379,7 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt val sql = group + "COUNT(*) > 1 AND NULLIF(COUNT(*), 0) > 2" val criteria = havingOf(unvalidated(sql), sql) val thrown = intercept[IllegalStateException] { - MetricSelectorScript.metricSelector(criteria) + MetricSelectorScript.selectorScript(criteria) } thrown.getMessage should include("HAVING cannot be applied to") // ... and it must not have quietly emitted the OTHER half instead. @@ -937,10 +939,13 @@ class HavingOverAggregateFunctionSpec extends AnyFlatSpec with Matchers with Opt // ๐Ÿ”ด `MAX(c1) = 1` renders `params.max_c1 == 1`, which holds `1 == 1` -- the text // `metricSelector` answers when there is nothing to filter. Stripped out of the whole script, // it left `params.max_c`; `= 10` left `params.max_c0`, and `IN (1, 2)` lost its first member. + // F1: `max_c1` is read as NULL when it is null, NaN or infinite (an empty group). + val m = "((def) (params.max_c1 == null || Double.isNaN(params.max_c1) || " + + "Double.isInfinite(params.max_c1) ? null : params.max_c1))" Seq( - "MAX(c1) = 1" -> "(params.max_c1 == null ? false : (params.max_c1 == 1))", - "MAX(c1) = 10" -> "(params.max_c1 == null ? false : (params.max_c1 == 10))", - "MAX(c1) IN (1, 2)" -> "(params.max_c1 == null ? false : (params.max_c1 == 1 || params.max_c1 == 2))" + "MAX(c1) = 1" -> s"($m == null ? false : ($m == 1))", + "MAX(c1) = 10" -> s"($m == null ? false : ($m == 10))", + "MAX(c1) IN (1, 2)" -> s"($m == null ? false : ($m == 1 || $m == 2))" ).foreach { case (condition, script) => withClue(s"[$condition] ") { unvalidated( diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/query/MetricSelectorPrecedenceSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/query/MetricSelectorPrecedenceSpec.scala index bffc81a1..13a8c9c4 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/query/MetricSelectorPrecedenceSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/query/MetricSelectorPrecedenceSpec.scala @@ -64,11 +64,13 @@ class MetricSelectorPrecedenceSpec extends AnyFlatSpec with Matchers { out.toList } + // The rendering BEFORE the null-aware read (`metricSelector` reads through it): the read replaces + // each metric in place and leaves the tree this spec evaluates as it is. private def scriptOf(sql: String): String = Parser(sql) match { case Right(s: SingleSearch) => - MetricSelectorScript.metricSelector( - s.having.flatMap(_.criteria).getOrElse(fail(s"[$sql] no HAVING")) - ) + MetricSelectorScript + .selectorScript(s.having.flatMap(_.criteria).getOrElse(fail(s"[$sql] no HAVING"))) + .getOrElse("1 == 1") case other => fail(s"[$sql] $other") } 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 9cd6ab20..ceb42568 100644 --- a/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala +++ b/testkit/src/main/scala/app/softnetwork/elastic/client/GroupByCompletenessSpec.scala @@ -18,17 +18,26 @@ package app.softnetwork.elastic.client import akka.NotUsed import akka.actor.ActorSystem -import akka.stream.scaladsl.Source +import akka.stream.scaladsl.{Sink, Source} import app.softnetwork.elastic.client.bulk._ -import app.softnetwork.elastic.client.result.{ElasticFailure, ElasticResult, ElasticSuccess} +import app.softnetwork.elastic.client.result.{ + ElasticFailure, + ElasticResult, + ElasticSuccess, + QueryRows, + QueryStream, + QueryStructured +} 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 scala.collection.immutable.ListMap import scala.concurrent.Await import scala.concurrent.duration._ import scala.language.implicitConversions @@ -110,6 +119,8 @@ trait GroupByCompletenessSpec extends AnyFlatSpecLike with ElasticDockerTestKit override def afterAll(): Unit = { client.deleteIndex(index) + client.deleteIndex(nullIndex) + client.deleteIndex(coalesceIndex) super.afterAll() } @@ -1177,4 +1188,648 @@ trait GroupByCompletenessSpec extends AnyFlatSpecLike with ElasticDockerTestKit } } } + + // --------------------------------------------------------------------------------------------- + // F1 -- an aggregate reads in HAVING exactly as SELECT returns it. + // + // In a group where no document has the aggregated column, SELECT answers NULL for MIN, MAX, + // AVG, the percentiles and arithmetic over them (`ClientAggregation.nullOverEmptyInput`), and + // SQL keeps a group only when its condition is TRUE -- a comparison with NULL is UNKNOWN, and + // so is its NOT. The `bucket_selector` was handed Elasticsearch's sentinel instead (`NaN`), so + // `MAX(v) <> 5` KEPT such a group and `ISNULL(MAX(v))` dropped it, HTTP 200. + // + // The population is DERIVED, not listed: the aggregates from that rule, every comparison, every + // NOT the grammar accepts, ISNULL / ISNOTNULL, each of them under AND / OR with COUNT(*), on + // both venues of the aggregate (HAVING only, and published by the SELECT list), plus the + // implicit whole-table group. The expected groups are computed HERE, from the fixture, with + // three-valued logic -- never from the engine. Every statement runs through the gateway. + // --------------------------------------------------------------------------------------------- + + private val nullIndex = "having_null_groups" + + /** Each group and the `v` of each of its documents (`None`: the document has no `v`): absent from + * every document, partly present, fully present. The present values of a group are all equal, so + * every percentile of it is exact on every major -- the oracle never models a digest. + */ + private val nullGroups: Seq[(String, Seq[Option[Int]])] = Seq( + "a1" -> Seq(None, None), + "a2" -> Seq(None, None, None), + "p1" -> Seq(Some(5), None), + "p2" -> Seq(Some(7), Some(7), None), + "f1" -> Seq(Some(5), Some(5)), + "f2" -> Seq(Some(7), Some(7), Some(7)) + ) + + private lazy val nullGroupsLoaded: Unit = { + client + .createIndex(nullIndex, settings = """{"number_of_shards": 1, "number_of_replicas": 0}""") + .get shouldBe true + client + .setMapping( + nullIndex, + """{"properties": {"id": {"type": "keyword"}, "g": {"type": "keyword"}, "v": {"type": "integer"}}}""" + ) + .get shouldBe true + val docs = for { + (g, vs) <- nullGroups.toList + (v, offset) <- vs.zipWithIndex + } yield v.fold(s"""{"id":"$g-$offset","g":"$g"}""")(x => + s"""{"id":"$g-$offset","g":"$g","v":$x}""" + ) + implicit val bulkOptions: BulkOptions = BulkOptions(defaultIndex = nullIndex, 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(nullIndex) + case ElasticFailure(error) => fail(s"Bulk indexing into $nullIndex failed: ${error.message}") + } + } + + /** The aggregates SELECT answers NULL over a group with no value, DERIVED from the rule the + * response parser applies (`ClientAggregation.nullOverEmptyInput`) over every aggregation type. + */ + private lazy val nullOverEmptyTypes: Seq[AggregationType.AggregationType] = + AggregationType.values.toSeq.filter(t => + ClientAggregation( + aggName = "a", + aggType = t, + distinct = false, + sourceField = "v", + windowing = false, + bucketPath = "", + bucketRoot = "" + ).nullOverEmptyInput + ) + + /** The derived aggregates a HAVING cannot answer at all -- MEASURED on main, every form failing + * in Elasticsearch before any group is filtered, and recorded as their own defects. + */ + private val notAnsweredByHaving: Map[AggregationType.AggregationType, String] = Map( + AggregationType.Stddev -> "extended_stats read without its key by the bucket_selector", + AggregationType.StddevSamp -> "extended_stats read without its key by the bucket_selector", + AggregationType.StddevPop -> "extended_stats read without its key by the bucket_selector", + AggregationType.Variance -> "extended_stats read without its key by the bucket_selector", + AggregationType.VarSamp -> "extended_stats read without its key by the bucket_selector", + AggregationType.VarPop -> "extended_stats read without its key by the bucket_selector", + AggregationType.FirstValue -> "top_hits: no number a bucket_selector can read", + AggregationType.LastValue -> "top_hits: no number a bucket_selector can read" + ) + + /** How a HAVING names an aggregate of `nullOverEmptyTypes` over `v`, and what SELECT answers for + * it over a group's present values. Arithmetic over aggregates is named by its SELECT alias. + */ + private final case class NullableAggregate( + kind: AggregationType.AggregationType, + having: String, + selectItem: String, + value: Seq[Int] => Double + ) + + private def nullableAggregate(t: AggregationType.AggregationType): NullableAggregate = { + def single(present: Seq[Int]): Double = { + present.distinct should have size 1L + present.head.toDouble + } + t match { + case AggregationType.Min => NullableAggregate(t, "MIN(v)", "MIN(v) AS a", _.min.toDouble) + case AggregationType.Max => NullableAggregate(t, "MAX(v)", "MAX(v) AS a", _.max.toDouble) + case AggregationType.Avg => + NullableAggregate(t, "AVG(v)", "AVG(v) AS a", vs => vs.sum.toDouble / vs.size) + case AggregationType.PercentileCont => + val p = "PERCENTILE_CONT(0.5) WITHIN GROUP (ORDER BY v)" + NullableAggregate(t, p, s"$p AS a", single) + case AggregationType.PercentileDisc => + val p = "PERCENTILE_DISC(0.5) WITHIN GROUP (ORDER BY v)" + NullableAggregate(t, p, s"$p AS a", single) + case AggregationType.BucketScript => + NullableAggregate(t, "d", "MAX(v) - MIN(v) AS d", vs => (vs.max - vs.min).toDouble) + case other => fail(s"$other is NULL over an empty group and has no HAVING spelling here") + } + } + + /** A HAVING condition over the aggregate `a`, rendered as SQL and evaluated as SQL does: `None` + * is UNKNOWN. + */ + private sealed trait NullCond { + def sql(a: String): String + def eval(v: Option[Double], rows: Int): Option[Boolean] + } + + private final case class Cmp(op: String, k: Int) extends NullCond { + def sql(a: String): String = s"$a $op $k" + def eval(v: Option[Double], rows: Int): Option[Boolean] = v.map { x => + op match { + case ">" => x > k + case "<" => x < k + case ">=" => x >= k + case "<=" => x <= k + case "=" => x == k + case "<>" | "!=" => x != k + } + } + } + + private final case class NotCmp(cmp: Cmp) extends NullCond { + def sql(a: String): String = s"NOT ${cmp.sql(a)}" + def eval(v: Option[Double], rows: Int): Option[Boolean] = cmp.eval(v, rows).map(!_) + } + + private final case class Between(lo: Int, hi: Int, not: Boolean) extends NullCond { + def sql(a: String): String = s"$a ${if (not) "NOT " else ""}BETWEEN $lo AND $hi" + def eval(v: Option[Double], rows: Int): Option[Boolean] = + v.map(x => (x >= lo && x <= hi) != not) + } + + private final case class In(ks: Seq[Int], not: Boolean) extends NullCond { + def sql(a: String): String = s"$a ${if (not) "NOT " else ""}IN (${ks.mkString(", ")})" + def eval(v: Option[Double], rows: Int): Option[Boolean] = + v.map(x => ks.exists(_.toDouble == x) != not) + } + + private final case class NullTest(isNull: Boolean) extends NullCond { + def sql(a: String): String = if (isNull) s"ISNULL($a)" else s"ISNOTNULL($a)" + def eval(v: Option[Double], rows: Int): Option[Boolean] = Some(v.isEmpty == isNull) + } + + private final case class RowsAbove(n: Int) extends NullCond { + def sql(a: String): String = s"COUNT(*) > $n" + def eval(v: Option[Double], rows: Int): Option[Boolean] = Some(rows > n) + } + + /** `left AND|OR [NOT] right`: the grammar's `NOT` after an operator negates the RIGHT operand. */ + private final case class Junction( + left: NullCond, + and: Boolean, + notRight: Boolean, + right: NullCond + ) extends NullCond { + def sql(a: String): String = + s"${left.sql(a)} ${if (and) "AND" else "OR"}${if (notRight) " NOT" else ""} ${right.sql(a)}" + def eval(v: Option[Double], rows: Int): Option[Boolean] = { + val l = left.eval(v, rows) + val r = right.eval(v, rows).map(b => if (notRight) !b else b) + if (and) { + (l, r) match { + case (Some(false), _) | (_, Some(false)) => Some(false) + case (Some(true), Some(true)) => Some(true) + case _ => None + } + } else { + (l, r) match { + case (Some(true), _) | (_, Some(true)) => Some(true) + case (Some(false), Some(false)) => Some(false) + case _ => None + } + } + } + } + + private val comparisons: Seq[Cmp] = + Seq( + Cmp(">", 6), + Cmp("<", 6), + Cmp(">=", 7), + Cmp("<=", 5), + Cmp("=", 5), + Cmp("<>", 5), + Cmp("!=", 7) + ) + + private val positives: Seq[NullCond] = + comparisons ++ Seq(Between(4, 6, not = false), In(Seq(5, 8), not = false)) ++ + Seq(NullTest(isNull = true), NullTest(isNull = false)) + + /** Every NOT the grammar accepts over an aggregate: before a comparison, inside BETWEEN / IN, and + * after AND / OR (below). `NOT ISNULL(...)` at the start of a clause is not accepted. + */ + private val negations: Seq[NullCond] = + comparisons.map(c => NotCmp(c)) ++ Seq(Between(4, 6, not = true), In(Seq(5, 8), not = true)) + + private val rowsAbove = RowsAbove(2) + + private val nullForms: Seq[NullCond] = { + val alone = positives ++ negations + val withCount = alone.flatMap(c => + Seq( + Junction(rowsAbove, and = true, notRight = false, c), + Junction(rowsAbove, and = false, notRight = false, c), + Junction(c, and = true, notRight = false, rowsAbove), + Junction(c, and = false, notRight = false, rowsAbove) + ) + ) + val negatedAfterOperator = positives.flatMap(c => + Seq( + Junction(rowsAbove, and = true, notRight = true, c), + Junction(rowsAbove, and = false, notRight = true, c) + ) + ) + alone ++ withCount ++ negatedAfterOperator + } + + private def readsNullTest(c: NullCond): Boolean = c match { + case _: NullTest => true + case Junction(l, _, _, r) => readsNullTest(l) || readsNullTest(r) + case _ => false + } + + /** `wholeTable`: the one group the WHERE selects, for a statement with no GROUP BY. */ + private final case class NullStatement( + sql: String, + expected: Set[String], + wholeTable: Option[String] = None + ) + + private lazy val nullCandidates: Seq[NullStatement] = { + val answered = nullOverEmptyTypes.filterNot(notAnsweredByHaving.contains).map(nullableAggregate) + val oracle: Seq[(String, Option[Seq[Int]], Int)] = nullGroups.map { case (g, vs) => + (g, Some(vs.flatten).filter(_.nonEmpty), vs.size) + } + def keep(agg: NullableAggregate, c: NullCond): Set[String] = + oracle.collect { + case (g, present, rows) if c.eval(present.map(agg.value), rows).contains(true) => g + }.toSet + val grouped = for { + agg <- answered + c <- nullForms + // `ISNULL(d)` over the alias of arithmetic over aggregates reads the OPERANDS, which its + // selector does not declare -- it fails on main, before any group is filtered. + if !(agg.kind == AggregationType.BucketScript && readsNullTest(c)) + select <- + if (agg.kind == AggregationType.BucketScript) Seq(s"g, ${agg.selectItem}") + else Seq("g, COUNT(*) AS c", s"g, ${agg.selectItem}") + } yield NullStatement( + s"SELECT $select FROM $nullIndex GROUP BY g HAVING ${c.sql(agg.having)}", + keep(agg, c) + ) + // The implicit whole-table group, one group of each kind at a time. + val wholeTable = for { + agg <- answered.filterNot(_.kind == AggregationType.BucketScript) + c <- positives ++ negations + (g, vs) <- nullGroups.filter { case (g, _) => Set("a1", "p1", "f1").contains(g) } + } yield NullStatement( + s"SELECT ${agg.selectItem} FROM $nullIndex WHERE g = '$g' HAVING ${c.sql(agg.having)}", + if (c.eval(Some(vs.flatten).filter(_.nonEmpty).map(agg.value), vs.size).contains(true)) + Set(g) + else Set.empty, + wholeTable = Some(g) + ) + grouped ++ wholeTable + } + + /** The forms the grammar accepts -- and the ones it does not, which must be ONLY `ISNULL(...)` / + * `ISNOTNULL(...)` over the WITHIN GROUP spelling of a percentile (asserted in the test). + */ + private lazy val nullPartition: (Seq[NullStatement], Seq[NullStatement]) = + nullCandidates.partition(st => Parser(st.sql).isRight) + + private def nullPopulation: Seq[NullStatement] = nullPartition._1 + + private def nullPopulationNotParsed: Seq[NullStatement] = nullPartition._2 + + /** The rows of a statement, through `GatewayApi.run` (JDBC / REPL / Flight SQL). */ + private def gatewayRows(sql: String): Either[String, Seq[ListMap[String, Any]]] = + 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) + } + + /** The groups a statement keeps, through the gateway. */ + private def keptGroups(statement: NullStatement): Either[String, Set[String]] = + gatewayRows(statement.sql).map { rs => + statement.wholeTable match { + case None => rs.map(_.getOrElse("g", "?").toString).toSet + // The implicit group is ONE row or none; any other count is a wrong answer of its own. + case Some(g) => + if (rs.isEmpty) Set.empty[String] + else if (rs.size == 1) Set(g) + else Set(g, s"${rs.size} rows") + } + } + + "a HAVING over an aggregate a group has no value for" should + "keep exactly the groups SQL's three-valued logic keeps" in { + nullGroupsLoaded + // Non-vacuity, computed over the material: the population must hold the shapes the defect + // lives in, and the oracle must both keep and drop the groups that have no value. + val population = nullPopulation + population.size should be >= 1000 + Seq( + " <> ", + " != ", + "HAVING NOT ", + " NOT BETWEEN ", + " NOT IN ", + "ISNULL(", + "ISNOTNULL(", + " AND NOT ", + " OR NOT " + ) + .foreach(shape => + withClue(s"[$shape] ")(population.exists(_.sql.contains(shape)) shouldBe true) + ) + population.exists(_.expected.contains("a1")) shouldBe true + population.exists(st => !st.expected.contains("a1") && st.expected.contains("f1")) shouldBe true + // The parser trimmed only `ISNULL` / `ISNOTNULL` over the WITHIN GROUP spelling. + nullPopulationNotParsed + .map(_.sql) + .filterNot(sql => + sql.contains("ISNULL(PERCENTILE_") || sql.contains("ISNOTNULL(PERCENTILE_") + ) shouldBe empty + + 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 valueless groups: ${population.size} statements over ${nullOverEmptyTypes.size} derived " + + s"aggregates (${notAnsweredByHaving.keys.toSeq.map(_.toString).sorted.mkString(", ")} not " + + s"answered by HAVING); wrong: ${wrong.size} statements / $wrongGroups groups; " + + s"errors: ${errors.size}" + ) + val report = wrong.take(20).map { case (st, actual) => + s"[${st.sql}] kept ${actual.toSeq.sorted.mkString(",")}, expected ${st.expected.toSeq.sorted.mkString(",")}" + } + withClue( + s"${wrong.size} wrong statements ($wrongGroups wrong groups), ${errors.size} errors:\n" + + (report ++ errors.take(10)).mkString("\n") + "\n" + ) { + wrong shouldBe empty + errors shouldBe empty + } + } + + // --------------------------------------------------------------------------------------------- + // A COALESCE of aggregates in HAVING is guarded on its RESULT: the comparison is UNKNOWN only + // when every argument is NULL. It was guarded on the argument its rendering does not test, the + // LAST one, so once an empty group's aggregate read as NULL, `COALESCE(MAX(a), MIN(b)) > 1` + // dropped a group whose documents have `a` and no `b`. + // + // The fixture is the review's: a group with `a` only, `b` only, both, neither, and a `0` on + // either side. The population is a COALESCE in every position its guard is derived for -- + // compared, under NOT / BETWEEN / IN, on the right of an aggregate or of a literal, + // null-tested, nested, with three arguments, as the argument of a function -- on both venues + // of the aggregates, plus the review's four statements in the implicit whole-table group. The + // expected groups are computed HERE, with three-valued logic, never from the engine. + // --------------------------------------------------------------------------------------------- + + private val coalesceIndex = "having_coalesce_groups" + + /** Each group and the `(a, b)` of each of its documents (`None`: the document lacks it). */ + private val coalesceGroups: Seq[(String, Seq[(Option[Int], Option[Int])])] = Seq( + "g1" -> Seq((Some(5), None), (Some(5), None)), + "g2" -> Seq((None, Some(5))), + "g3" -> Seq((Some(5), Some(5))), + "g4" -> Seq((None, None)), + "g5" -> Seq((Some(0), None)), + "g6" -> Seq((None, Some(0))) + ) + + private lazy val coalesceGroupsLoaded: Unit = { + client + .createIndex(coalesceIndex, settings = """{"number_of_shards": 1, "number_of_replicas": 0}""") + .get shouldBe true + client + .setMapping( + coalesceIndex, + """{"properties": {"id": {"type": "keyword"}, "g": {"type": "keyword"}, "a": {"type": "integer"}, "b": {"type": "integer"}}}""" + ) + .get shouldBe true + val docs = for { + (g, rows) <- coalesceGroups.toList + ((a, b), offset) <- rows.zipWithIndex + } yield (Seq(s""""id":"$g-$offset"""", s""""g":"$g"""") ++ a.map(x => s""""a":$x""") ++ + b.map(x => s""""b":$x""")).mkString("{", ",", "}") + implicit val bulkOptions: BulkOptions = + BulkOptions(defaultIndex = coalesceIndex, 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(coalesceIndex) + case ElasticFailure(error) => + fail(s"Bulk indexing into $coalesceIndex failed: ${error.message}") + } + } + + /** The present `a` and `b` values of a group, and its document count. */ + private final case class CoalesceGroup(rows: Int, as: Seq[Int], bs: Seq[Int]) + + /** An operand of the HAVING comparison, and its value over a group as SELECT answers it (#336): + * NULL over no value, except `SUM`, which is `0`. + */ + private sealed trait CoalesceOperand { + def sql: String + def value(group: CoalesceGroup): Option[Double] + } + + private final case class Aggregate(sql: String) extends CoalesceOperand { + def value(group: CoalesceGroup): Option[Double] = sql match { + case "MAX(a)" => group.as.reduceOption(_ max _).map(_.toDouble) + case "MIN(b)" => group.bs.reduceOption(_ min _).map(_.toDouble) + case "MAX(b)" => group.bs.reduceOption(_ max _).map(_.toDouble) + case "SUM(a)" => Some(group.as.sum.toDouble) + case other => fail(s"no value for $other") + } + } + + private final case class Literal(k: Int) extends CoalesceOperand { + def sql: String = k.toString + def value(group: CoalesceGroup): Option[Double] = Some(k.toDouble) + } + + private final case class CoalesceOf(arguments: CoalesceOperand*) extends CoalesceOperand { + def sql: String = arguments.map(_.sql).mkString("COALESCE(", ", ", ")") + def value(group: CoalesceGroup): Option[Double] = + arguments.flatMap(_.value(group)).headOption + } + + private final case class SignOf(argument: CoalesceOperand) extends CoalesceOperand { + def sql: String = s"SIGN(${argument.sql})" + def value(group: CoalesceGroup): Option[Double] = argument.value(group).map(math.signum) + } + + private def compares(left: Double, op: String, right: Double): Boolean = op match { + case ">" => left > right + case "<" => left < right + case ">=" => left >= right + case "<=" => left <= right + case "=" => left == right + case "<>" => left != right + } + + /** `COUNT(*) X`: an aggregate on the LEFT of the operand. */ + private final case class CountVs(op: String) extends NullCond { + def sql(a: String): String = s"COUNT(*) $op $a" + def eval(v: Option[Double], rows: Int): Option[Boolean] = + v.map(x => compares(rows.toDouble, op, x)) + } + + /** `k X`: a literal on the LEFT of the operand. */ + private final case class LiteralVs(k: Int, op: String) extends NullCond { + def sql(a: String): String = s"$k $op $a" + def eval(v: Option[Double], rows: Int): Option[Boolean] = + v.map(x => compares(k.toDouble, op, x)) + } + + private val maxA = Aggregate("MAX(a)") + private val minB = Aggregate("MIN(b)") + private val maxB = Aggregate("MAX(b)") + + /** The review's four statements, over the review's fixture. */ + private val reviewForms: Seq[(CoalesceOperand, NullCond)] = Seq( + CoalesceOf(maxA, minB) -> Cmp(">", 1), + CoalesceOf(maxA, maxB) -> Cmp(">", 1), + CoalesceOf(maxA, Literal(5)) -> Cmp(">", 1), + CoalesceOf(maxA, minB) -> Junction(RowsAbove(0), and = true, notRight = true, Cmp(">", 1)) + ) + + private val coalesceOperands: Seq[CoalesceOperand] = Seq( + CoalesceOf(maxA, minB), + CoalesceOf(maxA, maxB), + // never NULL + CoalesceOf(maxA, Literal(5)), + CoalesceOf(minB, maxA), + CoalesceOf(maxA, minB, maxB), + CoalesceOf(maxA, minB, Literal(5)), + // SUM is never NULL either (#336) + CoalesceOf(Aggregate("SUM(a)"), minB), + CoalesceOf(CoalesceOf(maxA, minB), Literal(5)), + CoalesceOf(maxB, CoalesceOf(maxA, minB)), + SignOf(CoalesceOf(maxA, minB)) + ) + + private val coalesceForms: Seq[NullCond] = Seq( + Cmp(">", 1), + Cmp("<", 1), + Cmp("=", 5), + Cmp("<>", 5), + NotCmp(Cmp(">", 1)), + NotCmp(Cmp("=", 0)), + Between(1, 5, not = false), + Between(1, 5, not = true), + In(Seq(0, 5), not = false), + In(Seq(0, 5), not = true), + CountVs(">"), + CountVs("<="), + LiteralVs(1, "<"), + NullTest(isNull = true), + NullTest(isNull = false), + Junction(RowsAbove(0), and = true, notRight = true, Cmp(">", 1)), + Junction(RowsAbove(0), and = true, notRight = false, Cmp(">", 1)), + Junction(Cmp(">", 1), and = false, notRight = false, RowsAbove(1)), + Junction(RowsAbove(0), and = true, notRight = true, NullTest(isNull = true)) + ) + + private lazy val coalescePopulation: Seq[NullStatement] = { + val oracle: Seq[(String, CoalesceGroup)] = coalesceGroups.map { case (g, rows) => + g -> CoalesceGroup(rows.size, rows.flatMap(_._1), rows.flatMap(_._2)) + } + def keep(operand: CoalesceOperand, c: NullCond): Set[String] = + oracle.collect { + case (g, group) if c.eval(operand.value(group), group.rows).contains(true) => g + }.toSet + val grouped = for { + operand <- coalesceOperands + c <- coalesceForms + // `ISNULL(SIGN(...))` fails to compile in Elasticsearch on main already (`Cannot cast from + // [int] to [java.lang.Object]`): the null test of a function rendering a primitive. + if !(operand.isInstanceOf[SignOf] && readsNullTest(c)) + select <- Seq("g, COUNT(*) AS c", "g, MAX(a) AS ma, MIN(b) AS mb, MAX(b) AS xb, SUM(a) AS sa") + } yield NullStatement( + s"SELECT $select FROM $coalesceIndex GROUP BY g HAVING ${c.sql(operand.sql)}", + keep(operand, c) + ) + // The implicit whole-table group, one group at a time. + val wholeTable = for { + (operand, c) <- reviewForms + (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 ++ wholeTable + } + + "a HAVING over a COALESCE of aggregates" should + "keep exactly the groups SQL's three-valued logic keeps" in { + coalesceGroupsLoaded + val population = coalescePopulation + // Non-vacuity, computed over the material: the review's four statements are in it, and the + // oracle keeps and drops each group whose `a` or `b` has no value. + reviewForms.foreach { case (operand, c) => + val sql = + s"SELECT g, COUNT(*) AS c FROM $coalesceIndex GROUP BY g HAVING ${c.sql(operand.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 a COALESCE 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 + } + } + + // --------------------------------------------------------------------------------------------- + // 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 + // key filter reads only the key, so every group came back. + // --------------------------------------------------------------------------------------------- + + "a MATCH in HAVING over an aggregate or a non-key column" should + "be refused by name through the gateway" in { + nullGroupsLoaded + Seq( + s"SELECT g, COUNT(*) AS c FROM $nullIndex GROUP BY g HAVING MATCH (MAX(g)) AGAINST ('zzz')" -> + "HAVING cannot apply MATCH to the aggregate MAX(g)", + s"SELECT g, COUNT(*) AS c FROM $nullIndex GROUP BY g HAVING COUNT(*) > 0 AND MATCH (MAX(g)) AGAINST ('zzz')" -> + "HAVING cannot apply MATCH to the aggregate MAX(g)", + s"SELECT g, COUNT(*) AS c FROM $nullIndex GROUP BY g HAVING MATCH (id) AGAINST ('zzz')" -> + "HAVING cannot apply MATCH to the column id", + s"SELECT g, COUNT(*) AS c FROM $nullIndex GROUP BY g HAVING COUNT(*) > 0 OR MATCH (id) AGAINST ('zzz')" -> + "HAVING cannot apply MATCH to the column id" + ).foreach { case (sql, refusal) => + withClue(s"[$sql] ") { + gatewayRows(sql) match { + case Left(error) => + error should include(refusal) + error should include("Put the MATCH in WHERE.") + case Right(rows) => fail(s"answered ${rows.size} rows instead of refusing") + } + } + } + } }