search

trinodb / trino-python-client

Python client for Trino
Apache License 2.0
311 stars 154 forks source link

Support setting roles in dbapi and sqlalchemy #212

Closed mdesmet closed 1 year ago

mdesmet commented 1 year ago

fixes #230

hovaesco commented 1 year ago

Please rebase on master to resolve CI failures.

mdesmet commented 1 year ago

@hashar: This would enable us to support grants in dbt-trino.

mdesmet commented 1 year ago

@hashhar: Please advise on how to get this merged. Note that we had roles already supported (within the session) and they were tested in the same way (now expanded up to http_headers) but not in integration tests as we don't have a connector that supports roles in this project.

As mentioned we need this feature in as we want to support roles security within dbt_trino.

hashhar commented 1 year ago

I'm fine with current tests for now but as we start to work more on this area we'd eventually need better integration tests - let's create an issue with current ideas and we can follow up their.

@ebyhr WDYT?

hashhar commented 1 year ago

@mdesmet Can you also update PR description with the template + propose a release note?

mdesmet commented 1 year ago

I'm fine with current tests for now but as we start to work more on this area we'd eventually need better integration tests - let's create an issue with current ideas and we can follow up their.

Created #241

hashhar commented 1 year ago

@mdesmet Can you rebase, I see some failures when running tests locally. Possibly something changed on master in the meantime.

hashhar commented 1 year ago

Seems like some changes in post-397 affect query cancellations causing "most" of the test failures. The last successful run was against 397.

mdesmet commented 1 year ago

@hashhar : Unfortunately something fishy is going on: 351 and 395 pass. However 398 does fail consistently (also locally), while 397 works fine.

I checked the release notes but didn't immediately find something. I'll keep digging.

hashhar commented 1 year ago

See https://github.com/trinodb/trino/pull/13907/files#r984659618, a "bug" has been fixed - we now don't get result sets for non-SELECT queries like it should've been all along.

mdesmet commented 1 year ago

@hashhar: I've realigned the implementation on the session_properties and your changes in #242

It would be nice to get this one in as well.