GH-48076: [C++][Flight] fix GeneratorStream for Tables #48082
Conversation
no23reason
requested review from
AlenkaF,
lidavidm,
raulcd and
rok
as code owners
|
|
github-actions
bot
added
Component: C++
Component: Python
Component: FlightRPC
awaiting review
no23reason
force-pushed
the
gh-48076-generator-stream
branch
from
ef7f961 to
c11ea33
Compare
|
Unfortunately, there is more at play than I expected and I am not sure how to proceed :( but I believe the tests I added should be included in upstream and should be passing so that all the possible types backing the GeneratorStream are tested. |
raulcd
left a comment
raulcd
left a comment
There was a problem hiding this comment.
Thank you very much for raising the issue and working on this! @no23reason
| if (!writer_) { | ||
| return Status::UnknownError( | ||
| "Writer should be initialized before reading Next batches"); | ||
| RETURN_NOT_OK(InitializeWriter()); |
There was a problem hiding this comment.
I think in this specific scenario we might have to drop the first message after calling writer_->WriteRecordBatch, which will be a schema message, after creation of the ipc::internal::OpenRecordBatchWriter and we want to keep the rest of the messages which should be the expected RecordBatch for those cases.
I would have to debug but from what I understand that might be the cause of the current problem.
There was a problem hiding this comment.
Thank you for the suggestion, that is a good idea. I will take a look as soon as I can, unfortunately something more urgent cropped up I have to deal with first :( I will let you know what I find out
github-actions
bot
added
awaiting changes
no23reason
force-pushed
the
gh-48076-generator-stream
branch
from
c11ea33 to
72d4258
Compare
github-actions
bot
added
awaiting change review
After the changes in apache#47115, GeneratorStreams backed by anything else than RecordBatches failed. This includes Tables and RecordBatchReaders. This was caused by a too strict assumption that the RecordBatchStream#GetSchemaPayload would always get called, which is not the case when the GeneratorStream is backed by a Table or a RecordBatchReader. So to fix this, remove the assertion and instead initialize the writer on first access. Also, to accommodate for this case, drop the incoming message when initializing the writer in Next, as the message there is of the SCHEMA type and we want RECORD_BATCH one.
no23reason
force-pushed
the
gh-48076-generator-stream
branch
from
72d4258 to
921e6b7
Compare
github-actions
bot
added
awaiting merge
|
Thank you for guiding me through it! :) |
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
A schema message may be followed by dictionary batches - so in this context would that effectively be treated as dictionary replacement? Or would those batches be unexpected?
github-actions
bot
added
awaiting changes
|
@lidavidm I added some more tests that use dictionaries and they seem to be passing. I ran a debugger on one of them and saw a DICTIONARY_BATCH coming in and being returned from So I am not sure it answers your question, though. |
To ensure the changes in the previous commit do not break dictionaries, add more tests with data using them.
no23reason
force-pushed
the
gh-48076-generator-stream
branch
from
6014860 to
1c024c7
Compare
|
I am not sure whether I should do a rebase to try fixing the remaining failures (they all seem SQL/Substrait related), but I do not want to waste the CI resources 🤔 |
|
The CI failures are unrelated see: We are waiting for a new version of protobuf (33.1) released yesterday to be available on homebrew. It is expected to fix the problem. |
|
Right, thank you for the details, so I will wait until that is resolved and then do a rebase, so that we have "all green" CI :) |
I don't think is strictly required, once @lidavidm can take another look, if he's happy we can merge as we know those CI failures are unrelated. Nevertheless if those are fixed before a review, a rebase to have a green CI is always welcome :) |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit f5b3fc7. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them. |
3 participants
Rationale for this change
After the changes in #47115, GeneratorStreams backed by anything else than RecordBatches failed. This includes Tables and RecordBatchReaders.
This was caused by a too strict assumption that the RecordBatchStream#GetSchemaPayload would always get called, which is not the case when the GeneratorStream is backed by a Table or a RecordBatchReader.
What changes are included in this PR?
Removal of the problematic assertion and initialization of the writer object when it is needed first.
Also, to accommodate for this case, drop the incoming message when initializing the writer in Next, as the message there
is of the SCHEMA type and we want RECORD_BATCH or DICTIONARY_BATCH one.
Are these changes tested?
Yes, via CI. Tests for the GeneratorStreams were extended so that they test GeneratorStreams backed by Tables and RecordBatchReaders, not just RecordBatches.
Are there any user-facing changes?
No, just a fix for a regression restoring the functionality from version 21.0.0 and earlier.