[API-900] Wait for the initial response in the SQL queries #480
Conversation
mdumandag
force-pushed
the
sql-wait-first-response
branch
from
5a7ed36 to
8087d36
Compare
Codecov Report
@@ Coverage Diff @@
## master #480 +/- ##
==========================================
- Coverage 95.23% 94.95% -0.29%
==========================================
Files 346 346
Lines 17734 17705 -29
==========================================
- Hits 16889 16811 -78
- Misses 845 894 +49
Continue to review full report at Codecov.
|
Up until now, we were returning the `SqlResult` immediately after initiating the query to the server, without waiting for the first response. The server might not be able to handle some consequences of such an API. To align the behavior with the Java implementation and make it more intuitive, we now wait for the initial response in the `execute` and `execute_statement` methods. With that change, the return types are changed from `SqlResult` to `Future[SqlResult]`. Also, I got rid of the Futures in the return types of the methods over the `SqlResult` except for the `close`, as the `close` might in fact go to the server. With this change, I cleaned up the implementation so that it reflects the new behavior better.
mdumandag
force-pushed
the
sql-wait-first-response
branch
from
0708c93 to
efec85e
Compare
mdumandag
changed the title
| with client.sql.execute(mapping_query) as result: | ||
| # wait until query completes | ||
| result.update_count().result() | ||
| client.sql.execute(mapping_query).result(): |
There was a problem hiding this comment.
| client.sql.execute(mapping_query).result(): | |
| client.sql.execute(mapping_query).result() |
| *params: Query parameters that will be passed to | ||
| :func:`SqlStatement.add_parameter`. | ||
| Returns: |
There was a problem hiding this comment.
Looks like in docstring, the code in "Usage" section needs to be updated too
| :func:`update_count`. :: | ||
| update_count = client.sql.execute("SELECT ...").update_count().result() | ||
| update_count = client.sql.execute("UPDATE ...").result().update_count() |
There was a problem hiding this comment.
Do we support UPDATE queries?
There was a problem hiding this comment.
I don't know but I copied this from the Java side. I assume they did it like this because currently, we don't have any queries that return update count
hazelcast/sql.py
Outdated
| # There is nothing more we can get from the server. | ||
| self._closed = True | ||
| return ( | ||
| execute_response.row_page is None # Just an update count |
There was a problem hiding this comment.
can we check row_metadata to be null here? Java does that also we checked row_metadata in other places to check if result is a rowset or not.
There was a problem hiding this comment.
This is the same and more inline with the check below
There was a problem hiding this comment.
After a second look, the row page might be null, let me change that
Up until now, we were returning the
SqlResultimmediately afterinitiating the query to the server, without waiting for the first
response.
The server might not be able to handle some consequences of such
an API. To align the behavior with the Java implementation and
make it more intuitive, we now wait for the initial response
in the
executeandexecute_statementmethods.With that change, the return types are changed from
SqlResulttoFuture[SqlResult]. Also, I got rid of the Futures in the return typesof the methods over the
SqlResultexcept for theclose, as theclosemight in fact go to the server.With this change, I cleaned up the implementation so that it reflects
the new behavior better.
Also, we now return a rejected Future immediately if the first execute
the request fails.