-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Support elasticsearch-sql plugin for SQL query #2582
Support elasticsearch-sql plugin for SQL query #2582
Conversation
ElasticSearch 6.3 officailly supported SQL but for earlier versions you have to install elasticsearch-sql plugin
To avoid no_such_index error when trailing semicolon is added
Thanks for submitting your first pr, @korkeatw! We'll review this as soon as we have some bandwidth. :) |
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.
Thank you for opening this pull request. Please see my comment about the implementation.
result_fields = query_dict.pop("result_fields", None) | ||
url = "{0}/{1}/_search".format(self.server_url, index_name) | ||
params = {"source": json.dumps(query_dict), "source_content_type": "application/json"} | ||
except JSONDecodeError: |
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.
Relaying on JSONDecodeError
to detect SQL queries, means that users who just have some issue with their JSON will get an unexplained error when the fallback behavior is triggered.
How about we create a dedicate query runner for ES SQL?
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 see your point. My code may change normal behavior of the query runner. Thanks @arikfr
If I understand correctly, you prefer creating new subclass for ES SQL right?
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.
BTW. "ES with SQL plugin" and "Standard ES SQL" (version >=6.3) interfaces are different. For example:
ES with SQL plugin
GET /_sql?sql=SELECT * FROM table LIMIT 5
Standard ES SQL
POST /_xpack/sql?format=txt
{
"query": "SELECT * FROM table LIMIT 5"
}
If we implement query runner for ES SQL plugin separately (separate into the new data source), it will be the problem because user may use both normal JSON query and SQL.
Any suggestions?
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.
If I understand correctly, you prefer creating new subclass for ES SQL right?
Yes.
As for the second question, is there a simple way to check what version of ES we're querying? Or maybe, just add a checkbox configuration option to set whether we're using the plugin or not?
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.
Good idea! I like the concept that we're having the checkbox to select between JSON query and SQL query.
Will take care of this.
@@ -43,6 +45,13 @@ | |||
} | |||
|
|||
|
|||
def get_index_from_sql(sql): | |||
matches = re.search(r"from ([\w\d]+)", sql, flags=re.IGNORECASE) |
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.
It does not match an index of the type metadata_0_2018-08-29_18:40:21
, I would add ([\w\d\-:]+)
Thank you for your effort on this and apologies that it wasn't merged eventually. Due to the state of the current implementation and the inability to properly test new changes, we decided to commit to making a new Elasticsearch query runner (#4293) and will address this and the other issues there. |
ElasticSearch version 6.3 and later supported SQL statement for querying the data but not for the earlier versions. To use the SQL statement, you have to install elasticsearch-sql plugin.
The PR is to support SQL statement if user use ElasticSearch as data source and the plugin is already installed.