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.

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

Merged
merged 30 commits into from
Jan 21, 2021
Merged

feat: support opendistro #45

merged 30 commits into from
Jan 21, 2021

Conversation

dpgaspar
Copy link
Member

@dpgaspar dpgaspar commented Jan 18, 2021

Provides support for Opendistro Elasticsearch https://opendistro.github.io/for-elasticsearch-docs/

Still pending some issues:

Also adds mypy check

Closes: #31 #5 #7 #6

@codecov-io
Copy link

codecov-io commented Jan 18, 2021

Codecov Report

Merging #45 (5434dac) into master (698006e) will increase coverage by 4.12%.
The diff coverage is 98.11%.

Impacted file tree graph

@@            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     
Flag Coverage Δ
python 93.53% <98.11%> (+4.12%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
es/basesqlalchemy.py 85.03% <88.23%> (-2.37%) ⬇️
es/baseapi.py 90.97% <100.00%> (+1.19%) ⬆️
es/elastic/api.py 86.20% <100.00%> (+0.87%) ⬆️
es/elastic/sqlalchemy.py 100.00% <100.00%> (ø)
es/opendistro/api.py 100.00% <100.00%> (+100.00%) ⬆️
es/opendistro/sqlalchemy.py 100.00% <100.00%> (+100.00%) ⬆️
es/tests/fixtures/fixtures.py 100.00% <100.00%> (ø)
es/tests/test_dbapi.py 100.00% <100.00%> (ø)
es/tests/test_sqlalchemy.py 100.00% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 698006e...5434dac. Read the comment docs.

@dpgaspar dpgaspar marked this pull request as ready for review January 19, 2021 12:17
Copy link
Contributor

@villebro villebro left a 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
Copy link
Contributor

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?

Copy link
Member Author

@dpgaspar dpgaspar Jan 20, 2021

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 ;)

Copy link
Contributor

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 Show resolved Hide resolved
es/basesqlalchemy.py Outdated Show resolved Hide resolved
es/basesqlalchemy.py Outdated Show resolved Hide resolved
es/basesqlalchemy.py Outdated Show resolved Hide resolved
es/basesqlalchemy.py Outdated Show resolved Hide resolved
es/basesqlalchemy.py Outdated Show resolved Hide resolved
es/basesqlalchemy.py Outdated Show resolved Hide resolved
es/baseapi.py Show resolved Hide resolved
es/opendistro/api.py Outdated Show resolved Hide resolved
dpgaspar and others added 10 commits January 20, 2021 09:36
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>
Copy link

@willbarrett willbarrett left a 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.

@dpgaspar
Copy link
Member Author

Thank you @villebro and @willbarrett for taking the time to review

Copy link
Contributor

@villebro villebro left a 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
Copy link
Contributor

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 Show resolved Hide resolved
return "DATETIME"

def visit_TIME(self, type_, **kwargs):
def visit_TIME(self, type_, **kwargs: Dict[str, Any]) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

this one still pending

raise exceptions.NotSupportedError("Type TIME is not supported")

def visit_BINARY(self, type_, **kwargs):
def visit_BINARY(self, type_, **kwargs: Dict[str, Any]) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

this one still pending

raise exceptions.NotSupportedError("Type BINARY is not supported")

def visit_VARBINARY(self, type_, **kwargs):
def visit_VARBINARY(self, type_, **kwargs: Dict[str, Any]) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

..and this

@dpgaspar dpgaspar merged commit f0b013e into master Jan 21, 2021
@dpgaspar dpgaspar deleted the feat/opendistro branch January 21, 2021 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Opendistro not working with Superset
4 participants