Skip to content

Normalise Execution Response (clean backend interfaces) #587

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

Merged
merged 49 commits into from
Jun 11, 2025

Conversation

varun-edachali-dbx
Copy link
Collaborator

@varun-edachali-dbx varun-edachali-dbx commented Jun 9, 2025

What type of PR is this?

  • Refactor

Description

The overall goal is to clean up the backend interfaces to facilitate the introduction of the SEA client with minimal overhead.

  • Earlier, ExecuteResponse was used as a Thrift aligned object for the return type of execution commands. Now, it is used as a normalised object store for common state that is passed as a parameter to the ResultSet type to prevent too many primitive types.
  • More common functionality and attributes have been pulled up to the common base ResultSet class.
  • Move Queue initialisation away from the execution phase, instead doing it directly in the ResultSet constructor, to better decouple the exec and fetch phases.
               # Before                                       # After
                                                
ExecuteResponse = namedtuple(                   @dataclass
  "ExecuteResponse",                            class ExecuteResponse:
  "command_handle                                 command_id: CommandId
   status                                         status: CommandState
   description                                    description: Optional[List[Tuple]] = None
   has_more_rows                                 
   arrow_queue                                    
   has_been_closed_server_side                    has_been_closed_server_side: bool = False
   lz4_compressed                                 lz4_compressed: bool = True
   is_staging_operation                           is_staging_operation: bool = False
   arrow_schema_bytes"                            arrow_schema_bytes: Optional[bytes] = None
)                                                 result_format

How is this tested?

  • Unit tests
  • E2E Tests
  • Manually
  • N/A

Related Tickets & Documents

https://docs.google.com/document/d/1Y-eXLhNqqhrMVGnOlG8sdFrCxBTN1GdQvuKG4IfHmo0/edit?usp=sharing

@varun-edachali-dbx varun-edachali-dbx changed the base branch from main to sea-migration June 9, 2025 06:29
Signed-off-by: varun-edachali-dbx <[email protected]>
Signed-off-by: varun-edachali-dbx <[email protected]>
Signed-off-by: varun-edachali-dbx <[email protected]>
Signed-off-by: varun-edachali-dbx <[email protected]>
Signed-off-by: varun-edachali-dbx <[email protected]>
Signed-off-by: varun-edachali-dbx <[email protected]>
Signed-off-by: varun-edachali-dbx <[email protected]>
Signed-off-by: varun-edachali-dbx <[email protected]>
Signed-off-by: varun-edachali-dbx <[email protected]>
Signed-off-by: varun-edachali-dbx <[email protected]>
Signed-off-by: varun-edachali-dbx <[email protected]>
Signed-off-by: varun-edachali-dbx <[email protected]>
Signed-off-by: varun-edachali-dbx <[email protected]>
Signed-off-by: varun-edachali-dbx <[email protected]>
Signed-off-by: varun-edachali-dbx <[email protected]>
@samikshya-db
Copy link
Contributor

arrow_schema_bytes - this is also specific to thrift flow, isn't it?

@varun-edachali-dbx
Copy link
Collaborator Author

arrow_schema_bytes - this is also specific to thrift flow, isn't it?

My understanding is that it will be used by SEA for the construction of the arrow table for the EXTERNAL_LINKS disposition. Is that right?

Signed-off-by: varun-edachali-dbx <[email protected]>
Copy link
Contributor

@jayantsing-db jayantsing-db left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments inline. rest lgtm.

…tations and not used over the wire

Signed-off-by: varun-edachali-dbx <[email protected]>
@samikshya-db
Copy link
Contributor

My understanding is that it will be used by SEA for the construction of the arrow table for the EXTERNAL_LINKS disposition. Is that right?

This is not correct. But given the fact that we have discussed the mental model with J on this offline, i am good. Approving.

Signed-off-by: varun-edachali-dbx <[email protected]>
@varun-edachali-dbx varun-edachali-dbx merged commit 6d63df0 into sea-migration Jun 11, 2025
23 checks passed
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.

3 participants