Skip to content

Hide Elasticsearch prefix-based pass-through query behind feature toggle#14015

Merged
kokosing merged 5 commits intotrinodb:masterfrom
kamil-rafalko:es-query-passthrough-toggle
Sep 27, 2022
Merged

Hide Elasticsearch prefix-based pass-through query behind feature toggle#14015
kokosing merged 5 commits intotrinodb:masterfrom
kamil-rafalko:es-query-passthrough-toggle

Conversation

@kamil-rafalko
Copy link
Copy Markdown
Member

@kamil-rafalko kamil-rafalko commented Sep 6, 2022

Description

Hides Elasticsearch prefix-based pass-through query behind a feature toggle as discussed here #13390

The prefix-based pass-through query is deprecated. the raw_query table function should be used instead. To emphasize the fact that prefix-based pass-through query is deprecated before removing it at all, we decided to hide it behind the feature toggle.

Release notes

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

@cla-bot cla-bot bot added the cla-signed label Sep 6, 2022
@kamil-rafalko kamil-rafalko force-pushed the es-query-passthrough-toggle branch 4 times, most recently from b72026e to 650405b Compare September 7, 2022 05:53
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.

Use PLANNER_CONTEXT.getTypeManager()

then you can revert TestingTypeManager changes

@findepi findepi requested a review from wendigo September 7, 2022 15:47
@kamil-rafalko kamil-rafalko force-pushed the es-query-passthrough-toggle branch from 650405b to fc104b1 Compare September 8, 2022 05:19
@github-actions github-actions bot added the docs label Sep 8, 2022
@kamil-rafalko kamil-rafalko force-pushed the es-query-passthrough-toggle branch from 26c958a to f526f6a Compare September 8, 2022 07:56
@kamil-rafalko kamil-rafalko marked this pull request as ready for review September 8, 2022 08:05
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.

I am not sure if it comes from you or is it is a bad rebase.

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.

please invert the condition then you can return fast and avoid indentation. You will avoid changing lines. Easier to review ;)

if (!constraint) {
  throw exception
}
the old code

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.

We don't do such testing ;) We try to avoid plumbing. Instead please create a new test class that extends abstract test query framewok and create new query runner that query pass through disabled. Then you can simply do assertQueryFail

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.

it is default value for boolean field

@kamil-rafalko kamil-rafalko force-pushed the es-query-passthrough-toggle branch 2 times, most recently from 54fd7c5 to 8e92c83 Compare September 11, 2022 13:44
@kamil-rafalko kamil-rafalko force-pushed the es-query-passthrough-toggle branch from 8e92c83 to facb98a Compare September 11, 2022 13:47
@findepi
Copy link
Copy Markdown
Member

findepi commented Sep 12, 2022

Hide Elasticsearch pass-through query behind feature toggle

We have two "pass-through queries"

  • io.trino.plugin.elasticsearch.ElasticsearchMetadata#PASSTHROUGH_QUERY_SUFFIX
  • io.trino.plugin.elasticsearch.ptf.RawQuery

please make sure the commit title indicates which one is getting behind a toggle and also add rationale to the commit message

Copy link
Copy Markdown
Member

@kokosing kokosing 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 to me % comments and CI

Thanks! It looks much simpler now!

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.

TestDisabledLegacyPassThroughQuery

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.

Please recommend that PTF (table function based) query pass through should be used instead.

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.

@findepi I would use elasticsearch.legacy-pass-through-query.enabled. WDYT?

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.

i am ok either way

@findepi
Copy link
Copy Markdown
Member

findepi commented Sep 12, 2022

ES build is failing

@kamil-rafalko kamil-rafalko force-pushed the es-query-passthrough-toggle branch 2 times, most recently from f99da85 to 65f9093 Compare September 13, 2022 20:13
@kamil-rafalko kamil-rafalko changed the title Hide Elasticsearch pass-through query behind feature toggle Hide Elasticsearch prefix-based pass-through query behind feature toggle Sep 13, 2022
@kamil-rafalko kamil-rafalko force-pushed the es-query-passthrough-toggle branch 6 times, most recently from de81690 to 1651d4b Compare September 19, 2022 18:48
Copy link
Copy Markdown
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

% comment. Please rebase and squash commits.

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.

You are using elasticsearch.legacy-pass-through-query.enabled in code. Please update.

@kamil-rafalko kamil-rafalko force-pushed the es-query-passthrough-toggle branch 7 times, most recently from 98cf35b to 0c0bfbe Compare September 25, 2022 20:28
@kamil-rafalko kamil-rafalko force-pushed the es-query-passthrough-toggle branch from 0c0bfbe to 4016a90 Compare September 25, 2022 21:00
@kamil-rafalko
Copy link
Copy Markdown
Member Author

@kokosing @findepi @wendigo it's ready for review again. Can you take a look?

@kokosing kokosing merged commit 737b3a7 into trinodb:master Sep 27, 2022
@kokosing
Copy link
Copy Markdown
Member

Thank you!

@github-actions github-actions bot added this to the 398 milestone Sep 27, 2022
@colebow
Copy link
Copy Markdown
Member

colebow commented Sep 28, 2022

Thank you for including the docs for this change as well!

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

Development

Successfully merging this pull request may close these issues.

4 participants