-
Notifications
You must be signed in to change notification settings - Fork 52
[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 15 commits
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 |
|---|---|---|
|
|
@@ -32,7 +32,7 @@ on: | |
| - master | ||
|
|
||
| env: | ||
| SPARK_VERSION: '4.0.0' | ||
| SPARK_VERSION: '4.0.1' | ||
| HADOOP_VERSION: '3' | ||
|
|
||
| permissions: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ import ( | |
| "errors" | ||
| "fmt" | ||
| "io" | ||
| "iter" | ||
|
|
||
| "github.com/apache/spark-connect-go/spark/sql/utils" | ||
|
|
||
|
|
@@ -368,6 +369,15 @@ func (c *ExecutePlanClient) ToTable() (*types.StructType, arrow.Table, error) { | |
| c.done = false | ||
| for { | ||
| resp, err := c.responseStream.Recv() | ||
| if err != nil { | ||
| fmt.Printf("DEBUG: Recv error: %v, is EOF: %v\n", err, errors.Is(err, io.EOF)) | ||
| } | ||
| if err == nil && resp != nil { | ||
| fmt.Printf("DEBUG: Received response type: %T\n", resp.ResponseType) | ||
| if _, ok := resp.ResponseType.(*proto.ExecutePlanResponse_ResultComplete_); ok { | ||
| fmt.Println("DEBUG: Got ResultComplete!") | ||
| } | ||
| } | ||
|
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. Probably shouldn't keep these debug lines in here. At minimum, use
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. ah, a blunder, thanks
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. removed the debug lines |
||
| // EOF is received when the last message has been processed and the stream | ||
| // finished normally. | ||
| if errors.Is(err, io.EOF) { | ||
|
|
@@ -434,6 +444,110 @@ func (c *ExecutePlanClient) ToTable() (*types.StructType, arrow.Table, error) { | |
| } | ||
| } | ||
|
|
||
| // ToRecordSequence returns a single Seq2 iterator | ||
| func (c *ExecutePlanClient) ToRecordSequence(ctx context.Context) iter.Seq2[arrow.Record, error] { | ||
| // Return Seq2 iterator that directly yields results as they arrive, upstream callers can convert this as needed | ||
| iterator := func(yield func(arrow.Record, error) bool) { | ||
|
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. just do |
||
| // 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(): | ||
| // Yield the context error and stop | ||
| yield(nil, ctx.Err()) | ||
| return | ||
| default: | ||
| } | ||
|
|
||
| resp, err := c.responseStream.Recv() | ||
|
|
||
| // Check for context cancellation after potentially blocking operations | ||
| select { | ||
| case <-ctx.Done(): | ||
| yield(nil, ctx.Err()) | ||
| return | ||
| default: | ||
| } | ||
|
|
||
| // EOF is received when the last message has been processed (Observed on Databricks instances) | ||
| if errors.Is(err, io.EOF) { | ||
| return // Clean end of stream | ||
| } | ||
|
|
||
| // Handle other errors | ||
| if err != nil { | ||
| if se := sparkerrors.FromRPCError(err); se != nil { | ||
| yield(nil, sparkerrors.WithType(se, sparkerrors.ExecutionError)) | ||
| } else { | ||
| yield(nil, err) | ||
| } | ||
| return // Stop on error | ||
| } | ||
|
|
||
| // Only proceed if we have a valid response | ||
| if resp == nil { | ||
| continue | ||
| } | ||
|
|
||
| // Validate session ID | ||
| if resp.GetSessionId() != c.sessionId { | ||
| yield(nil, sparkerrors.WithType( | ||
| &sparkerrors.InvalidServerSideSessionDetailsError{ | ||
| OwnSessionId: c.sessionId, | ||
| ReceivedSessionId: resp.GetSessionId(), | ||
| }, sparkerrors.InvalidServerSideSessionError)) | ||
| return | ||
| } | ||
|
|
||
| // Process schema if present | ||
| if resp.Schema != nil { | ||
| var schemaErr error | ||
| c.schema, schemaErr = types.ConvertProtoDataTypeToStructType(resp.Schema) | ||
| if schemaErr != nil { | ||
| yield(nil, sparkerrors.WithType(schemaErr, sparkerrors.ExecutionError)) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| // Process response types | ||
| 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_: | ||
| record, err := types.ReadArrowBatchToRecord(x.ArrowBatch.Data, c.schema) | ||
| if err != nil { | ||
| yield(nil, err) | ||
| return | ||
| } | ||
|
|
||
| // Yield the record and check if consumer wants to continue | ||
| if !yield(record, nil) { | ||
| // Consumer stopped iteration early | ||
| // Note: Consumer is responsible for releasing the record | ||
| return | ||
| } | ||
|
|
||
| case *proto.ExecutePlanResponse_ResultComplete_: | ||
| c.done = true | ||
| return | ||
|
|
||
| case *proto.ExecutePlanResponse_ExecutionProgress_: | ||
| // Progress updates - ignore for now | ||
|
|
||
| default: | ||
| // Explicitly ignore unknown message types | ||
| } | ||
| } | ||
|
caldempsey marked this conversation as resolved.
|
||
| } | ||
|
|
||
| return iterator | ||
| } | ||
|
|
||
| func NewExecuteResponseStream( | ||
| responseClient proto.SparkConnectService_ExecutePlanClient, | ||
| sessionId string, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.