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! :) |
2 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 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.