ESQL: LEAST and GREATEST functions by nik9000 · Pull Request #98630 · elastic/elasticsearch

nik9000 · 2023-08-17T21:35:00Z

Adds LEAST and GREATEST functions to find the min or max of the values in many columns.

Adds `LEAST` and `GREATEST` functions to find the min or max of the values in many colunms.

github-actions · 2023-08-17T21:35:12Z

Documentation preview:

✨ Changed pages

elasticsearchmachine · 2023-08-17T21:35:24Z

Hi @nik9000, I've created a changelog YAML for you.

elasticsearchmachine · 2023-08-17T21:35:24Z

Pinging @elastic/es-ql (Team:QL)

elasticsearchmachine · 2023-08-17T21:35:25Z

Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL)

astefan

LGTM, but I believe more CSV tests are needed, especially for real (non-row command) data in employees table.

For example, from employees | eval min = least(still_hired), max = greatest(still_hired) | sort emp_no | limit 10 | keep still_hired, min, max fails with

{
    "error": {
        "root_cause": [
            {
                "type": "unsupported_operation_exception",
                "reason": null
            }
        ],
        "type": "unsupported_operation_exception",
        "reason": null
    },
    "status": 500
}

Wondering if a nicer error message isn't more appropriate. If I replace least and greatest with min and max I get something more informative:

{
    "error": {
        "root_cause": [
            {
                "type": "verification_exception",
                "reason": "Found 2 problems\nline 1:29: argument of [min(still_hired)] must be [numeric or datetime], found value [min(still_hired)] type [boolean]\nline 1:53: argument of [max(still_hired)] must be [numeric or datetime], found value [max(still_hired)] type [boolean]"
            }
        ],
        "type": "verification_exception",
        "reason": "Found 2 problems\nline 1:29: argument of [min(still_hired)] must be [numeric or datetime], found value [min(still_hired)] type [boolean]\nline 1:53: argument of [max(still_hired)] must be [numeric or datetime], found value [max(still_hired)] type [boolean]"
    },
    "status": 400
}

I think it would be useful to test the null default handling for multi-values. For example from employees | eval min = least(job_positions), max = greatest(job_positions) | sort emp_no | limit 10 | keep job_positions, min, max.

astefan · 2023-08-21T08:16:39Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/math.csv-spec

+
+least
+// tag::least[]
+ROW a = 10, b=20


Whitespacing is inconsistent.

astefan · 2023-08-21T08:17:12Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/math.csv-spec

+
+greatest
+// tag::greatest[]
+ROW a = 10, b=20


Same here about inconsistency.

astefan · 2023-08-21T09:17:51Z

docs/reference/esql/functions/greatest.asciidoc

+include::{esql-specs}/math.csv-spec[tag=greatest-result]
+|===
+
+NOTE: When run on `keyword` or `text` fields, this'll return the last string


Is this about multi-values support?

They are like most of our other functions at this point and don't support multivalued fields. This is a case where it'd make a lot of sense to take, like, mv_min for least.

It's about what LEAST and GREATEST mean when you use them with string style fields. Let me see about clarifying.

alex-spies

Basically LGTM, I only wonder about handling/testing of some edge cases like empty varargs or multi-value fields.

alex-spies · 2023-08-21T08:55:53Z

docs/reference/esql/functions/greatest.asciidoc

+[[esql-greatest]]
+=== `GREATEST`
+
+Returns the maximum value from many columns.


nit/question: does it make sense to mention behavior for multi-value columns? I'd be tempted to do something like

row a=[1,2,3], b = 0 | eval greatest(a,b)

and would expect to receive either 0 or an error, or null with a warning.

docs/reference/esql/functions/greatest.asciidoc

alex-spies · 2023-08-21T09:07:54Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/io/stream/PlanNamedTypes.java

+    static final Map<String, BiFunction<Source, List<Expression>, ScalarFunction>> VARARG_CTORS = Map.ofEntries(
+        entry(name(Case.class), Case::new),
+        entry(name(Coalesce.class), Coalesce::new),
+        entry(name(Greatest.class), Greatest::new),
+        entry(name(Least.class), Least::new)
+    );


question (I'm not fully familiar with the code base yet, so this is going to sound naive): It seems like instead of a map, one could also define an enum representing the vararg functions, or maybe have them all implement an interface that is used by vararg functions. Why do we go with a map?

They could all implement an interface, namely Writeable. Then we wouldn't have any of these. But we ended up going this way to prevent us from having to implement that interface on QL code. To be honest, I wonder if, now that we've merged to main, we should back that out.

Implementing Writable means touching all the QL code (expression, attribute, etc..) instead of having one class that handles all the messy details of writing and another for reading.

alex-spies · 2023-08-21T09:16:52Z

...l/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/VaragsTestCases.java

+/**
+ * Builds test cases for variable argument functions.
+ */
+public class VaragsTestCases {


naming bikeshed (might be inconsistent with other names, is meant absolutely optional): Wouldn't VarargsTestCaseBuilder be more precise? At first glance, I thought that an instance of this represents a fixed test case, rather than supplying an arbitrary number of test cases.

VarargsTestCaseBuilder is more precise I think.

...l/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/VaragsTestCases.java

alex-spies · 2023-08-21T09:23:36Z

...rc/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/GreatestTests.java

+
+import static org.hamcrest.Matchers.equalTo;
+
+public class GreatestTests extends AbstractFunctionTestCase {


nit: maybe a test case would be nice that documents behavior unter multi-value fields, even if it just documents that multi-value fueilds results in an error/warning.

...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Greatest.java

alex-spies · 2023-08-21T09:33:17Z

...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Greatest.java

+
+    @Evaluator(extraName = "Int")
+    static int process(int[] values) {
+        int max = values[0];


nit: can't this be out of bounds? Not sure the case is handled when the number of varargs is zero.

The function requires at least one argument. Or, at least, it should. Let me make sure there's a test for that case.

Looks like I've made this error a few times!

alex-spies · 2023-08-21T09:35:09Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/math.csv-spec

+
+g:integer
+1234
+;


question/nit: do we allow varargs functions to take 0 arguments? IMHO, a test case for this would be good, either as int test (if it's allowed) or as unit test (if it's supposed to error out).

I'm adding some more tests asserting that we don't support 0 arguments. Actually, none of our varargs functions should allow 0 arguments. That requires a bit of fiddling on the constructors which I'll do.

astefan · 2023-08-21T09:50:40Z

I'd also add one more test that's a bit more complex: from employees | eval min_plus_max = to_int(least(salary_change) + greatest(salary_change)), double = to_int(salary_change * 2), are_equal = to_int(least(salary_change) + greatest(salary_change)) == to_int(salary_change * 2) | sort emp_no | limit 10 | keep salary_change, min_plus_max, double, are_equal

nik9000

@alex-spies and @astefan this is ready for another round I think! I've added multivalue fields to the VarargsTestCaseBuilder. I've added a little bit to the docs. But just a little. Let me know what you think!

alex-spies

LGTM! Only left remarks/questions that are all meant to be highly optional.

alex-spies · 2023-08-22T15:38:47Z

.../src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/Case.java

 import java.util.function.Function;
 import java.util.function.Supplier;
 import java.util.stream.IntStream;
+import java.util.stream.Stream;


praise: yay for complie-time enforcement of at least 1 argument.

alex-spies · 2023-08-22T15:41:56Z

...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Greatest.java

+            );
        }
-        throw new UnsupportedOperationException();
+        throw new QlIllegalArgumentException("unsupported type [" + dataType + "]");


nit: you can use EsqlIllegalArgumentException::illegalDataType, although this code should likely never be reached, anyway.

I didn't know this was there. Happy to use it.

@alex-spies Better to move that utility method from the exception class to an util class specialized for the topic - see TypeResolutions/ SqlTypeResolution(we would have to make on for ESQL) orEsqlDataTypes`

Will move that @costin!

...rc/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/GreatestTests.java

alex-spies · 2023-08-22T15:48:24Z

...rc/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/GreatestTests.java

    @Override
    protected Greatest build(Source source, List<Expression> args) {
-        return new Greatest(Source.EMPTY, args.stream().toList());
+        return new Greatest(Source.EMPTY, args.get(0), args.subList(1, args.size()));


nit: maybe worth adding a comment why the source argument is unused/ignored.

We shouldn't ignore it. It doesn't really add much to the test, but if we have it we should pass it.

...c/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/nulls/CoalesceTests.java

alex-spies · 2023-08-22T15:58:28Z

...test/java/org/elasticsearch/xpack/esql/expression/function/scalar/VaragsTestCaseBuilder.java

+        return this;
+    }
+
+    public VaragsTestCaseBuilder expectBoolean(Function<Stream<boolean[]>, Optional<boolean[]>> expectedBoolean) {


thought: I wonder if we could reduce the code duplication in this file via generics, although that may be more pain than is worth it given that we probably need unboxed primitives.

astefan · 2023-08-22T16:15:36Z

docs/reference/esql/functions/least.asciidoc

+|===
+
+NOTE: When run on `keyword` or `text` fields, this'll return the first string
+      in alphabetical order. When run on `boolean` columns this will return


I think this needs to be "... this will return false if any values are false".

astefan · 2023-08-22T16:16:46Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/math.csv-spec

+10
+;
+
+leastGreatestMany


astefan · 2023-08-22T16:25:54Z

...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Greatest.java

+ * 2.0.
+ */
+
+package org.elasticsearch.xpack.esql.expression.function.scalar.math;


Doesn't really matter, but the package this is in is kind of an unusual choice. Is not really a great option, but if I would have to choose I'd go with org.elasticsearch.xpack.esql.expression.function.scalar.conditional instead.

I think it is like max, I can move it.

astefan

LGTM 👍

elasticsearchmachine · 2023-08-22T17:31:30Z

Hi @nik9000, I've created a changelog YAML for you.

ESQL: LEAST and GREATEST functions

ad83b30

Adds `LEAST` and `GREATEST` functions to find the min or max of the values in many colunms.

nik9000 added >feature :Analytics/ES|QL AKA ESQL v8.11.0 labels Aug 17, 2023

nik9000 requested a review from not-napoleon August 17, 2023 21:35

elasticsearchmachine added the Team:QL (Deprecated) Meta label for query languages team label Aug 17, 2023

More tests

8a77592

nik9000 force-pushed the column_min branch from 0521f57 to 8a77592 Compare August 17, 2023 21:36

nik9000 requested a review from astefan August 17, 2023 21:37

nik9000 mentioned this pull request Aug 17, 2023

ESQL: Functions! #98545

Open

astefan reviewed Aug 21, 2023

View reviewed changes

alex-spies reviewed Aug 21, 2023

View reviewed changes

nik9000 added 5 commits August 21, 2023 10:11

Fixup varargs

f09e7a7

More

ec2e4f1

More

8ae145c

Last one?

6b72f3e

Extra docs

8b8d633

nik9000 commented Aug 22, 2023

View reviewed changes

nik9000 added 2 commits August 22, 2023 10:46

Merge branch 'main' into column_min

86481c3

Merge more

e39033c

alex-spies approved these changes Aug 22, 2023

View reviewed changes

astefan reviewed Aug 22, 2023

View reviewed changes

x-pack/plugin/esql/qa/testFixtures/src/main/resources/math.csv-spec

10

;

leastGreatestMany

Copy link
Copy Markdown

Contributor

astefan Aug 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

astefan reviewed Aug 22, 2023

View reviewed changes

astefan approved these changes Aug 22, 2023

View reviewed changes

nik9000 added 3 commits August 22, 2023 13:12

Move things

deb2e60

Move

a551ee8

doc fix

f36a87c

nik9000 added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Aug 22, 2023

Update docs/changelog/98630.yaml

c427bb1

elasticsearchmachine merged commit 65ea90d into elastic:main Aug 22, 2023

nik9000 deleted the column_min branch August 22, 2023 18:15


		import static org.hamcrest.Matchers.equalTo;

		public class GreatestTests extends AbstractFunctionTestCase {

Conversation

nik9000 commented Aug 17, 2023

Uh oh!

github-actions bot commented Aug 17, 2023

Uh oh!

elasticsearchmachine commented Aug 17, 2023

Uh oh!

elasticsearchmachine commented Aug 17, 2023

Uh oh!

elasticsearchmachine commented Aug 17, 2023

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan commented Aug 21, 2023

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!