prestodb / presto-python-client

Python DB-API client for Presto
Apache License 2.0
239 stars 87 forks source link

Change Presto client to support intergration with SQLAlchemy #130

Closed mlyublena closed 1 year ago

mlyublena commented 1 year ago

Change Presto client to support integration with SQLAlchemy

This is a cherry-pick of the following two commits:

[Make ClientSession thread-safe] cherry-pick of https://github.com/trinodb/trino-python-client/commit/79a4814a0f8054cf6596fc6cf25337464c6ea514 Co-authored by Michiel De Smet (mdesmet)

[Support parameterized statements in Presto Python Client] Cherry-pick of https://github.com/trinodb/trino-python-client/commit/a7438556213bcf954f8a7a5063c7823bbb177edb

Co-authored by: Harrington Joseph (harph) Testplan: pytest

collected 31 items

integration_tests/test_dbapi.py ...............                                                                                                                                                                                                           [ 48%]
tests/test_client.py ...........                                                                                                                                                                                                                          [ 83%]
tests/test_exceptions.py ...                                                                                                                                                                                                                              [ 93%]
tests/test_http.py ..                                                                                                                                                                                                                                     [100%]

======================================================================================================================= warnings summary ========================================================================================================================
tests/test_client.py::test_authentication_fail_retry
  /Users/lyublena/dev/presto-python-client/venv/lib/python3.8/site-packages/_pytest/threadexception.py:73: PytestUnhandledThreadExceptionWarning: Exception in thread Thread-4

  Traceback (most recent call last):
    File "/opt/facebook/nix/store/p4yr7ykc4mzzbsh489w9bysqcr0l0r8a-python3-3.8.9/lib/python3.8/threading.py", line 932, in _bootstrap_inner
      self.run()
    File "/opt/facebook/nix/store/p4yr7ykc4mzzbsh489w9bysqcr0l0r8a-python3-3.8.9/lib/python3.8/threading.py", line 870, in run
      self._target(*self._args, **self._kwargs)
    File "/Users/lyublena/dev/presto-python-client/venv/lib/python3.8/site-packages/httpretty/core.py", line 1133, in fill_filekind
      fk.write(utf8(item) + b'\n')
  ValueError: write to closed file

    warnings.warn(pytest.PytestUnhandledThreadExceptionWarning(msg))

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================================================================================================================ 31 passed, 1 warning in 30.46s =================================================================================================================

Above warning is independent of changes and exists on master too.

ajaygeorge commented 1 year ago

Includes the following changes:

  • Improve PEP 249 compliance
  • support for prepared statements

https://peps.python.org/pep-0249/

Can you split this into 2 commits. Also please make sure the commit message adheres to the format.

mlyublena commented 1 year ago

Includes the following changes:

  • Improve PEP 249 compliance
  • support for prepared statements

https://peps.python.org/pep-0249/

Can you split this into 2 commits. Also please make sure the commit message adheres to the format.

Done. @ajaygeorge Can you take a look?

ajaygeorge commented 1 year ago

Please make sure the cherry-picked commit has the original author as a co-author on this commit as well. See example : https://github.com/prestodb/presto/pull/19098/commits Also see Attribution guidelines.

ajaygeorge commented 1 year ago

Please fix the small typo in commit message as well. intrgration

mlyublena commented 1 year ago

Please make sure the cherry-picked commit has the original author as a co-author on this commit as well. See example : https://github.com/prestodb/presto/pull/19098/commits Also see Attribution guidelines.

Done. Please take a look

ajaygeorge commented 1 year ago

@mlyublena if you take a look at this sample PR https://github.com/prestodb/presto/pull/19098/commits you can see both the authors are listed as the authors to the commit. feilong-liu and kasiafi committed on Mar 1 Can you please fix that so that the original author is added as a committer.

mlyublena commented 1 year ago

@mlyublena if you take a look at this sample PR https://github.com/prestodb/presto/pull/19098/commits you can see both the authors are listed as the authors to the commit. feilong-liu and kasiafi committed on Mar 1 Can you please fix that so that the original author is added as a committer.

done