`execute` does not kick off query on Trino Server

[benrifkind]

Expected behavior

The call to client.execute does not start the query on the Trino cluster.

Actual behavior

The query only starts on the Trino cluster once client.fetch is called.

Steps To Reproduce

Not sure if this is an issue or just expected behavior but I noticed that the query is not actually run by the Trino cluster until data is requested by the client.

So this code will not actually run the query

from trino.dbapi import connect

conn = connect(
    host="<host>",
    port=<port>,
    user="<username>",
    catalog="<catalog>",
    schema="<schema>",
)
cur = conn.cursor()
cur.execute("CREATE TABLE hive.<schema>.<name> AS SELECT 'a' AS a")

Nothing happens on the Trino server at this point.

The query only runs when we call fetch. This line is what kicks off the query.

cur.fetch()

Basically it seems like the initial post request does not actual kick off the query. This seems unintuitive to me. Since I would have thought that execute would actually run the query. As an example if I am trying to insert data into a table, I don't feel the need to call fetch because there is no data to fetch. I just want the query to run and I want to be able to wait until it is successful. I guess I'm wondering what the reasoning is behind the current behavior. I believe the behavior of PyHive was to actually start the query when execute was called. Thanks!

Log output

No response

Operating System

macOS Monterey

Trino Python client version

0.309.0

Trino Server version

388

Python version

3.8.6

Are you willing to submit PR?

• [X] Yes I am willing to submit a PR!

[hashhar]

It's expected behaviour at this point since the Trino REST API won't send/execute query until the nextUri is polled from the response (which happens by calling fetch).

A possible solution to this is to use thread pool to keep calling fetch on the resource and populate a buffer - then the Cursor's fetch methods can operate on the buffer instead of actual results.

That has the problem that even for clients who only want to read few rows at a time the buffer will still occupy memory and depending on implementation lead into OOM issues with large result sets.

[benrifkind]

Appreciate the quick response and clarification! I more or less implemented your suggested solution. Agree that it's not ideal but it works well enough for our use case. Thanks again. Feel free to close.

[mdesmet]

I actually agree with @benrifkind. This is counter-intuitive and seems definitely not a standard behaviour of other dbapi implementations.

The Trino REST API should be abstracted as much as possible through a standard and intuitive dbapi implementation. dbapi protocol is not the same as Trino protocol. It is true that dbapi doesn't prescribe the return value of execute, however it makes sense to follow the other dbms so that we stay compatible with abstraction frameworks like sqlalchemy.

Note that this behavior also causes the flaky sqlalchemy tests. sqlalchemy expects execute to block until the DML operation completes and only expects result sets to be returned through the RETURNING clause. See sqlalchemy source code.

UPDATE parts 
         SET part_name = UPPER (part_name) 
       WHERE part_name LIKE 'K%' 
   RETURNING part_number 
        INTO l_num; 

Some other dbapi examples:

sqlite

cur.execute("""
    INSERT INTO movie VALUES
        ('Monty Python and the Holy Grail', 1975, 8.2),
        ('And Now for Something Completely Different', 1971, 7.5)
""")
con.commit()

No need to fetch the result of a DML query.

psycopg

# Execute a command: this creates a new table
>>> cur.execute("CREATE TABLE test (id serial PRIMARY KEY, num integer, data varchar);")

# Pass data to fill a query placeholders and let Psycopg perform
# the correct conversion (no more SQL injections!)
>>> cur.execute("INSERT INTO test (num, data) VALUES (%s, %s)",
...      (100, "abc'def"))

Again no need to call fetch* functions

mysql

add_employee = ("INSERT INTO employees "
               "(first_name, last_name, hire_date, gender, birth_date) "
               "VALUES (%s, %s, %s, %s, %s)")
add_salary = ("INSERT INTO salaries "
              "(emp_no, salary, from_date, to_date) "
              "VALUES (%(emp_no)s, %(salary)s, %(from_date)s, %(to_date)s)")

data_employee = ('Geert', 'Vanderkelen', tomorrow, 'M', date(1977, 6, 14))

# Insert new employee
cursor.execute(add_employee, data_employee)
emp_no = cursor.lastrowid

Again no need for fetch*

cc @hovaesco

[hashhar]

I do mean that we should change the behaviour (and that's why didn't close this issue yet) to hide the protocol details from user but it involves some decisions and more work than you point out here:

• who buffers results?
• how much results to buffer?
• should the buffer be populated on calls to fetch? Or should we fetch entire results via a background thread.

Note that it also requires change in the engine to not return result sets in some cases.

[mdesmet]

I think all this decisions are not really relevant to the issue but rather separate improvements.

The double buffer introduced in #220 has nothing to do with what is done on execute() vs fetch*().

The proposed fix for this issue is not different from what was there before in a sense that we just do a fetchone() on execute() instead of relying on the user to perform the fetch.

who buffers results?

This is two layered: results are buffered on the server until the query abandonment limit kicks in.

In the client #220 introduces a double buffer similar to the JDBC implementation. It is up to the user to keep calling fetch or cancel the query.

how much results to buffer?

How many rows that reside in the buffer depends on first what is being returned by the Trino in one call to the REST api and second on if we want to split result retrieval from serialization.

On the first point I don't think this is part of this change. Currently Trino doesn't support setting fetchsize.

On the second point see the next question.

should the buffer be populated on calls to fetch? Or should we fetch entire results via a background thread.

I think background thread(s) is probably mostly useful to parallelise the results retrieval in the fetchall() case. I don't think it matters for this issue. The proposed fix just blocks execute() until either at least one row is returned (dml or scalar queries) or the query is finished (no resultset). Once the first result is in we'll know what the client is interested in, either fetchone() and an eventual cancel(), or fetchall(), where we could do result retrieval and serialization through a background thread(s) and pass them on the foreground thread (similar like how Snowflake does it).