[ES|QL] Optional named arguments for function in map #118619

fang-xing-esql · 2024-12-12T21:08:20Z

This PR is to support named arguments for functions - provided in map format. The named arguments are provided as constant/literal values, in comma separated name:value pairs, as described in an example below. The types of the values can be string, numeric, boolean or an array of them, null is not supported as a valid value.

FROM test | EVAL x = function(field, {"stringArg":"value", "intArg":1, "doubleArg":2.0, "boolArg":true, "arrayArg":["a", "b"]})

This map format is only supported by function arguments, and it is not available in the other places in the query. This PR does not introduce a Map type in the ES|QL.

Internally the map is represented as a MapExpression that hosts a list of EntryExpressions, it can be a(optional) child of a function.

A couple of test functions(MapCount, LogWithBaseInMap) are included in this PR to validate that MapExpression and EntryExpression work as expected as function arguments, serialization works properly, and the related docs can be generated properly. These two test functions are under snapshot only.

MapCount does not require serialization, it is evaluated at planning time by constant folding at coordinator node, it is not sent to the data nodes.

LogWithBaseInMap requires serialization, and the two related tests - LogWithBaseInMapTests and LogWithBaseInMapSerializationTests will not be merged, as they may trigger the generation of unnecessary docs, they are there to validate serialization and doc generation work properly.

elasticsearchmachine · 2024-12-12T21:08:44Z

Hi @fang-xing-esql, I've created a changelog YAML for you.

...java/org/elasticsearch/xpack/esql/expression/function/scalar/math/LogWithBaseInMapTests.java

elasticsearchmachine · 2024-12-23T19:35:04Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

carlosdelest · 2025-01-09T18:17:56Z

...src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractFunctionTestCase.java

 builder.startObject();
 builder.field("name", arg.name());
- if (arg.mapParam()) {
+ if (arg instanceof EsqlFunctionRegistry.MapArgSignature mp) {


Nit - This different behaviour for ArgSignature / MapArgSignature could be included in the specific classes in a method like definition(XcontentBuilder builder) instead of using a instanceof

carlosdelest · 2025-01-09T18:22:52Z

...ugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/MapExpression.java

+/**
+ * Represent a collect of key-value pairs.
+ */
+public class MapExpression extends Expression implements Map<Expression, Expression> {


Is this something planned? I don't think we need to implement the Map interface on this class, but expose the underlying Map

carlosdelest · 2025-01-10T09:04:12Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java

+ Expression value = expression(entry.constant());
+ if (value instanceof Literal l) {
+ if (l.dataType() == NULL) {
+ throw new ParsingException(source(ctx), "Invalid named function argument [{}], NULL is not supported", l);


Can we add the function arg name (key) so the error message is more actionable?

carlosdelest · 2025-01-10T09:04:23Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java

+ namedArgs.add(new Literal(source(entry.string()), key, KEYWORD));
+ namedArgs.add(l);
+ } else {
+ throw new ParsingException(source(ctx), "Invalid named function argument [{}], only constant value is supported", value);


Same here, can we add the function arg name?

carlosdelest · 2025-01-10T09:35:04Z

.../main/java/org/elasticsearch/xpack/esql/expression/function/scalar/map/LogWithBaseInMap.java

+ return map;
+ }
+
+ private TypeResolution validateOptions() {


It's not needed for this PR, but we will need to provide a validation mechanism for map options so they are checked for valid keys, valid types etc

carlosdelest · 2025-01-10T09:38:58Z

...ugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/MapExpression.java

+ entryExpressions.add(new EntryExpression(key.source(), key, value));
+ map.put(key, value);
+ if (key.foldable()) {
+ this.keyFoldedMap.put(key.fold(), value);


We should check that there are no duplicate key entries, otherwise users could add them by mistake, like

ROW value = 8.0 | EVAL l = round(log_with_base_in_map(value, {"base": 9.0, "base": 3.0}

which can be difficult to debug

I'll do the validation on duplicated keys in the parser, so that we stop as soon as duplicated keys are found.

carlosdelest · 2025-01-10T09:43:49Z

docs/reference/esql/functions/parameters/log_with_base_in_map.asciidoc

+Numeric expression. If `null`, the function returns `null`.
+
+`map`::
+Input value. The input is a valid constant map expression.


I wonder how to describe the map type to users so it's more informative.

Maybe something like "arguments map" or "named arguments"? "Map expression" sounds a bit weird.

I know, naming... 🤷

Yeah, I know, the naming could be tricky. How about "named arguments in map"? It may look more clear in the docs if we provide them some examples. We can have our documentation team friends review it, perhaps they have a better idea how to describe it in a more meaningful way.

I don't plan to merge this log_with_base_in_map related tests or docs when this PR is ready to merge, I use them to validate the doc generation and serialization to catch some possible issues that the Match function may hit. Do you prefer to merge the log_with_base_in_map as a snapshot function? Or shall I remove it when this is ready to merge?

"named arguments in map" sounds better - we can have some iterations on this with the docs team as you're suggesting 👍

I don't plan to merge this log_with_base_in_map related tests or docs when this PR is ready to merge

Got it, I'm thinking on match implementing this once this is merged. We can postpone naming decisions with that.

Do you prefer to merge the log_with_base_in_map as a snapshot function? Or shall I remove it when this is ready to merge?

I think it's a good idea to keep the function as a testing ground, so we don't have to re-test the map param mechanism as part of new functions. 💯

carlosdelest

LGTM, thanks for working on this @fang-xing-esql !

astefan · 2025-01-10T17:14:53Z

...ugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/MapExpression.java

+ Expression value = entries.get(i * 2 + 1);
+ entryExpressions.add(new EntryExpression(key.source(), key, value));
+ map.put(key, value);
+ if (key.foldable()) {


Can key be non-foldable or be foldable later? (as in optimization rules kicking in and propagating values and making the keys foldable)

In the implementation of MapExpression for function arguments, the keys are all Literals(foldable). If MapExpression is used for some other purposes in the future, we could have non-foldable keys, that become foldable during optimization, and this foldable map need to be updated/refreshed, if this happens, one option is to have an optimization rule to refresh foldable map of a MapExpression.

astefan · 2025-01-13T16:05:03Z

...ugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/MapExpression.java

+ if (key instanceof Expression) {
+ return map.get(key);
+ } else {
+ key = key instanceof String s ? s.toLowerCase(Locale.ROOT) : key;


Have you thought about the case sensitiveness of the keys?
ES itself, if I'm not mistaken, is case sensitive to how the elements of the queries are specified. For example, query_string query written like

{ "query": { "query_string": { "query": "(new york city) OR (big apple)", "Default_field": "content" } } }

where Default_field is used will complain that there is no such element. I know it's not the exact same thing (ES|QL and the query DSL) but we have functions - full text search - that may want to expose all sorts of parameters and users who are used to query DSL want to use the same/similar functionality in ES|QL, we might want to have a consistent behavior.
Also, allowing this (not being case sensitive) from the beginning, in case we are required to make it consistent with ES, we might not be doing it so easily (bwc concerns).

This is a good point, I didn't know that search is case-sensitive, keeping the map key string unchanged makes sense to me. Here are a couple of examples that I tried.

+ curl -u elastic:password -v -X GET 'localhost:9200/case_sensitive/_search?format=txt&pretty' -H 'Content-Type: application/json' '-d { "query": { "match": { "field1": { "Query": "a" } } } } ' { "error" : { "root_cause" : [ { "type" : "parsing_exception", "reason" : "[match] query does not support [Query]", "line" : 6, "col" : 18 } ], "type" : "parsing_exception", "reason" : "[match] query does not support [Query]", "line" : 6, "col" : 18 }, "status" : 400 } + curl -u elastic:password -v -X GET 'localhost:9200/case_sensitive/_search?format=txt&pretty' -H 'Content-Type: application/json' '-d { "query": { "query_string": { "query": "(a) or (A)", "Default_field": "field1" } } } ' { "error" : { "root_cause" : [ { "type" : "parsing_exception", "reason" : "[query_string] query does not support [Default_field]", "line" : 6, "col" : 24 } ], "type" : "parsing_exception", "reason" : "[query_string] query does not support [Default_field]", "line" : 6, "col" : 24 }, "status" : 400 }

astefan · 2025-01-13T17:12:28Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java

+ List<String> names = new ArrayList<>(ctx.entryExpression().size());
+ List<EsqlBaseParser.EntryExpressionContext> kvCtx = ctx.entryExpression();
+ for (EsqlBaseParser.EntryExpressionContext entry : kvCtx) {
+ String key = visitString(entry.string()).fold().toString().toLowerCase(Locale.ROOT); // make key case-insensitive


Why calling fold here? (folding is a function of the optimizer) This is a string here nevertheless.
Also, same thing I mentioned earlier about case sensitivity.

fold() is replaced by getText() and unquote().

astefan · 2025-01-13T17:14:41Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java

+ namedArgs.add(l);
+ names.add(key);
+ } else {
+ throw new ParsingException(


When is this possible?

Good catch! This is only possible when a parameter is provided as a map entry value. We take constants as map entry values, so parameters are valid inputs. And parameters can be for constant values, identifiers and patterns, if users misuse it and provide a parameter for identifiers or patterns, we end up with this code path. I added some tests in StatementParserTests to cover this.

astefan

LGTM

fang-xing-esql · 2025-01-16T23:45:42Z

Thanks for reviewing @ivancea @carlosdelest @astefan !

* MapExpression for functions

fang-xing-esql · 2025-01-17T06:05:28Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Questions ?

Please refer to the Backport tool documentation

* MapExpression for functions (cherry picked from commit 11fbc8c) # Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser.interp # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java

Adds options to QSTR function. #118619 added named function parameters. This PR uses this mechanism for allowing query string function parameters, so query string parameters can be used in ES|QL. Closes #120933

Adds options to QSTR function. elastic#118619 added named function parameters. This PR uses this mechanism for allowing query string function parameters, so query string parameters can be used in ES|QL. Closes elastic#120933

Adds options to QSTR function. elastic#118619 added named function parameters. This PR uses this mechanism for allowing query string function parameters, so query string parameters can be used in ES|QL. Closes elastic#120933 (cherry picked from commit ee4bcac) # Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

Adds options to QSTR function. elastic#118619 added named function parameters. This PR uses this mechanism for allowing query string function parameters, so query string parameters can be used in ES|QL. Closes elastic#120933 (cherry picked from commit ee4bcac) # Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/resources/qstr-function.csv-spec # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java

…5114) * Added optional parameters to QSTR ES|QL function (#121787) Adds options to QSTR function. #118619 added named function parameters. This PR uses this mechanism for allowing query string function parameters, so query string parameters can be used in ES|QL. Closes #120933 (cherry picked from commit ee4bcac) # Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/resources/qstr-function.csv-spec # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java * [CI] Auto commit changes from spotless * Fix compilation error * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

…5112) * Added optional parameters to QSTR ES|QL function (#121787) Adds options to QSTR function. #118619 added named function parameters. This PR uses this mechanism for allowing query string function parameters, so query string parameters can be used in ES|QL. Closes #120933 (cherry picked from commit ee4bcac) # Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java * [CI] Auto commit changes from spotless * Fix compilation error * Fix missing import * getFirst() -> get(0) * Make method public * Make asBuilder public in subclasses * Revert "Make asBuilder public in subclasses" This reverts commit f444aa6. * Revert "Make method public" This reverts commit a1d9f56. * .asQueryBuilder() -> .toQueryBuilder() * Remove extraneous change which sneaked in during backport --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

MapExpression for functions

6dd0ed2

fang-xing-esql added >enhancement :Analytics/EQL EQL querying v9.0.0 labels Dec 12, 2024

Update docs/changelog/118619.yaml

9a3724e

fang-xing-esql added the test-release Trigger CI checks against release build label Dec 12, 2024

fang-xing-esql and others added 8 commits December 12, 2024 17:18

Merge branch 'main' into mapexpression-function-arg

500f01b

Merge branch 'main' into mapexpression-function-arg

dae1ae4

Add a test function for MapExpression

42dd838

Merge branch 'main' into mapexpression-function-arg

a43cb3e

Merge branch 'main' into mapexpression-function-arg

6b02745

add tests for LogWithBaseInMap

6842c1e

Merge branch 'main' into mapexpression-function-arg

ccced73

Merge branch 'main' into mapexpression-function-arg

cf792ef

fang-xing-esql commented Dec 19, 2024

View reviewed changes

...java/org/elasticsearch/xpack/esql/expression/function/scalar/math/LogWithBaseInMapTests.java Outdated Show resolved Hide resolved

costin added :Analytics/ES|QL AKA ESQL and removed :Analytics/EQL EQL querying labels Dec 19, 2024

fang-xing-esql added 8 commits December 19, 2024 13:59

update generated docs to support map

8200c23

Merge branch 'main' into mapexpression-function-arg

15c2216

refactor FunctionDescription and make MapExpression implement Map

f37033a

remove generated files

a81a81e

refactor

5fef44d

Merge branch 'main' into mapexpression-function-arg

adf3861

more refactor

e12c2f1

Merge branch 'main' into mapexpression-function-arg

d1e122d

fang-xing-esql marked this pull request as ready for review December 23, 2024 19:34

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Dec 23, 2024

fang-xing-esql requested a review from costin December 26, 2024 14:57

fang-xing-esql added 2 commits January 9, 2025 10:29

Merge branch 'main' into mapexpression-function-arg

071817f

Merge branch 'main' into mapexpression-function-arg

87049a9

carlosdelest reviewed Jan 10, 2025

View reviewed changes

fang-xing-esql added 2 commits January 10, 2025 13:06

Merge branch 'main' into mapexpression-function-arg

9201214

update according to review comments

f06a595

carlosdelest mentioned this pull request Jan 13, 2025

ES|QL MATCH roadmap #116261

Open

carlosdelest approved these changes Jan 13, 2025

View reviewed changes

Merge branch 'main' into mapexpression-function-arg

f4fa894

astefan reviewed Jan 13, 2025

View reviewed changes

fang-xing-esql added 3 commits January 13, 2025 17:05

make map expression key case sensitive

0d54b5c

Merge branch 'main' into mapexpression-function-arg

25e8f18

add tests for case-sensitive map keys

1560c74

astefan approved these changes Jan 16, 2025

View reviewed changes

fang-xing-esql and others added 2 commits January 16, 2025 13:10

remove the test and its generated docs of test function LogWithBaseInMap

6432565

Merge branch 'main' into mapexpression-function-arg

11953b6

fang-xing-esql merged commit 11fbc8c into elastic:main Jan 16, 2025
17 checks passed

rjernst pushed a commit to rjernst/elasticsearch that referenced this pull request Jan 16, 2025

[ES|QL] Optional named arguments for function in map (elastic#118619)

48d49da

* MapExpression for functions

fang-xing-esql added the auto-backport Automatically create backport pull requests when merged label Jan 16, 2025

fang-xing-esql mentioned this pull request Jan 17, 2025

[8.x] [ES|QL] Optional named arguments for function in map (#118619) #120347

Merged

carlosdelest mentioned this pull request Jan 17, 2025

ESQL - Add Match function options #120360

Merged

carlosdelest mentioned this pull request Mar 5, 2025

Added optional parameters to QSTR ES|QL function #121787

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ES|QL] Optional named arguments for function in map #118619

[ES|QL] Optional named arguments for function in map #118619

Uh oh!

fang-xing-esql commented Dec 12, 2024 •

edited

Loading

elasticsearchmachine commented Dec 12, 2024

Uh oh!

elasticsearchmachine commented Dec 23, 2024

carlosdelest Jan 9, 2025

carlosdelest Jan 9, 2025

carlosdelest Jan 10, 2025

carlosdelest Jan 10, 2025

carlosdelest Jan 10, 2025

carlosdelest Jan 10, 2025

fang-xing-esql Jan 10, 2025

carlosdelest Jan 10, 2025

fang-xing-esql Jan 10, 2025

carlosdelest Jan 13, 2025

carlosdelest left a comment

astefan Jan 10, 2025

fang-xing-esql Jan 14, 2025

astefan Jan 13, 2025

fang-xing-esql Jan 13, 2025

astefan Jan 13, 2025

fang-xing-esql Jan 13, 2025

astefan Jan 13, 2025

fang-xing-esql Jan 13, 2025

astefan left a comment

fang-xing-esql commented Jan 16, 2025

Uh oh!

fang-xing-esql commented Jan 17, 2025

Labels

6 participants

[ES|QL] Optional named arguments for function in map #118619

[ES|QL] Optional named arguments for function in map #118619

Uh oh!

Conversation

fang-xing-esql commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

elasticsearchmachine commented Dec 12, 2024

Uh oh!

elasticsearchmachine commented Dec 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosdelest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

fang-xing-esql commented Jan 16, 2025

Uh oh!

fang-xing-esql commented Jan 17, 2025

💚 All backports created successfully

Questions ?

Labels

6 participants

fang-xing-esql commented Dec 12, 2024 •

edited

Loading