Repository navigation
fix: populate rowcount after async DML completion #1016
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c481be8
268a890
a70462e
9056c1f
c4ab0fd
59879fc
e0212c3
1dee683
f06979f
14466cf
6464c30
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| 6f4b6d607a61bdde61dcda2cf82bc8e80fd32cb9 | ||
| 453017f8c305e083e14b9b85183bc00920523ad5 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -966,7 +966,9 @@ def get_execution_result( | |
| # telemetry — an accepted, narrow gap limited to the case where | ||
| # result-set construction fails after a successful await. | ||
| try: | ||
| return self._make_result_set(stream, cursor, command_id) | ||
| result_set = self._make_result_set(stream, cursor, command_id) | ||
| result_set.num_modified_rows = handle.num_modified_rows | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 High — The sync path in this file already handles this safely with
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The connector release requires kernel #432 and a package minimum set to the version containing it, as stated in the PR description. That version update is pending publication of the companion binding; v1.4.0 is still the latest released kernel. The direct getter is verified against the pinned source build in the native tests and live matrix. Keeping this release prerequisite visible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Understood. The kernel floor bump has to wait until the binding that includes #432 is published, and the native/live tests build against that source. Until then, though, line 970 is still a direct There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 High — This reads On a 1.4.x wheel, the attribute lookup raises The sync path a few hundred lines up guards against this with
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kernel #432 is merged and KERNEL_REV points to its merge commit. The remaining dependency-version update requires publication of that kernel release and must land before this connector is merged or released. The source-built and native tests use that binding. Leaving this release prerequisite open. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, merging #432 and pinning There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 High — Fix: use |
||
| return result_set | ||
| except Exception as exc: | ||
| raise _wrap_kernel_exception("get_execution_result", exc) from exc | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| import re | ||
| import time | ||
| import threading | ||
| import weakref | ||
| from typing import Dict, List, Optional, Union, Any, TYPE_CHECKING | ||
| from uuid import UUID | ||
|
|
||
|
|
@@ -267,6 +268,9 @@ def __init__( | |
| raise | ||
|
|
||
| self._request_lock = threading.RLock() | ||
| self._terminal_operation_statuses: weakref.WeakKeyDictionary[ | ||
| CommandId, ttypes.TGetOperationStatusResp | ||
| ] = weakref.WeakKeyDictionary() | ||
| self._session_id_hex = None | ||
|
|
||
| @property | ||
|
|
@@ -927,10 +931,32 @@ def _results_message_to_execute_response( | |
| def get_execution_result( | ||
| self, command_id: CommandId, cursor: Cursor | ||
| ) -> "ResultSet": | ||
| """Fetch a completed async result and its latest operation metadata. | ||
|
|
||
| FetchResults does not carry ``numModifiedRows``, so reuse a terminal | ||
| status observed for the same ``CommandId`` instance by | ||
| ``get_query_state``, or refresh the status when the caller resumes | ||
| directly by command ID. | ||
| Failed or closed operations raise before result rows are fetched. | ||
| """ | ||
| thrift_handle = command_id.to_thrift_handle() | ||
| if not thrift_handle: | ||
| raise ValueError("Not a valid Thrift command ID") | ||
|
|
||
| with self._request_lock: | ||
| operation_status = self._terminal_operation_statuses.pop(command_id, None) | ||
|
aakash-saravanan-db marked this conversation as resolved.
Resolved
|
||
| if operation_status is None: | ||
|
aakash-saravanan-db marked this conversation as resolved.
Resolved
aakash-saravanan-db marked this conversation as resolved.
Resolved
aakash-saravanan-db marked this conversation as resolved.
Resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Low — When resuming directly by command ID (no cached terminal status), |
||
| operation_status = self._poll_for_status(thrift_handle) | ||
| self._check_command_not_in_error_or_closed_state( | ||
|
aakash-saravanan-db marked this conversation as resolved.
Resolved
|
||
| thrift_handle, operation_status | ||
| ) | ||
|
|
||
| num_modified_rows = ( | ||
| operation_status.numModifiedRows | ||
| if operation_status.operationState == ttypes.TOperationState.FINISHED_STATE | ||
| else None | ||
| ) | ||
|
|
||
| req = ttypes.TFetchResultsReq( | ||
| operationHandle=ttypes.TOperationHandle( | ||
| thrift_handle.operationId, | ||
|
|
@@ -981,6 +1007,7 @@ def get_execution_result( | |
| is_staging_operation=is_staging_operation, | ||
| arrow_schema_bytes=schema_bytes, | ||
| result_format=t_result_set_metadata_resp.resultFormat, | ||
| num_modified_rows=num_modified_rows, | ||
| ) | ||
|
|
||
| return ThriftResultSet( | ||
|
|
@@ -1029,6 +1056,9 @@ def get_query_state(self, command_id: CommandId) -> CommandState: | |
| state = CommandState.from_thrift_state(operation_state) | ||
| if state is None: | ||
| raise ValueError(f"Unknown command state: {operation_state}") | ||
| if state == CommandState.SUCCEEDED: | ||
| with self._request_lock: | ||
| self._terminal_operation_statuses[command_id] = poll_resp | ||
| return state | ||
|
|
||
| @staticmethod | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.