-
Notifications
You must be signed in to change notification settings - Fork 25.7k
ESQL: Add support for Full Text Functions and Lucene Pushable Predicates for Lookup Join #136104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Hi @julian-elastic, I've created a changelog YAML for you. |
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
carlosdelest
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from the full text functions perspective 👍 :
- Changes imply adding checks for lookup join conditions
- IT tests have been updated with relevant tests
alex-spies
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First round of comments: analysis
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java
Outdated
Show resolved
Hide resolved
alex-spies
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a question on the modeling of right-only filters, see below.
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/ExpressionQueryList.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/ExpressionQueryList.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Productive code reviewed and looking mighty good!
Wrapping up the tests as well. Found some things that we need to have a look at there, but nothing that'll block this for long.
I think we'll want to add the new expression joins to our generative tests, no?
Also, this generally allows all Lucene-pushable filters to be put into the ON condition. Some expressions might not consider this correct and will require an update of, say, the validation (as for full text functions). Maybe we should consider a randomized test that generates a pushable expression that we then stuff into the join condition? (We can compare the result by just running the WHERE filter on the lookup index in a separate query.)
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/ExpressionQueryList.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/ExpressionQueryList.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/ExpressionQueryList.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java
Show resolved
Hide resolved
...rc/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/FullTextFunction.java
Show resolved
Hide resolved
...rc/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/FullTextFunction.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join-expression.csv-spec
Outdated
Show resolved
Hide resolved
alex-spies
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, all done now. This still looks very good. I found another oopsie re. the capabilities used which we should fix, but in total I think we can merge after the next iteration, most likely. Nice!
x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join-expression.csv-spec
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join-expression.csv-spec
Show resolved
Hide resolved
x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java
Show resolved
Hide resolved
...gin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/ExpressionQueryList.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/ParsingTests.java
Show resolved
Hide resolved
|
@vadimkibana @stratoula This PR is not making any changes to the antler grammar, but it does allow for more complex expressions as part of the lookup join on condition. Do you need any changes on your side for this? Please have this in mind when working on elastic/kibana#236939 |
|
Thanx @julian-elastic noted! |
alex-spies
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thanks @julian-elastic !
|
|
||
| FROM airports | ||
| | RENAME abbrev AS airport_code | ||
| | LOOKUP JOIN airport_city_boundaries ON airport_code == abbrev AND ST_CONTAINS(TO_GEOSHAPE("POLYGON((-0.3 51.0, -0.1 51.0, -0.1 51.2, -0.3 51.2, -0.3 51.0))"), city_boundary) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@craigtaverner , you may want to see this :) woohoo
...gin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java
Show resolved
Hide resolved
…tes for Lookup Join (elastic#136104) • Added Full Text Functions Support in Lookup Join Expressions: Extended lookup join functionality to support full-text functions like MATCH() as part of join conditions, enabling text search capabilities within lookup operations. • Enhanced Lucene Pushable Filters Support. Implemented support for any Lucene pushable filters that contain only fields from the right side of the lookup join, allowing for more complex filtering conditions. At least one condition relating the left and right side of the Lookup Join is still required. This commit may include content that was generated or assisted by GenAI tools Cursor, Gemini CLI and/or Github Copilot
• Added Full Text Functions Support in Lookup Join Expressions: Extended lookup join functionality to support full-text functions like MATCH() as part of join conditions, enabling text search capabilities within lookup operations.
• Enhanced Lucene Pushable Filters Support. Implemented support for any Lucene pushable filters that contain only fields from the right side of the lookup join, allowing for more complex filtering conditions. At least one condition relating the left and right side of the Lookup Join is still required.
Full text function
OR of Lucene pushable filters on fields from the lookup index
OR and NOT of Lucene pushable filters on fields from the lookup index