-
Notifications
You must be signed in to change notification settings - Fork 3
feat: add temporary result storage and smoke test options #39
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?
Conversation
i'm getting some errors running this sql statement: it fails if i try to use either |
so iiuc this change doesn't provide a way to get access to the results URI from the sdk, correct? but you can opt into behind-the-scenes temp storage. which you would do because it increases the limit on the number of results you can get back? and/or you get more efficient cursoring through the results because sql-session can pull them from its s3 cache based on the execution id? |
another interesting behavior i discovered, when passing boolean values in the execute sql web socket message, they have to be stringified. i.e. this is valid:
but this is not (note no quotes around
|
63fdf92
to
d31f4e7
Compare
@@ -134,6 +137,9 @@ def __listen(self) -> None: | |||
# On a state_updated event telling us the query succeeded, | |||
# ask for results. | |||
if kind == EventKind.STATE_UPDATED: | |||
logging.info( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to keep that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and no. Since result_uri and size is currently part of the state updated event (and not the retrieve result) we might want to have a way to surface that info programatically. Right now it's an easy way to just log dump it, but honestly it's a poor way to surface it.
We should talk about really making it part of a request result call even if it's another extra round or have a nicer way to handle it from the client side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. My recommendation is to extend the interface of Connection
beyond what the DB-API interface calls for, as that part is specifically for "DB driver"-like work where you get query results directly through the Cursor
from the cursor()
method.
Maybe we provide another, different type of handler:
def cursor(self) -> Cursor:
return Cursor(self.__execute_sql, self.__cancel_query)
def stored(self, format: ResultFormat, single: bool, presigned: bool) -> StoredResult:
return StoredResult(self.__execute_sql, self.__cancel_query, format, single, presigned)
Which exposes:
class StoredResult:
def __init__(self, exec_fn, cancel_fn, format: ResultFormat, single: bool, presigned: bool):
# ...
def execute(self, operation: str, parameters: dict[str, Any] = None):
# ... call exec_fn with the correct store{} parameters
def get_result_url(self) -> str:
# ...
Uh oh!
There was an error while loading. Please reload this page.