Skip to content

ESQL: Add parsing for a LOOKUP command#109040

Merged
elasticsearchmachine merged 7 commits intoelastic:mainfrom
nik9000:lookup_syntax
May 28, 2024
Merged

ESQL: Add parsing for a LOOKUP command#109040
elasticsearchmachine merged 7 commits intoelastic:mainfrom
nik9000:lookup_syntax

Conversation

@nik9000
Copy link
Copy Markdown
Member

@nik9000 nik9000 commented May 24, 2024

This command will serve as a sort of "inline" enrich. This commit itself is mostly antlr generated code and paranoid tests that the new LOOKUP keyword doesn't clash with any variables named lookup.

I've also marked our ANTLR generated files as linguist-generated which causes them to be hidden by default in github's UI. You can still click a button to see them if you like. See https://docs.github.com/en/repositories/working-with-files/managing-files/customizing-how-changed-files-appear-on-github

This command will serve as a sort of "inline" enrich. This commit itself
is mostly antlr generated code and paranoid tests that the new `LOOKUP`
keyword doesn't clash with any variables named `lookup`.
@nik9000 nik9000 requested review from alex-spies and costin May 24, 2024 17:21
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label May 24, 2024
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

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

Comment on lines +7 to +10
x-pack/plugin/esql/src/main/antlr/*.tokens linguist-generated=true
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/*.interp linguist-generated=true
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseLexer*.java linguist-generated=true
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser*.java linguist-generated=true
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Learned a new thing today!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yeah! I saw kibana doing it for something so I stole their trick.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ha! interesting. what does the IDE do, if anything, with this ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So far as I can tell, nothing.

Copy link
Copy Markdown
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

LGTM

@nik9000
Copy link
Copy Markdown
Member Author

nik9000 commented May 24, 2024

BWC failure is real - LOOKUP can't be used there. Easy enough.

Copy link
Copy Markdown
Contributor

@alex-spies alex-spies left a comment

Choose a reason for hiding this comment

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

Looks good; I think we could make the table name less general, though. (See below.)



//
// Make sure that the new LOOKUP syntax doesn't clash with any existing things
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

++ to these tests, very nice!

;

lookupCommand
: LOOKUP tableName=qualifiedNamePattern ON matchFields=qualifiedNamePatterns
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm really not sure if this should use a qualified name pattern for the table name. IMHO the table name should be somewhat in line with FROM's index name and ENRICH's policy name. Qualified name patterns are usually used for field names, so we can do stuff like

KEEP `some-thing`.`something-else`.yet_something_else

FROM uses an INDEX_UNQUOTED_IDENTIFIER, ENRICH a similar ENRICH_POLICY_NAME_BODY.

Maybe we just align with FROM here and use INDEX_UNQUOTED_IDENTIFIER as well, unless table names need to be more general than that?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll give it a go.

@nik9000 nik9000 added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label May 28, 2024
nik9000 added 3 commits May 28, 2024 09:23
This removes the shim layers for `nested` that we inherited from QL.
ESQL doesn't support nested yet and when it does support nested its not
going to use the same tricks that QL uses.
@elasticsearchmachine elasticsearchmachine merged commit e4cb2c9 into elastic:main May 28, 2024
@nik9000 nik9000 deleted the lookup_syntax branch May 28, 2024 17:32
@stratoula stratoula added the ES|QL-ui Impacts ES|QL UI label Jun 3, 2024
elasticsearchmachine pushed a commit that referenced this pull request Jun 7, 2024
This adds support for `LOOKUP`, a command that implements a sort of
inline `ENRICH`, using data that is passed in the request:

```
$ curl -uelastic:password -HContent-Type:application/json -XPOST \
    'localhost:9200/_query?error_trace&pretty&format=txt' \
-d'{
    "query": "ROW a=1::LONG | LOOKUP t ON a",
    "tables": {
        "t": {
            "a:long":     [    1,     4,     2],
            "v1:integer": [   10,    11,    12],
            "v2:keyword": ["cat", "dog", "wow"]
        }
    },
    "version": "2024.04.01"
}'
      v1       |      v2       |       a       
---------------+---------------+---------------
10             |cat            |1
```

This required these PRs: * #107624 * #107634 * #107701 * #107762 *
#107923 * #107894 * #107982 * #108012 * #108020 * #108169 * #108191 *
#108334 * #108482 * #108696 * #109040 * #109045

Closes #107306
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) ES|QL-ui Impacts ES|QL UI >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.15.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants