Skip to content

Use str type for driver and name in HiveDialect #450

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
May 9, 2023

Conversation

Usiel
Copy link
Contributor

@Usiel Usiel commented Apr 28, 2023

PyHive's HiveDialect usage of bytes for the name and driver fields is not the norm is causing issues upstream: apache/superset#22316
Other dialects within PyHive already use strings. While SQLAlchemy does not strictly require a string, all the stock dialects return a string, so I figure it is heavily implied.

I think the risk of breaking something upstream with this change is low (but it is there ofc). I figure in most cases we just make someone's str(dialect.driver) expression redundant. To be extra safe we could just go for a major version bump.

Examples for some of the other stock sqlalchemy dialects (name and driver fields using str): https://github.com/zzzeek/sqlalchemy/blob/main/lib/sqlalchemy/dialects/sqlite/pysqlite.py#L501 https://github.com/zzzeek/sqlalchemy/blob/main/lib/sqlalchemy/dialects/sqlite/base.py#L1891 https://github.com/zzzeek/sqlalchemy/blob/main/lib/sqlalchemy/dialects/mysql/base.py#L2383 https://github.com/zzzeek/sqlalchemy/blob/main/lib/sqlalchemy/dialects/mysql/mysqldb.py#L113 https://github.com/zzzeek/sqlalchemy/blob/main/lib/sqlalchemy/dialects/mysql/pymysql.py#L59

PS: I have submitted the CLA.

PyHive's HiveDialect usage of bytes for the name and driver fields is not the norm is causing issues upstream: apache/superset#22316
Even other dialects within PyHive use strings. SQLAlchemy does not strictly require a string, but all the stock dialects return a string, so I figure it is heavily implied.

I think the risk of breaking something upstream with this change is low (but it is there ofc). I figure in most cases we just make someone's `str(dialect.driver)` expression redundant.

Examples for some of the other stock sqlalchemy dialects (name and driver fields using str):
https://github.com/zzzeek/sqlalchemy/blob/main/lib/sqlalchemy/dialects/sqlite/pysqlite.py#L501
https://github.com/zzzeek/sqlalchemy/blob/main/lib/sqlalchemy/dialects/sqlite/base.py#L1891
https://github.com/zzzeek/sqlalchemy/blob/main/lib/sqlalchemy/dialects/mysql/base.py#L2383
https://github.com/zzzeek/sqlalchemy/blob/main/lib/sqlalchemy/dialects/mysql/mysqldb.py#L113
https://github.com/zzzeek/sqlalchemy/blob/main/lib/sqlalchemy/dialects/mysql/pymysql.py#L59
@CLAassistant
Copy link

CLAassistant commented Apr 28, 2023

CLA assistant check
All committers have signed the CLA.

@bkyryliuk
Copy link
Collaborator

@Usiel can you confirm that apache/superset#22316 is addressed with this PR ?

@Usiel
Copy link
Contributor Author

Usiel commented Apr 30, 2023

@Usiel can you confirm that apache/superset#22316 is addressed with this PR ?

Yes, this fixes the issue on Superset (I also made a comment on that issue).

@bkyryliuk bkyryliuk merged commit 1f99552 into dropbox:master May 9, 2023
@kashifjavedaddo
Copy link

@Usiel so how do i get this fixed on my installation.

do I need to upgrade my existing superset 2.1.0 installation ?

Please update with exact steps to get this done.

@Usiel
Copy link
Contributor Author

Usiel commented May 10, 2023

Thanks for the merge @bkyryliuk! Will there be a release for this on PyPI?
Can confirm that by installing the current master with pip install git+https://github.com/dropbox/PyHive.git I can fix the linked Superset issue.

@bkyryliuk bkyryliuk mentioned this pull request May 17, 2023
betodealmeida added a commit to preset-io/PyHive that referenced this pull request Aug 8, 2024
* feat: add HTTP and HTTPS to hive (dropbox#385)

* feat: add https protocol

* support HTTP

* fix: make hive https py2 compat (dropbox#389)

* fix: make hive https py2 compat

* fix lint

* Update README.rst (dropbox#423)

* chore: rename Trino entry point (dropbox#428)

* Support for Presto decimals (dropbox#430)

* Support for Presto decimals

* lower

* Use str type for driver and name in HiveDialect (dropbox#450)

PyHive's HiveDialect usage of bytes for the name and driver fields is not the norm is causing issues upstream: apache/superset#22316
Even other dialects within PyHive use strings. SQLAlchemy does not strictly require a string, but all the stock dialects return a string, so I figure it is heavily implied.

I think the risk of breaking something upstream with this change is low (but it is there ofc). I figure in most cases we just make someone's `str(dialect.driver)` expression redundant.

Examples for some of the other stock sqlalchemy dialects (name and driver fields using str):
https://github.com/zzzeek/sqlalchemy/blob/main/lib/sqlalchemy/dialects/sqlite/pysqlite.py#L501
https://github.com/zzzeek/sqlalchemy/blob/main/lib/sqlalchemy/dialects/sqlite/base.py#L1891
https://github.com/zzzeek/sqlalchemy/blob/main/lib/sqlalchemy/dialects/mysql/base.py#L2383
https://github.com/zzzeek/sqlalchemy/blob/main/lib/sqlalchemy/dialects/mysql/mysqldb.py#L113
https://github.com/zzzeek/sqlalchemy/blob/main/lib/sqlalchemy/dialects/mysql/pymysql.py#L59

* Correcting Iterable import for python 3.10 (dropbox#451)

* changing drivers to support hive, presto and trino with sqlalchemy>=2.0 (dropbox#448)

* Revert "changing drivers to support hive, presto and trino with sqlalchemy>=2.0 (dropbox#448)" (dropbox#452)

This reverts commit b0206d3.

* Update __init__.py (dropbox#453)

dropbox@1c1da8b

dropbox@1f99552

* use pure-sasl with python 3.11 (dropbox#454)

* minimal changes for sqlalchemy 2.0 support (dropbox#457)

* update readme to reflect recent changes (dropbox#459)

* Update README.rst (dropbox#475)

* Update README.rst (dropbox#476)

* feat: JWT support

* Add CI to build package

---------

Co-authored-by: Daniel Vaz Gaspar <danielvazgaspar@gmail.com>
Co-authored-by: Bogdan <b.kyryliuk@gmail.com>
Co-authored-by: serenajiang <serena.jiang@airbnb.com>
Co-authored-by: Usiel Riedl <usiel.riedl@gmail.com>
Co-authored-by: Multazim Deshmukh <57723564+mdeshmu@users.noreply.github.com>
Co-authored-by: nicholas-miles <nicholas.miles6@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants