SNOW-692968: Async queries support #787
Conversation
include/snowflake/client.h
Outdated
| * | ||
| * @return sfstmt SNOWFLAKE_STMT context for async queries. | ||
| */ | ||
| SF_STMT* STDCALL snowflake_async_stmt(SF_CONNECT *sf, const char *query_id); |
There was a problem hiding this comment.
The function name sounds confusing since it seems to be an equivalent of snowflake_stmt. Maybe use the similar function name as JDBC, create_resultset or to be more specific, create_async_query_result? Still return a SF_STMT instance since libsfclient doesn't have an struct specifically for result set, but would be easier to understand the functionality of the function.
lib/client.c
Outdated
| char* queryStatus = snowflake_cJSON_GetStringValue(status); | ||
| ret = get_status_from_string(queryStatus); | ||
| } | ||
| else { |
There was a problem hiding this comment.
After seeing the logic in get_real_results I'm not quite sure whether we should set error here. Seems no data is acceptable, the error log in get_query_metadata would be the same.
lib/client.c
Outdated
| #ifdef _WIN32 | ||
| Sleep(sleep_time); |
There was a problem hiding this comment.
define this as a common function in platform.c? I think same logic being used in http_perform.c as well.
There was a problem hiding this comment.
Please add test cases to cover typical use case, checking query status before calling any fetch/metadata API. Take JDBC doc as example, https://docs.snowflake.com/en/developer-guide/jdbc/jdbc-using#examples-of-asynchronous-queries
Add usage example in PR description as well (could be reference to test case if the code is clean enough).
Calling fetch/metadata function without checking query status also needed. Please test with queries would take some time.
lib/connection.c
Outdated
| // Remove old result URL and query code if this isn't our first rodeo | ||
| SF_FREE(result_url); | ||
| memset(query_code, 0, QUERYCODE_LEN); | ||
| data = snowflake_cJSON_GetObjectItem(*json, "data"); | ||
| if (json_copy_string(&result_url, data, "getResultUrl") != | ||
| SF_JSON_ERROR_NONE) { |
There was a problem hiding this comment.
No changes? No reason to reformat.
There was a problem hiding this comment.
I think VS automatically did that accidentally when I wrapped it in an if clause. I'll revert these
lib/connection.c
Outdated
| // Error came from request up, just break | ||
| stop = SF_BOOLEAN_TRUE; | ||
| break; | ||
| GET_REQUEST_TYPE, error, SF_BOOLEAN_FALSE, |
lib/connection.c
Outdated
| SET_SNOWFLAKE_ERROR(error, SF_STATUS_ERROR_BAD_JSON, error_msg, | ||
| SF_SQLSTATE_UNABLE_TO_CONNECT); | ||
| break; | ||
| stop = SF_BOOLEAN_TRUE; |
lib/client.c
Outdated
| char* metadata_str = get_query_metadata(sfstmt); | ||
| if (metadata_str) { | ||
| cJSON* metadata = snowflake_cJSON_Parse(metadata_str); | ||
| cJSON* stats = snowflake_cJSON_GetObjectItem(metadata, "stats"); |
There was a problem hiding this comment.
We assume it's an object?
lib/client.c
Outdated
| @@ -2073,6 +2284,13 @@ SF_STATUS STDCALL _snowflake_execute_ex(SF_STMT *sfstmt, | |||
| body = create_query_json_body(sfstmt->sql_text, sfstmt->sequence_counter, | |||
| is_string_empty(sfstmt->connection->directURL) ? | |||
| NULL : sfstmt->request_id, is_describe_only); | |||
|
|
|||
| if (is_async_exec) { | |||
| snowflake_cJSON_AddBoolToObject(body, "asyncExec", SF_BOOLEAN_TRUE); | |||
There was a problem hiding this comment.
snowflake_cJSON_AddBoolToObject(body, "asyncExec", is_async_exec); ?
| while (strcmp(query_code, QUERY_IN_PROGRESS_CODE) == 0 || | ||
| strcmp(query_code, QUERY_IN_PROGRESS_ASYNC_CODE) == 0) { | ||
| sf_bool isAsyncExec = SF_BOOLEAN_FALSE; | ||
| cJSON *json_body = snowflake_cJSON_Parse(body); |
There was a problem hiding this comment.
Error handling?
…m/snowflakedb/libsnowflakeclient into SNOW-692968-async-queries-support
lib/client.c
Outdated
| return metadata; | ||
| } | ||
| SF_FREE(status_query); | ||
| log_info("No query metadata found. Query id: %s", sfstmt->sfqid); |
There was a problem hiding this comment.
log_error? Seems this would happen if there is a bug in our code or api is misused.
lib/client.c
Outdated
|
|
||
| SF_QUERY_STATUS STDCALL snowflake_get_query_status(SF_STMT *sfstmt) { | ||
| SF_QUERY_STATUS ret = SF_QUERY_STATUS_NO_DATA; | ||
| char *metadata = get_query_metadata(sfstmt); |
There was a problem hiding this comment.
We do not free the metadata
lib/client.c
Outdated
| } | ||
|
|
||
| // Get query stats | ||
| char* metadata_str = get_query_metadata(sfstmt); |
There was a problem hiding this comment.
We do not free metadata_str?
lib/client.c
Outdated
| * | ||
| * The query metadata | ||
| */ | ||
| char *get_query_metadata(SF_STMT* sfstmt) { |
There was a problem hiding this comment.
Maybe this function should return a striuct with parsed metadata instead of char array containing JSON. Consider moving parsing and validation into this function
There was a problem hiding this comment.
Especially since stringify it in this function and parse it again when it's used
lib/client.c
Outdated
| NULL, NULL, NULL, SF_BOOLEAN_FALSE)) { | ||
|
|
||
| s_resp = snowflake_cJSON_Print(resp); | ||
| log_trace("Here is JSON response:\n%s", s_resp); |
There was a problem hiding this comment.
Please be more descriptive in logs, maybe: log_trace("GET %s returned response: %s", status_query, s_resp)
Added snowflake_async_execute to execute async queries and snowflake_async_stmt to get previously run async queries using query id.