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

NOT FOR MERGING: Isolate hang in Arrow test #86

Closed
wants to merge 3 commits into from
Closed

Conversation

krlmlr
Copy link
Collaborator

@krlmlr krlmlr commented Feb 24, 2024

This PR shows a minimal change that leads to dbSendQuery(arrow = TRUE) hanging.

Test code:

con <- DBI::dbConnect(duckdb::duckdb(), ":memory:")
DBI::dbSendQuery(con, "SELECT 1", arrow = TRUE)

On the main branch of this repo, this code works. On the commit "Harmless parts" from this PR, the code works. On the commit "Remove special case", it does not terminate.

This change is part of duckdb v0.10.0. I'd like to resolve this before proceeding with the CRAN release.

I also tested with SET threads = 1, same result.

Upstream: duckdb/duckdb#10245 (comment)

The first commit in this PR corresponds to the main branch just before merging duckdb/duckdb#10245. The last two commits are a decomposition of duckdb/duckdb@68c9e1a.

@Tmonster: Can you replicate

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
@krlmlr krlmlr changed the title chore: Update vendored sources to duckdb/duckdb@c815886f33437b11bfdae956db90021000785449 NOT FOR MERGING: Isolate hang in Arrow test Feb 24, 2024
@krlmlr krlmlr marked this pull request as draft February 24, 2024 16:03
@krlmlr krlmlr added this to the 0.10.0 milestone Feb 24, 2024
@krlmlr krlmlr requested a review from Tmonster February 24, 2024 16:13
@Tmonster
Copy link
Contributor

I can replicate. Will discuss with Core team as to why this is happening.

@Tmonster Tmonster mentioned this pull request Feb 26, 2024
@Tmonster
Copy link
Contributor

I have another PR up here #89 that should better address this issue.

@krlmlr feel free to cherry-pick the commit onto a branch that you will use for updating the vendored sources.

@krlmlr
Copy link
Collaborator Author

krlmlr commented Feb 27, 2024

Fixed in #90.

@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