-
Notifications
You must be signed in to change notification settings - Fork 51
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
feat: support opendistro #45
Conversation
Codecov Report
@@ Coverage Diff @@
## master #45 +/- ##
==========================================
+ Coverage 89.41% 93.53% +4.12%
==========================================
Files 15 15
Lines 633 681 +48
==========================================
+ Hits 566 637 +71
+ Misses 67 44 -23
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
A few comments
@@ -120,19 +177,19 @@ def __init__(self, url, es, **kwargs): | |||
# this is set to an iterator after a successfull query | |||
self._results = None | |||
|
|||
@property | |||
@property # type: ignore |
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.
what was the complaint here?
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.
es/baseapi.py:189: error: Decorated property not supported
python/mypy#1362 with a bunch of comments from the creator himself ;)
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.
Oh wow, quite the can of worms
Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
…dbapi into feat/opendistro
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 don't know ElasticSearch well enough to verify that the implementation is correct, but I don't see any red flags.
Thank you @villebro and @willbarrett for taking the time to review |
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.
Just a few pending nits, LGTM
@@ -120,19 +177,19 @@ def __init__(self, url, es, **kwargs): | |||
# this is set to an iterator after a successfull query | |||
self._results = None | |||
|
|||
@property | |||
@property # type: ignore |
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.
Oh wow, quite the can of worms
es/basesqlalchemy.py
Outdated
return "DATETIME" | ||
|
||
def visit_TIME(self, type_, **kwargs): | ||
def visit_TIME(self, type_, **kwargs: Dict[str, Any]) -> str: |
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.
this one still pending
es/basesqlalchemy.py
Outdated
raise exceptions.NotSupportedError("Type TIME is not supported") | ||
|
||
def visit_BINARY(self, type_, **kwargs): | ||
def visit_BINARY(self, type_, **kwargs: Dict[str, Any]) -> str: |
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.
this one still pending
es/basesqlalchemy.py
Outdated
raise exceptions.NotSupportedError("Type BINARY is not supported") | ||
|
||
def visit_VARBINARY(self, type_, **kwargs): | ||
def visit_VARBINARY(self, type_, **kwargs: Dict[str, Any]) -> str: |
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.
..and this
Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
…dbapi into feat/opendistro
Provides support for Opendistro Elasticsearch https://opendistro.github.io/for-elasticsearch-docs/
Still pending some issues:
_mapping
endpointSELECT 1
is caught and then issues an Elasticsearch.ping()Also adds
mypy
checkCloses: #31 #5 #7 #6