-
Notifications
You must be signed in to change notification settings - Fork 51
[SPARK-52780] Add StreamRows and Arrow Record Streaming #152
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
78c7091
5e0a589
c277f5b
7ce5d47
2b6044a
1a897ef
8c18703
3dcab75
485067e
f285079
917ce9f
ad7e935
d38170b
a18468f
434a579
0432bde
928e9b3
146e423
aa4b293
fb2a9aa
b29e5ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,5 +48,6 @@ type SparkConnectClient interface { | |
|
|
||
| type ExecuteResponseStream interface { | ||
| ToTable() (*types.StructType, arrow.Table, error) | ||
| ToRecordBatches(ctx context.Context) (<-chan arrow.Record, <-chan error, *types.StructType) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be good to add comments on the how this is intended to be used and what the difference to the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 100% I'll add this to the next commit.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added |
||
| Properties() map[string]any | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -434,6 +434,119 @@ func (c *ExecutePlanClient) ToTable() (*types.StructType, arrow.Table, error) { | |
| } | ||
| } | ||
|
|
||
| func (c *ExecutePlanClient) ToRecordBatches(ctx context.Context) (<-chan arrow.Record, <-chan error, *types.StructType) { | ||
| recordChan := make(chan arrow.Record, 10) | ||
| errorChan := make(chan error, 1) | ||
|
|
||
| go func() { | ||
| defer func() { | ||
| // Ensure channels are always closed to prevent goroutine leaks | ||
| close(recordChan) | ||
| close(errorChan) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does close require any error handling or is this final state.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Final state here. Once closed, no more sends are allowed (sending on a closed channel panics), but you can still receive any buffered items, and subsequent receives yield the zero value immediately. Then attempting to close a closed channel will panic.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is outdated now, we use seq2 for pulls which simplifies a bit |
||
| }() | ||
|
|
||
| // Explicitly needed when tracking re-attachable execution. | ||
| c.done = false | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re-attachable execution is optional, we need to make sure it works with both modes.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not exactly sure how this is supposed to be implemented, any similar code or resources you can point out before I run over this again?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wouldn't this be a race condition? Should we be locking around accessing
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Taking another look at this. I've spliced in the approach from ToTable(), originally written by @grundprinzip. Instead of writing to the shared c.done field, ToRecordSequence now tracks completion with a local done variable inside the closure. After EOF, it checks if c.opts.ReattachExecution && !done and yields an error, the same way ToTable() does. This removes the race condition because nothing shared is being mutated, and it behaves correctly in both reattachable and non-reattachable modes. I do think we should DRY this up eventually, but judiciously. I've kept the code WET for now intentionally. I want both code paths to remain directly comparable until I fully understand the domain. Having @grundprinzip's original logic in ToTable() sitting side by side against my equivalent in ToRecordSequence makes it much easier to reason about correctness and spot differences. So rather than DRY it up in this PR, I'd like to take ownership of a fast follow-up where I consolidate ToRecordSequence and ToTable once this gets merged. That said, happy to take a crack. I'm being cautious since this is an open source project, and I'd rather not introduce a negative diff that risks breaking users when the current approach is correct, comparable, and maintains parity between the two separate critical paths. |
||
|
|
||
| for { | ||
| // Check for context cancellation before each iteration | ||
| select { | ||
| case <-ctx.Done(): | ||
| // Context cancelled - send the error and return immediately | ||
| select { | ||
| case errorChan <- ctx.Err(): | ||
| default: | ||
| // Channel might be full, but we're exiting anyway | ||
| } | ||
| return | ||
| default: | ||
| // Continue with normal processing | ||
| } | ||
|
|
||
| resp, err := c.responseStream.Recv() | ||
|
|
||
| // Check for context cancellation after potentially blocking operations | ||
| select { | ||
| case <-ctx.Done(): | ||
| select { | ||
| case errorChan <- ctx.Err(): | ||
| default: | ||
| } | ||
| return | ||
| default: | ||
| } | ||
|
|
||
| // EOF is received when the last message has been processed and the stream | ||
| // finished normally. | ||
| if errors.Is(err, io.EOF) { | ||
| return | ||
| } | ||
|
|
||
| // If the error was not EOF, there might be another error. | ||
| if se := sparkerrors.FromRPCError(err); se != nil { | ||
| select { | ||
| case errorChan <- sparkerrors.WithType(se, sparkerrors.ExecutionError): | ||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| return | ||
| } | ||
|
|
||
| // Check if the response has already the schema set and if yes, convert | ||
| // the proto DataType to a StructType. | ||
| if resp.Schema != nil && c.schema == nil { | ||
| c.schema, err = types.ConvertProtoDataTypeToStructType(resp.Schema) | ||
| if err != nil { | ||
| select { | ||
| case errorChan <- sparkerrors.WithType(err, sparkerrors.ExecutionError): | ||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| return | ||
| } | ||
| } | ||
|
|
||
| switch x := resp.ResponseType.(type) { | ||
| case *proto.ExecutePlanResponse_SqlCommandResult_: | ||
| if val := x.SqlCommandResult.GetRelation(); val != nil { | ||
| c.properties["sql_command_result"] = val | ||
| } | ||
|
|
||
| case *proto.ExecutePlanResponse_ArrowBatch_: | ||
| // This is what we want - stream the record batch | ||
| record, err := types.ReadArrowBatchToRecord(x.ArrowBatch.Data, c.schema) | ||
| if err != nil { | ||
| select { | ||
| case errorChan <- err: | ||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| return | ||
| } | ||
|
|
||
| // Try to send the record, but respect context cancellation | ||
| select { | ||
| case recordChan <- record: | ||
| // Successfully sent | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All this effort just so we can do this. Again, we should opt in to a DRY implementation with ToTable.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any ideas how to improve this? I'm not crazy familiar with golang and the idiomatic implementation of channels.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will have a think, and make a proposal, then run that by you in this thread before implementation.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. left this for now, I think we can punt this down the road, mainly because I don't think it needs to be the focus of this PR but maybe the focus of a following one |
||
| case <-ctx.Done(): | ||
| // Context cancelled while trying to send - release the record and exit | ||
| record.Release() | ||
| return | ||
| } | ||
|
|
||
| case *proto.ExecutePlanResponse_ResultComplete_: | ||
| c.done = true | ||
| return | ||
|
|
||
| default: | ||
| // Explicitly ignore messages that we cannot process at the moment. | ||
| } | ||
| } | ||
|
caldempsey marked this conversation as resolved.
|
||
| }() | ||
|
|
||
| return recordChan, errorChan, c.schema | ||
| } | ||
|
|
||
| func NewExecuteResponseStream( | ||
| responseClient proto.SparkConnectService_ExecutePlanClient, | ||
| sessionId string, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added so we can consume Apache Arrow record batches rather than the entire table at once, in truth, ToTable should re-use private methods w/ ToRecordBatches, but I've avoided this to avoid breaking clients for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than exposing a channel directly, it might make more sense to use either an
array.RecordReaderor aniter.Seq2[arrow.Record, error].Either that, or create something that wraps the two channels and produces one of those, simply to make it easier to consume for users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'll apply this comment (and all the others) in the next round of commits 🥳
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use Seq2