Skip to content
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

frontend: support concrete stream type rather than using BoxStream #5624

Closed
ZENOTME opened this issue Sep 28, 2022 · 3 comments · Fixed by #5677
Closed

frontend: support concrete stream type rather than using BoxStream #5624

ZENOTME opened this issue Sep 28, 2022 · 3 comments · Fixed by #5677
Assignees
Labels
component/frontend Protocol, parsing, binder. type/feature

Comments

@ZENOTME
Copy link
Contributor

ZENOTME commented Sep 28, 2022

Is your feature request related to a problem? Please describe.

as discuss in #5556
We use BoxStream to transfer data now. But Boxstream will impact the performance because it's a dynamic bind. Boxstream is Box, it's a dynamic bind. So I guess every time we call 'stream.next()' to get a row, it needs to call by some mechanism like vtable. Hence if there are millions of rows in the result set, we may need to access vtable millions of times.

Describe the solution you'd like

``The solution is to implement the concrete type which implements stream trait, such as:

struct LocalQueryStream {
 ...
}
impl Stream for LocalQueryStream{
  fn poll_next(){
   ...
  }
}

so that we can use the concrete result type instead of BoxStream to prevent dynamic binding.

// original version
struct PgResponse{
  ...
  result: BoxStream<>
}

// new version 
struct PgResponse{
  result: LocalQueryStream
}

Describe alternatives you've considered

No response

Additional context

No response

@github-actions github-actions bot added this to the release-0.1.14 milestone Sep 28, 2022
@ZENOTME ZENOTME added the component/frontend Protocol, parsing, binder. label Sep 28, 2022
@Gun9niR Gun9niR self-assigned this Sep 29, 2022
@Gun9niR
Copy link
Contributor

Gun9niR commented Sep 29, 2022

Should we use enum dispatch for different concrete stream types?

@ZENOTME
Copy link
Contributor Author

ZENOTME commented Sep 29, 2022

Yes, Because we have different stream: LocalQueryStream and DistributeQueryStream.

@Gun9niR
Copy link
Contributor

Gun9niR commented Sep 29, 2022

I wonder how many generic params you have used in your previous attempt. I imagine they will appear in fn signatures containing QueryResultSet/PgResultSet, and PgResponse's declaration? How many generic params are introduced?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/frontend Protocol, parsing, binder. type/feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants