Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for dereference pushdown in Elasticsearch #23839

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add support for dereference pushdown in Elasticsearch #23839

wants to merge 4 commits into from
+1,876 −47

Conversation

striderarun
Copy link

Description

Add dereference pushdown support in ElasticSearch. (Issue: #23069)

Learn more about dereference pushdown at
https://trino.io/docs/current/optimizer/pushdown.html#dereference-pushdown
https://trino.io/blog/2020/08/14/dereference-pushdown.html

Additional context and related issues

The feature is enabled by default. Can be disabled by setting elasticsearch.projection-pushdown-enabled configuration property or elasticsearch.projection_pushdown_enabled session property to false.

Based on reference implementation for OpenSearch dereference pushdown in #22646

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( x) Release notes are required, with the following suggested text:

# Elasticsearch
*Improve performance of queries that reference nested fields from Elasticsearch documents. ({issue}`23069`)

@cla-bot cla-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot added the docs label Oct 19, 2024
@striderarun striderarun changed the title Add support for dereference pushdown in Elasticsearch https://github.com/trinodb/trino/issues/23069 Add support for dereference pushdown in Elasticsearch Oct 19, 2024
@striderarun
Copy link
Author

@mayankvadariya Can you please help with an initial review ?

@cla-bot cla-bot

This comment was marked as outdated.

Sign in to view
@striderarun striderarun force-pushed the arun/dereference_pushdown_elasticsearch branch from dd51580 to 1be6202 Compare October 20, 2024 05:37
@cla-bot cla-bot

This comment was marked as outdated.

Sign in to view
@krvikash
Copy link
Contributor

Hi @striderarun Could you please fix the check-commit CI failure?

@cla-bot cla-bot

This comment was marked as outdated.

Sign in to view
@striderarun striderarun force-pushed the arun/dereference_pushdown_elasticsearch branch from 7e37b07 to 1be6202 Compare October 21, 2024 15:56
@cla-bot cla-bot

This comment was marked as outdated.

Sign in to view
@striderarun
Copy link
Author

@krvikash @mayankvadariya The CI failure is due to Checkstyle violation "Error: src/test/java/io/trino/plugin/elasticsearch/ElasticsearchServer.java:[54] (regexp) RegexpSinglelineJava: No new line before extends/implements".

But as you can see in ElasticsearchServer.java file https://github.com/trinodb/trino/pull/23839/files#diff-b59c7df884fdb8b4e27c9ecdb91f8e9de59feda646bc1cad16d300d7a231d5a9, implements Closeable is already on a newline.
Moreover, building Trino locally with these changes on my MacBook does not reproduce this problem. Running ./mvnw validate locally is also successful and does not report any Checkstyle violations.

I am also using the Airlift codestyle in my IntelliJ IDE as documented here: https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#code-style

I am not sure if I am overlooking something since this is my first pull request in Trino. Do you see anything obvious that I am missing here that explains the Checkstyle violation?
Appreciate your help on this!

@striderarun striderarun force-pushed the arun/dereference_pushdown_elasticsearch branch from 1be6202 to c17ec04 Compare October 22, 2024 03:56
@cla-bot cla-bot

This comment was marked as outdated.

Sign in to view
@ebyhr
Copy link
Member

ebyhr commented Oct 23, 2024

@cla-bot check

@cla-bot cla-bot bot added the cla-signed label Oct 23, 2024
@cla-bot cla-bot
Copy link

cla-bot bot commented Oct 23, 2024

The cla-bot has been summoned, and re-checked this pull request!

@striderarun
Copy link
Author

@mayankvadariya Would greatly appreciate your help in reviewing this PR when you get a chance!

krvikash
krvikash reviewed Oct 25, 2024
View reviewed changes
Copy link
Contributor

@krvikash krvikash left a comment

Choose a reason for hiding this comment

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

Skimmed, Overall LGTM. Still need to review the test classes.

plugin/trino-elasticsearch/src/test/java/io/trino/plugin/elasticsearch/ElasticsearchServer.java Outdated Show resolved Hide resolved
plugin/trino-elasticsearch/src/main/java/io/trino/plugin/elasticsearch/ElasticsearchConfig.java Outdated Show resolved Hide resolved
...n/trino-elasticsearch/src/main/java/io/trino/plugin/elasticsearch/ElasticsearchMetadata.java Outdated Show resolved Hide resolved
...n/trino-elasticsearch/src/main/java/io/trino/plugin/elasticsearch/ElasticsearchMetadata.java Outdated Show resolved Hide resolved
...n/trino-elasticsearch/src/main/java/io/trino/plugin/elasticsearch/ElasticsearchMetadata.java Outdated Show resolved Hide resolved
...n/trino-elasticsearch/src/main/java/io/trino/plugin/elasticsearch/ElasticsearchMetadata.java Outdated Show resolved Hide resolved
...n/trino-elasticsearch/src/main/java/io/trino/plugin/elasticsearch/ElasticsearchMetadata.java Outdated Show resolved Hide resolved
...n/trino-elasticsearch/src/main/java/io/trino/plugin/elasticsearch/ElasticsearchMetadata.java Outdated Show resolved Hide resolved
...lasticsearch/src/test/java/io/trino/plugin/elasticsearch/BaseElasticsearchConnectorTest.java Outdated Show resolved Hide resolved
...c/test/java/io/trino/plugin/elasticsearch/TestElasticsearchComplexTypePredicatePushDown.java Show resolved Hide resolved
@striderarun striderarun force-pushed the arun/dereference_pushdown_elasticsearch branch from c17ec04 to 42fd5aa Compare October 25, 2024 19:19
@striderarun striderarun self-assigned this Oct 25, 2024
@striderarun striderarun force-pushed the arun/dereference_pushdown_elasticsearch branch from 42fd5aa to 033816b Compare October 25, 2024 19:36
mayankvadariya
mayankvadariya reviewed Oct 25, 2024
View reviewed changes
Copy link
Contributor

@mayankvadariya mayankvadariya left a comment

Choose a reason for hiding this comment

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

Please try using client from OpenSearchServer to see if it helps test pass.

...ch/src/test/java/io/trino/plugin/elasticsearch/TestElasticsearchProjectionPushdownPlans.java Outdated Show resolved Hide resolved
...c/test/java/io/trino/plugin/elasticsearch/TestElasticsearchComplexTypePredicatePushDown.java Outdated Show resolved Hide resolved
...c/test/java/io/trino/plugin/elasticsearch/TestElasticsearchComplexTypePredicatePushDown.java Outdated Show resolved Hide resolved
...c/test/java/io/trino/plugin/elasticsearch/TestElasticsearchComplexTypePredicatePushDown.java Outdated Show resolved Hide resolved
...ch/src/test/java/io/trino/plugin/elasticsearch/TestElasticsearchProjectionPushdownPlans.java Outdated Show resolved Hide resolved
...lasticsearch/src/test/java/io/trino/plugin/elasticsearch/BaseElasticsearchConnectorTest.java Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/elasticsearch.md Outdated Show resolved Hide resolved
@striderarun striderarun force-pushed the arun/dereference_pushdown_elasticsearch branch from 033816b to 38a27d3 Compare October 25, 2024 22:19
@striderarun
Copy link
Author

Please try using client from OpenSearchServer to see if it helps test pass.

Thanks for your comment, @mayankvadariya. The tests passed with your suggestion.

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

@krvikash krvikash krvikash left review comments

@mayankvadariya mayankvadariya mayankvadariya left review comments

Assignees

@striderarun striderarun

Labels
cla-signed docs
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@striderarun @krvikash @ebyhr @mayankvadariya