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

Merged
merged 4 commits into from
Jan 17, 2025
Merged

Add support for dereference pushdown in Elasticsearch #23839

merged 4 commits into from
Jan 17, 2025

Conversation

striderarun
Copy link
Contributor

@striderarun striderarun commented Oct 19, 2024 •
edited by mosabua
Loading

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. 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}`23839`)

@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
Contributor 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
Contributor 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
Contributor 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
Contributor 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.

@ebyhr ebyhr mentioned this pull request Oct 27, 2024
Closed
@striderarun
Copy link
Contributor Author

striderarun commented Oct 29, 2024 •
edited
Loading

@mayankvadariya @krvikash Thanks so much for taking the time to review the PR and providing your comments.
@ebyhr Do you know any other reviewers who can help review the PR and take it to conclusion?

@ebyhr ebyhr requested a review from martint October 30, 2024 03:02
@striderarun
Copy link
Contributor Author

striderarun commented Nov 4, 2024 •
edited
Loading

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

@striderarun
Copy link
Contributor Author

@ebyhr I see that my PR branch is now out of date with master. Should I do "Update with rebase" to sync the branch with master?

@ebyhr
Copy link
Member

ebyhr commented Nov 4, 2024

@striderarun You don't need to do that unless conflicts happen.

@striderarun
Copy link
Contributor Author

@martint Requesting your help with this review.

@striderarun striderarun force-pushed the arun/dereference_pushdown_elasticsearch branch 4 times, most recently from fd82230 to 5bd88fd Compare November 29, 2024 02:42
@striderarun
Copy link
Contributor Author

@ebyhr I see there is a conflict with one of the files. I need to resolve the conflict in this PR before it can be merged right?

@mayankvadariya
Copy link
Contributor

@ebyhr I see there is a conflict with one of the files. I need to resolve the conflict in this PR before it can be merged right?

yes you need to rebase if there are any conflicts. However, I don't see a conflict at the bottom of the PR which usually I see if there is any.

@striderarun
Copy link
Contributor Author

@ebyhr @martint If there are no more comments or actions, does this PR look good to be merged?

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 6, 2025

This pull request has gone a while without any activity. Tagging for triage help: @mosabua

@github-actions github-actions bot added the stale label Jan 6, 2025
@striderarun
Copy link
Contributor Author

@mosabua @ebyhr @martint Please advise on next steps.

@github-actions github-actions bot removed the stale label Jan 15, 2025
@martint
Copy link
Member

martint commented Jan 17, 2025 •
edited
Loading

@striderarun, this looks great now. Can you resolve the conflict? I'll merge it then.

@striderarun striderarun force-pushed the arun/dereference_pushdown_elasticsearch branch from 5bd88fd to 6b5a861 Compare January 17, 2025 18:55
@striderarun
Copy link
Contributor Author

@striderarun, this looks great now. Can you resolve the conflict? I'll merge it then.

@martint Thanks for checking! I have resolved the conflict.

@martint martint merged commit b012f75 into trinodb:master Jan 17, 2025
17 checks passed
@github-actions github-actions bot added this to the 469 milestone Jan 17, 2025
@mosabua mosabua mentioned this pull request Jan 17, 2025
Open
@mayankvadariya
Copy link
Contributor

🎉

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

@mayankvadariya mayankvadariya mayankvadariya left review comments

@krvikash krvikash krvikash left review comments

@martint martint martint left review comments

Assignees

@striderarun striderarun

Labels
cla-signed docs
Milestone
469
Development

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