-
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
fix elasticsearch get_schema error when mapping contains no 'properties' key #3692
Conversation
This fix just fixes |
…, it will raise error due to lack of key 'properties'
Thanks this helped us. |
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 job~
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 couldn't get the elastic search.py to run on the docker image, the code below did work for me there
if not 'properties' in doc:
return result
@axelsean That's odd. Was that because |
I *think* so, but I'm a real Python novice (written 3 lines in my life !)
Dave
…On Wed, May 8, 2019 at 3:18 AM Xiao Wang ***@***.***> wrote:
@axelsean <https://github.com/axelsean> That's odd. Was that because doc
variable is not a dict but a list in your scenario?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3692 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEO2TPH6F5PK3AXWKSSEHS3PUKEHDANCNFSM4HEYHE5A>
.
|
Could you debug it by printing |
redash/query_runner/elasticsearch.py
Outdated
properties = doc.get('properties') | ||
if not properties: | ||
return result | ||
for field, description in properties.items(): |
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.
Hi! What about making it a bit shorter and just writing this?
for field, description in doc.get('properties', {}).items():
Thus, there will be no need for previous new lines
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.
Making python code short is a style thing, I am OK with that.
Besides, as latter comments pointed out, the doc
may not be a dict. So maybe we still need code below to avoid the "not dict" issue.
if not isinstance(doc, dict):
return result
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.
Or
if not isinstance(doc, dict) or 'properties' not in doc:
return result
and keep original code intact.
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. |
What type of PR is this? (check all applicable)
Description
fix elasticsearch get_schema error when index has no 'properties' key.
This bug could be reproduced by trying to use a elasticsearch instance with
.kibana
index in it.The version of Elasticsearch and Kibana I use is
5.6.3
. Sample.kibana
mappings or any other index which contains_default_
in mappings.kibana_mappings.txt
I also added exception traceback print for future debugging purposes.
Related Tickets & Documents
#3570
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
no changes