Skip to content
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

Fix Hanging query #89

Closed
wants to merge 2 commits into from
Closed

Conversation

Tmonster
Copy link
Contributor

As mentioned in #86, duckdb/duckdb#10245 would cause a simple case in duckdb-r to hang. This PR presents a solution that better addresses the infinite loop issue. Apparently Query results can be blocked or finished if allow_streaming_result = true. For arrow, we pass true for streaming result, so we need to check is the result is blocked or finished instead of just finished. A blocked query just means fetch needs to be called.

krlmlr and others added 2 commits February 24, 2024 16:48
moved buffered_query_result -> stream_query_result
initialize buffered count
use the buffered query result in the sqllogic tester
uncomment Materialize, taken from StreamQueryResult
use Initialize and Copy
added TODO
move the Fetch logic into the BufferedQueryResult, will be the exact same for the batched variant
add virtual destructor
introduce base class BufferedData, SimpleBufferedData inherits from it
preparation for batched variant
keep track of how many tuples would be added, to reduce the chance we unblock a sink only for it to block again right away
add assertion to make sure  doesn't go out into random memory
always output chunks of STANDARD_VECTOR_SIZE from Fetch if possible, have to construct this from (potentially) many smaller chunks
fix CI
move BufferedData out of BufferedQueryResult
PoC seems to be working
formatting
create the QueryResult before the pipeline is complete so we can keep it blocked and unblock it later
@Tmonster Tmonster changed the title Fi Hanging query Fix Hanging query Feb 26, 2024
@krlmlr
Copy link
Collaborator

krlmlr commented Feb 26, 2024

Thanks. Will 010d6af on top of #86 fix the issue? I'll investigate.

@Tmonster
Copy link
Contributor Author

Will 010d6af on top of #86 fix the issue?

Yes that would fix the issue

Are efc7cd5 and ec2cf28 necessary? The fix should work just by itself as well (although I did not do a full regression test, so I'm not sure)

@krlmlr
Copy link
Collaborator

krlmlr commented Feb 27, 2024

Done in #90. Thanks!

@krlmlr krlmlr closed this Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants