search

trinodb / trino-python-client

Python client for Trino
Apache License 2.0
309 stars 151 forks source link

get_prepared_statement_values fails if HEADER_ADDED_PREPARE is present in header but empty #262

Closed derek-pyne closed 1 year ago

derek-pyne commented 1 year ago

Expected behavior

Moving from 0.316 to 0.317 causes Trino queries to (run with pandas read_sql) to fail inside get_prepared_statement_values (https://github.com/trinodb/trino-python-client/blob/68f3d9d843e307c57b6ce728493a9837939ba60a/trino/client.py#L232) with a Value Error (ValueError: not enough values to unpack (expected 2, got 1))

This seems to be caused by this commit which checks for the HEADER_ADDED_PREPARE and parses the data in it.

However, in our case, our Trino setup is always returning this header ('X-Trino-Added-Prepare'), just with an empty string if there is no data in it. get_prepared_statement_values then fails trying to parse the data in an empty string.

One fix, would just to not only check that this header is present (which is what is currently done), but also check that there is a non-empty value in it.

Note: I have very little understanding of any of the guts here and only a surface level understanding of a Trino setup. If the feedback is 'Your Trino setup should not be returning headers with empty values' then I can get some more information about why thats the case on my side :)

Actual behavior

Value Error if X-Trino-Added-Prepare is present but empty

Steps To Reproduce

Add X-Trino-Added-Prepare to response with empty string. Unsure if this is expected behaviour (can get more information from my side if this is weird)

Log output

No response

Operating System

macOS 12.6

Trino Python client version

0.317

Trino Server version

388.007

Python version

3.9.9

Are you willing to submit PR?

hashhar commented 1 year ago

Trino Server version

388.007

Doesn't look like a real Trino version. Are you running a fork with some modifications? Asking because the header being passed with empty values is not expected AFAIK.

hashhar commented 1 year ago

BTW even if it's not expected we should still make the code more defensive either way (for example if there's some proxy sitting between Trino and the client which mangles the headers) - so please send a PR.

I do plan to improve the general mechanism by which we handle request and response headers so it'll likely get fixed then as well but I can't commit to "when" I'll be done with it.

hashhar commented 1 year ago

Fixed in https://github.com/trinodb/trino-python-client/pull/263