fix: Handling of sync and async requests#313
Draft
casperlehmann wants to merge 21 commits intotekumara:mainfrom
Draft
fix: Handling of sync and async requests#313casperlehmann wants to merge 21 commits intotekumara:mainfrom
casperlehmann wants to merge 21 commits intotekumara:mainfrom
Conversation
ed10609 to
2b6a6ae
Compare
c9f38c4 to
a57e6ad
Compare
Co-Authored-By: Andrew Bird <25768973+abirdzendesk@users.noreply.github.com>
a57e6ad to
ff43a93
Compare
The results cache was storing a dictionary format, but result_scan() and get_results_from_sfqid() expect a tuple of (arrow_table, rowcount, last_sql, last_params, last_transformed). Updated server.py to store the tuple format and reconstruct the response dictionary when needed for GET requests. Fixes test_server_async_query_with_retrieval. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
test: add comprehensive tests for results cache format
Add 6 new tests validating:
- result_scan() end-to-end functionality
- GET /queries/{query_id}/result endpoint
- LRU cache eviction (>50 entries)
- Empty result set caching
- Multiple query cache isolation
- Error handling for missing queries
Also fixes discovered bugs:
- Add rowtype as 6th element in cache tuple (was missing)
- Change results_cache from dict to OrderedDict for LRU eviction
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The results cache now stores a consistent 6-tuple format across both server.py and cursor.py. Previously, server.py stored 6-tuple with rowtype but cursor.py stored 5-tuple without it, causing "not enough values to unpack" errors when result_scan() or get_results_from_sfqid() tried to restore cached results. Rowtype computation skips DESCRIBE queries to avoid infinite recursion. Fixes test_execute_async_and_get_results_from_sfqid. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
a16fd35 to
8aa69af
Compare
The server now respects the FAKESNOW_DB_PATH environment variable if set, allowing users to specify a persistent database file path. If not set, it falls back to the default in-memory database behavior. This enables multiple connections to share either a persistent or in-memory database depending on configuration needs.
af502dd to
49b74aa
Compare
40d6e39 to
e349548
Compare
e349548 to
ee1c542
Compare
e0a0ac1 to
cec6926
Compare
4f1534a to
c568aa2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes Fakesnow's compatibility with both Node.js and Python Snowflake SDKs by implementing the correct response format and query result polling behavior.
Changes
1. Add status code for successful queries (ff43a93)
code: "0"field to POST response to explicitly signal "results ready"code: "0"does mean that we are essentially never serving async responses. But since I'm still not sure how to make that decision programmatically, I have left it like this for now.2. Add required response fields (a711a9c)
queryId,sqlText,returned,total, etc.rowset) and Python (rowsetBase64)3. Add async GET endpoint support (13ad8de)
get_cached_query_result()function to retrieve cached query results/queries/{query_id}/resultendpoint for async polling behaviorcode: "333333"(in progress) orcode: "333334"(async execution) is returnedconn.results_cache) with LRU eviction (max 50 entries) to store query results byqueryId"code"field in thequery_requestresponse, this is no longer the case. Since we added a default value (code: "0"--completed) toquery_request, the request will never enter the polling state, and thisGETendpoint will no longer get hit. But I am leaving it in, since it is already working and, ideally, thequery_requestshould be able to returnin-progressvalues forcodeas well (perhaps by setting a timeout in on the query execution).4. Add SafeJSONResponse for datetime serialization (88e0bfd)
datetime5. Store results cache in tuple format with rowtype (a16fd35, 63e73fe)
conn.results_cache) is never exposed externally - only used forresult_scan()andget_results_from_sfqid()(arrow_table, rowcount, last_sql, last_params, last_transformed, rowtype)/queries/{query_id}/resultendpoint to generaterowsetBase64(Arrow IPC format needed by Python SDK)6. Add support for persisting database in server mode
Allow clients the option to set
FAKESNOW_DB_PATHwhen running in server mode, in order to persist data on disk.Technical Details
The changes implement the private SDK API (
/queries/v1/query-request) used by Node.js and Python Snowflake connectors, not the public Snowflake SQL API. Key differences:"0"(not"090001"from public API)rowset/rowsetBase64(notdataarray from public API)"333333"(in progress),"333334"(detached) - defined in Node.js SDK and Python SDKSee also: Snowflake SQL API Reference for comparison with public API behavior.