SNOW-692968: Async queries support #787

sfc-gh-ext-simba-nl · 2024-11-28T04:12:25Z

Added snowflake_async_execute to execute async queries and snowflake_async_stmt to get previously run async queries using query id.

sfc-gh-ext-simba-hx · 2024-12-02T20:08:10Z

include/snowflake/client.h

+ *
+ * @return sfstmt SNOWFLAKE_STMT context for async queries.
+ */
+SF_STMT* STDCALL snowflake_async_stmt(SF_CONNECT *sf, const char *query_id);


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

…ng results to when user wants to actually get results

sfc-gh-ext-simba-hx · 2024-12-03T18:05:42Z

lib/client.c

      char* queryStatus = snowflake_cJSON_GetStringValue(status);
      ret = get_status_from_string(queryStatus);
    }
+    else {


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.

sfc-gh-ext-simba-hx · 2024-12-03T18:07:51Z

lib/client.c

 #ifdef _WIN32
-  Sleep(sleep_time);


define this as a common function in platform.c? I think same logic being used in http_perform.c as well.

sfc-gh-ext-simba-hx · 2024-12-03T18:22:28Z

tests/test_async.c

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.

sfc-gh-jszczerbinski

Please remove unnecessary formatting changes. It's hard to review with so much (unnecessary?) changes.

sfc-gh-jszczerbinski · 2024-12-12T15:48:44Z

lib/connection.c

            // 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) {


No changes? No reason to reformat.

I think VS automatically did that accidentally when I wrapped it in an if clause. I'll revert these

sfc-gh-jszczerbinski · 2024-12-12T15:49:52Z

lib/connection.c

-                // Error came from request up, just break
-                stop = SF_BOOLEAN_TRUE;
-                break;
+              GET_REQUEST_TYPE, error, SF_BOOLEAN_FALSE,


sfc-gh-jszczerbinski · 2024-12-12T15:50:01Z

lib/connection.c

-                SET_SNOWFLAKE_ERROR(error, SF_STATUS_ERROR_BAD_JSON, error_msg,
-                                    SF_SQLSTATE_UNABLE_TO_CONNECT);
-                break;
+              stop = SF_BOOLEAN_TRUE;


lib/client.c

sfc-gh-jszczerbinski · 2024-12-12T15:55:10Z

lib/client.c

+  char* metadata_str = get_query_metadata(sfstmt);
+  if (metadata_str) {
+    cJSON* metadata = snowflake_cJSON_Parse(metadata_str);
+    cJSON* stats = snowflake_cJSON_GetObjectItem(metadata, "stats");


We assume it's an object?

sfc-gh-jszczerbinski · 2024-12-12T15:56:29Z

lib/client.c

@@ -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);


snowflake_cJSON_AddBoolToObject(body, "asyncExec", is_async_exec); ?

sfc-gh-jszczerbinski · 2024-12-12T16:03:47Z

lib/connection.c

-        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);


Error handling?

…m/snowflakedb/libsnowflakeclient into SNOW-692968-async-queries-support

tests/test_async.c

include/snowflake/client.h

include/snowflake/platform.h

lib/client.c

tests/test_async.c

sfc-gh-jszczerbinski

Looks good. Still needs some work.

sfc-gh-jszczerbinski · 2025-01-16T17:07:34Z

lib/client.c

+    return metadata;
+  }
+  SF_FREE(status_query);
+  log_info("No query metadata found. Query id: %s", sfstmt->sfqid);


log_error? Seems this would happen if there is a bug in our code or api is misused.

sfc-gh-jszczerbinski · 2025-01-16T17:09:12Z

lib/client.c

+
+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);


We do not free the metadata

sfc-gh-jszczerbinski · 2025-01-16T17:13:41Z

lib/client.c

+  }
+
+  // Get query stats
+  char* metadata_str = get_query_metadata(sfstmt);


We do not free metadata_str?

sfc-gh-jszczerbinski · 2025-01-16T17:17:27Z

lib/client.c

+ * 
+ * The query metadata
+ */
+char *get_query_metadata(SF_STMT* sfstmt) {


Maybe this function should return a striuct with parsed metadata instead of char array containing JSON. Consider moving parsing and validation into this function

Especially since stringify it in this function and parse it again when it's used

sfc-gh-jszczerbinski · 2025-01-16T17:21:58Z

lib/client.c

+    NULL, NULL, NULL, SF_BOOLEAN_FALSE)) {
+
+    s_resp = snowflake_cJSON_Print(resp);
+    log_trace("Here is JSON response:\n%s", s_resp);


Please be more descriptive in logs, maybe: log_trace("GET %s returned response: %s", status_query, s_resp)

codecov · 2025-01-30T03:50:42Z

Codecov Report

Attention: Patch coverage is 77.77778% with 38 lines in your changes missing coverage. Please review.

Project coverage is 80.52%. Comparing base (c4b0fe2) to head (8b39c03).
Report is 10 commits behind head on master.

Files with missing lines	Patch %	Lines
lib/client.c	77.07%	36 Missing ⚠️
lib/http_perform.c	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #787      +/-   ##
==========================================
+ Coverage   74.36%   80.52%   +6.15%     
==========================================
  Files         105      104       -1     
  Lines        9293     9326      +33     
==========================================
+ Hits         6911     7510     +599     
+ Misses       2382     1816     -566

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sfc-gh-mkubik · 2025-01-30T13:33:05Z

include/snowflake/client.h

@@ -526,8 +546,10 @@ typedef struct SF_STMT {
    void* multi_stmt_result_ids;
    int64 multi_stmt_count;
    int64 paramset_size;
-    sf_bool array_bind_supported;
+	  sf_bool array_bind_supported;


Please fix the formatting here

sfc-gh-mkubik · 2025-01-30T15:15:35Z

lib/client.c

+      }
+    }
+
+    int sleep_time = retry_pattern[retry] * 500;


I guess this sleep is supposed to be executed below if (retry < max_retries) to ensure it won't get out of the array bounds. Maybe let's cover the max_retries limit with a unit test.

sfc-gh-mkubik · 2025-01-30T16:03:59Z

lib/client.c

@@ -2684,6 +2907,9 @@ static SF_STATUS _snowflake_execute_with_binds_ex(SF_STMT* sfstmt,
                                  is_string_empty(sfstmt->connection->directURL) ?
                                  NULL : sfstmt->request_id, is_describe_only,
                                  sfstmt->multi_stmt_count);
+
+	  snowflake_cJSON_AddBoolToObject(body, "asyncExec", is_async_exec);


sfc-gh-mmishchenko · 2025-01-30T19:38:13Z

lib/client.c

@@ -55,12 +57,197 @@ static SF_STATUS STDCALL
 _reset_connection_parameters(SF_CONNECT *sf, cJSON *parameters,
                             cJSON *session_info, sf_bool do_validate);

+static const char* query_status_names[] = {


This can be const itself probably, i.e.
static const char* const query_status_names[]

sfc-gh-mmishchenko · 2025-01-31T01:05:45Z

lib/client.c

+
+  // Get query results
+  char query[1024];
+  char* query_template = "select * from table(result_scan('%s'))";


This approach with use RESULT_SCAN to retrieve results has a significant flaw - the original order of the results isn't guaranteed. Probably we should use /queries/{sfqid}/result endpoint instead.

…result scan. Add tests

sfc-gh-ext-simba-nl added 4 commits November 27, 2024 20:08

Add async query support

763153f

Add async test file

974613b

Fix linux build by including unistd.h

a2f2701

Remove unnecessary test

3829b60