docs: document that search() results share structure with the query cache - #617
Conversation
|
Thanks for looking into this! Unfortunately, this PR has a major flaw: it significantly degrades performance for all users of the cache:
Ultimately, thinking about this again, I think there is no fix for #516 that does not introduce a performance penalty (deep copy) or break the API contract (make all results immutable). Given that my main issue was that the described behavior is unexpected, maybe it would be better to just properly document it. What do you think? |
|
Agreed, deep-copy isn't worth it here — the read path is the hot path and a 150-4000x hit for something meant to be a fast lookup is a bad trade for an edge case (mutating a returned result before the next identical search). I don't see a fix that's both cheap and complete: shallow-copying the top-level dict on read would stop attribute-reassignment corruption cheaply but not nested-value mutation, which is a half-fix that could still surprise someone. Happy to convert this into a docs-only PR noting that search() results share structure with the cache and must be treated as read-only — let me know if you'd rather write that note yourself. |
|
A docs-only PR is fine with me |
…ache Table.search() results are read from and written to the internal cache as a shallow list copy, so a returned document is the same object the cache holds. Mutating one (a key or a nested value) corrupts what a later identical search returns. msiemens#516 asked for a fix rather than a doc note; per the discussion there, a deep copy costs 150-4000x on the read hot path, so this documents the caveat instead. Fixes msiemens#516
b074091 to
1e8b70f
Compare
|
Done — reverted the code change, kept a docstring note on |
|
Thanks for reworking this! Could you also add a brief note to |
|
Done — added a note to the "Query Caching" section of |
|
Thanks, @gaoflow! |
Update — converted to a docs-only PR
Per discussion below: a deep copy costs 150-4000x on the read hot path, and there's no fix that's both cheap and complete (a shallow top-level copy stops attribute reassignment but not nested mutation). Reverted the code change; the fix is now a docstring note on
Table.search()plus a test that documents the actual sharing behavior (list is fresh, documents inside it are the cached objects).Original problem (#516)
Table.searchstored and returned shallow copies of the document list, so a returnedDocumentis the same object the cache holds. Mutating a key or a nested value in a returned document corrupts what a later identical search returns:Tests
test_query_cache_documents_are_shared_with_cachedocuments the boundary: mutating the returned list is safe, mutating a document inside it is not. 219/219 pass.