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 passthrough Elasticsearch queries #15900

Merged
merged 1 commit into from
Apr 16, 2021
Merged

Add support for passthrough Elasticsearch queries #15900

merged 1 commit into from
Apr 16, 2021

Conversation

v-jizhang
Copy link
Contributor

@v-jizhang v-jizhang commented Apr 2, 2021 •
edited
Loading

Cherry-pick of trinodb/trino#3735
This allows running queries over the results of a raw Elasticsearch query.
It extends the syntax of the enhanced ES table names with the following:

SELECT * FROM es.default."$query:"

The query is base32-encoded to avoid having to deal with escaping quotes and case
sensitivity issues in table identifiers.

The result of these query tables is a table with a single row and a single column
named "result" of type JSON.

Co-authored-by: Martin Traverso [email protected]

== RELEASE NOTES ==

ElasticSearch Connector Changes
* Add support for passthrough Elasticsearch queries.
  The Elasticsearch connector allows you to embed any valid Elasticsearch query, that uses the Elasticsearch [Query DSL](https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl.html) in your SQL query.

@aweisberg aweisberg self-requested a review April 5, 2021 16:38
@v-jizhang v-jizhang force-pushed the elastic_passthrough branch from 189d06c to c30b2e9 Compare April 5, 2021 19:19
@aweisberg aweisberg requested a review from zhenxiao April 7, 2021 23:12
@aweisberg
Copy link
Contributor

The Trino PR says it depends on https://github.com/trinodb/trino/pull/3718/files, but it looks to me like we already have it?
@zhenxiao tagging you also for review because you have done a lot of work in this area.

@aweisberg
Copy link
Contributor

One note, we should also bring the elastic search connector doc up to date trinodb/trino@f776741

@v-jizhang
Copy link
Contributor Author

The Trino PR says it depends on https://github.com/trinodb/trino/pull/3718/files, but it looks to me like we already have it?
@zhenxiao tagging you also for review because you have done a lot of work in this area.

Yes, we have it already.

@v-jizhang
Copy link
Contributor Author

One note, we should also bring the elastic search connector doc up to date trinodb/trino@f776741

I'll add it.

aweisberg
aweisberg suggested changes Apr 8, 2021
View reviewed changes
Copy link
Contributor

@aweisberg aweisberg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks good to me modulo the doc changes and the unresolved question about that error code. Whether we should take all the semantic error code removal changes or whether there is a different better error code to use in this case.

Pulling up to a higher level I was a little surprised the overlap in functionality between plain text pass through queries and base32 passthrough queries. I wonder what drove the introduction of base32 encoded passthrough.

I also found during review that we are missing limit and filter pushdown for ES and that isn't in the list of targeted improvements.

presto-spi/src/main/java/com/facebook/presto/spi/ColumnMetadata.java
@@ -151,4 +172,76 @@ public boolean equals(Object obj)
Objects.equals(this.extraInfo, other.extraInfo) &&
Objects.equals(this.hidden, other.hidden);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't all of trinodb/trino@6415b9a
It makes sense as a separate commit more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll commit separately next time. Thanks

presto-spi/src/main/java/com/facebook/presto/spi/StandardErrorCode.java
@@ -65,6 +65,7 @@
INVALID_ANALYZE_PROPERTY(0x0000_002A, USER_ERROR),
GENERATED_BYTECODE_TOO_LARGE(0x0000_002B, USER_ERROR),
WARNING_AS_ERROR(0x0000_002C, USER_ERROR),
INVALID_ARGUMENTS(0x0000_002D, USER_ERROR),
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm we don't have martint/trino@1f40322 so it's questionable whether this makes sense in isolation or if we should use some other existing error code.

Martin's reasoning seems sound. @arhimondr @rongrong @kaikalur do you have any opinions on direction for error codes?

@v-jizhang
Copy link
Contributor Author

The Trino PR says it depends on https://github.com/trinodb/trino/pull/3718/files, but it looks to me like we already have it?
@zhenxiao tagging you also for review because you have done a lot of work in this area.

Yes, we ready have it.

@v-jizhang
Copy link
Contributor Author

The failed test is not related. Looks like a network problem on CI.

aweisberg
aweisberg approved these changes Apr 9, 2021
View reviewed changes
Copy link
Contributor

@aweisberg aweisberg left a comment

Choose a reason for hiding this comment

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

Documentation commit needs a co-author and link to the source. Can you also squash and clean up the history?

@v-jizhang @martint @mosabua
Add support for passthrough Elasticsearch queries
9a2c06e 
Cherry-pick of trinodb/trino#3735
This allows running queries over the results of a raw Elasticsearch query.
It extends the syntax of the enhanced ES table names with the following:

SELECT * FROM es.default."<index>$query:<base32-encoded ES query>"

The query is base32-encoded to avoid having to deal with escaping quotes and case
sensitivity issues in table identifiers.

The result of these query tables is a table with a single row and a single column
named "result" of type JSON.

Co-authored-by: Martin Traverso <[email protected]>
Co-authored-by: Manfred Moser <[email protected]>
@v-jizhang v-jizhang force-pushed the elastic_passthrough branch from 2dd7d35 to 9a2c06e Compare April 12, 2021 04:02
@v-jizhang
Copy link
Contributor Author

Documentation commit needs a co-author and link to the source. Can you also squash and clean up the history?

Added co-author, thanks.

@aweisberg
Copy link
Contributor

@zhenxiao do you have time to review/merge this?

zhenxiao
zhenxiao approved these changes Apr 16, 2021
View reviewed changes
Copy link
Collaborator

@zhenxiao zhenxiao 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

@zhenxiao zhenxiao merged commit 8c0b1ff into prestodb:master Apr 16, 2021
@vaishnavibatni vaishnavibatni mentioned this pull request Apr 27, 2021
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aweisberg aweisberg aweisberg approved these changes

@zhenxiao zhenxiao zhenxiao approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@v-jizhang @aweisberg @zhenxiao