-
Notifications
You must be signed in to change notification settings - Fork 10
Added support for Dremio. #41
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
base: master
Are you sure you want to change the base?
Conversation
* Added Dremio docker image to the CI integration testing. * The `vertica` docker image is missing from dockerhub and is returning 404, so the vertica test is commented out. * Added python version markers to pyarrow, pandas and numpy as they each drop support for Python 3.8 so for working with Python 3.8 we need to install the last compatible version. * The cursor is the implementation in `sqlalchemy-dremio`. The current release of `sqlalchemy-dremio` (3.0.4) does not support datetime[ms]. The fix is on the master branch of `sqlalchemy-dremio` but the master branch dependency versions are not compatible with Python 3.8. To have the CI tests pass with Python 3.8, we use a fork of `sqlalchemy-dremio` that contains the fix but has compatible dependencies.
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.
Thanks for the PR!
Overall looks acceptable.
|
||
return super().parse_type(table_path, col_name, type_repr, datetime_precision, numeric_precision, numeric_scale) | ||
|
||
def set_timezone_to_utc(self) -> 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.
If Dremio works in UTC by default, shouldn't this method just pass without failing?
'queries instead.' | ||
) | ||
|
||
def type_repr(self, t) -> 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 method seems unnecessary
) -> ColType: | ||
from math import isnan | ||
if datetime_precision is not None and not isinstance(datetime_precision, int): | ||
if isnan(datetime_precision): |
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 should probably be a function (e.g. nan_to_none()
)
numeric_precision: int = None, | ||
numeric_scale: int = None, | ||
) -> ColType: | ||
from math import isnan |
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.
imports always at module level (unless there's a good reason not to)
(Just a few changes / responses necessary) |
vertica
docker image is missing from dockerhub and is returning 404, so the vertica test is commented out.sqlalchemy-dremio
. The current release ofsqlalchemy-dremio
(3.0.4) does not support datetime[ms]. The fix is on the master branch ofsqlalchemy-dremio
but the master branch dependency versions are not compatible with Python 3.8. To have the CI tests pass with Python 3.8, we use a fork ofsqlalchemy-dremio
that contains the fix but has compatible dependencies. sqlalchemy_dremio/issues/58