Skip to content
This repository was archived by the owner on May 17, 2024. It is now read-only.

Conversation

@danthelion
Copy link
Contributor

Mainly based on the Presto integration to serve as a starting point for future development.

@danthelion
Copy link
Contributor Author

danthelion commented Jul 12, 2022

@erezsh Fixed the Poetry dependencies which caused the CI job to fail.

@erezsh
Copy link
Contributor

erezsh commented Jul 12, 2022

Hi @danthelion , thanks for making a contribution!

The next step is be to add Trino to the DATABASE_TYPES dict in https://github.com/datafold/data-diff/blob/master/tests/test_database_types.py#L186, and run the tests against it. Make sure it's activated along with at least one other database, for example postgresql or mysql.

A few points:

  • Make sure you add as many distinct types as possible. However, no need to cover all the possible variations of them.

  • It might take a few minutes to run. If that becomes an issue, you can run the tests with unittest-parallel.

We're going to write a comprehensive guide for contributors real soon. But meanwhile, if you have any questions at all, I'll be happy to answer them.

@erezsh
Copy link
Contributor

erezsh commented Jul 12, 2022

Looks like the port is already used. Perhaps by Presto?

@erezsh
Copy link
Contributor

erezsh commented Jul 12, 2022

Just one note about the PR, please make trino optional, like the rest. i.e.

trino = { version = "^0.314.0", optional = true }
@danthelion
Copy link
Contributor Author

danthelion commented Jul 12, 2022

@erezsh Mapped the port to 8081 instead, Presto was indeed conflicting. Also added the Trino lib as optional and rebased to master.

@erezsh
Copy link
Contributor

erezsh commented Jul 12, 2022

Hi @danthelion ,

I opened a new PR to fix a few minor things and to make sure the tests run in the CI. (#162 )

I'm getting this error from Trino:

trino.exceptions.HttpError: error 401: b'Basic authentication or X-Presto-User must be sent' 

(See here: https://github.com/datafold/data-diff/runs/7309529815?check_suite_focus=true)

I don't have this issue when I run it locally. Any idea what's the problem and how to fix it?

@erezsh
Copy link
Contributor

erezsh commented Jul 12, 2022

P.S. I'm using TEST_TRINO_CONN_STRING = "trino://presto@127.0.0.1/postgresql/public"

@danthelion
Copy link
Contributor Author

@erezsh thanks for looking into this! I added the fixes to this branch. Could you try running the CI with TEST_TRINO_CONN_STRING="trino://postgres@127.0.0.1:8081/postgresql/public" (The user and port are different) - I think the Trino client is trying to talk to the Presto service this way

@erezsh
Copy link
Contributor

erezsh commented Jul 13, 2022

Keep in mind these tests are still running with (WARNING) root -- Connection to <class 'data_diff.databases.trino.Trino'> not configured. I'll update and check!

@erezsh
Copy link
Contributor

erezsh commented Jul 13, 2022

Now I'm getting trino.exceptions.TrinoExternalError: TrinoExternalError(type=EXTERNAL, name=JDBC_ERROR, message="The connection attempt failed.", query_id=20220713_082053_03808_dtucz)

Does it mean it couldn't find the server?

@erezsh
Copy link
Contributor

erezsh commented Jul 13, 2022

But it's happening here:

 File "/home/runner/work/data-diff/data-diff/data_diff/databases/trino.py", line 58, in _query return c.fetchone() 
@danthelion
Copy link
Contributor Author

Weird, in the py3.8 test suite the Presto connection fails. Could you check the connection strings for both?

erezsh added a commit that referenced this pull request Jul 13, 2022
@erezsh erezsh merged commit b839ae6 into datafold:master Jul 13, 2022
@erezsh
Copy link
Contributor

erezsh commented Jul 13, 2022

I gave up getting this into the CI. I tested it on my machine and that's good enough for now.

If you want to help us get Trino tests into the CI, please open a new PR. I think that the problem is running both the Presto and Trino servers at the same time; they step on each others' toes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

2 participants