feat(frontend):change query_handle to return data stream #5556

ZENOTME · 2022-09-26T09:23:05Z

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

PLEASE DO NOT LEAVE THIS EMPTY !!!

change query_handle to return data stream

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
All checks passed in ./risedev check (or alias, ./risedev c)

Refer to a related PR or issue link (optional)

#5233

codecov · 2022-09-26T11:24:33Z

Codecov Report

Merging #5556 (5fc18fa) into main (c9dc5b9) will decrease coverage by 0.05%.
The diff coverage is 61.76%.

❗ Current head 5fc18fa differs from pull request most recent head bdbf753. Consider uploading reports for the commit bdbf753 to get more accurate results

@@            Coverage Diff             @@
##             main    #5556      +/-   ##
==========================================
- Coverage   74.31%   74.25%   -0.06%     
==========================================
  Files         907      907              
  Lines      142996   143193     +197     
==========================================
+ Hits       106269   106334      +65     
- Misses      36727    36859     +132

Flag	Coverage Δ
rust	`74.25% <61.76%> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/frontend/src/handler/explain.rs	`0.00% <0.00%> (ø)`
src/frontend/src/handler/query.rs	`0.00% <0.00%> (ø)`
src/frontend/src/handler/variable.rs	`0.00% <0.00%> (ø)`
...rontend/src/scheduler/distributed/query_manager.rs	`14.01% <0.00%> (+0.38%)`	⬆️
src/frontend/src/scheduler/local.rs	`0.00% <0.00%> (ø)`
src/utils/pgwire/src/pg_protocol.rs	`61.33% <62.50%> (+0.04%)`	⬆️
src/utils/pgwire/src/pg_extended.rs	`88.90% <75.00%> (-0.22%)`	⬇️
src/utils/pgwire/src/pg_response.rs	`70.11% <80.00%> (+1.82%)`	⬆️
src/frontend/src/handler/describe.rs	`89.25% <100.00%> (+0.08%)`	⬆️
src/frontend/src/handler/show.rs	`87.24% <100.00%> (+0.08%)`	⬆️
... and 178 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

BowenXiao1999

LGTM

src/frontend/src/scheduler/distributed/query_manager.rs

BowenXiao1999 · 2022-09-26T12:08:59Z

src/frontend/src/handler/query.rs

@@ -69,10 +72,15 @@ pub async fn handle_query(
    };

    let rows_count = match stmt_type {
-        StatementType::SELECT => rows.len() as i32,
+        StatementType::SELECT => 0_i32,


Why this is 0?

for query statement, rows_count is meaningless because it's stream mode. We will calculate the row count when we receive the data.
I have added the related comment in the rows_count field of PgResponse.

risingwave/src/utils/pgwire/src/pg_response.rs

Line 80 in 37ce306

// row count of effected row. Used for INSERT, UPDATE, DELETE, COPY, and other statements that

It would be better if we use Option here.

BugenZhao · 2022-09-27T02:12:00Z

src/frontend/src/handler/query.rs

@@ -29,7 +32,7 @@ use crate::scheduler::{
 };
 use crate::session::{OptimizerContext, SessionImpl};

-pub type QueryResultSet = Vec<Row>;
+pub type QueryResultSet = BoxStream<'static, std::result::Result<Row, BoxedError>>;


Can we use generic to avoid this Box? There could be millions of rows in the result set, and I'm not sure whether this impacts the performance a lot.

Indeed it may impact the performance. I try to use generic but it will make so many places full of generic param. I think it's not appropriate to use generic param( at least the way I use .
I think there are other ways to solve this problem:

use the type implement stream trait to avoid generic param

use Vec instead of Row to decrease the time of call next()
BoxStream<'static, std::result::Result<Vec,BoxedError>>

Indeed it may impact the performance.

Could you elaborate this point? 👀

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.

Oh yeees, Sorry I misread it as "may not impact" 😄😄😄

use the type implement stream trait to avoid generic param

Yes. We may Type Alias Impl Trait here.

use Vec instead of Row to decrease the time of call next()
BoxStream<'static, std::result::Result<Vec,BoxedError>>

I have used this way to decrease the time of calling stream.next() .
And I will create a new PR to create a new stream type to avoid Boxstream later.

Please create an issue for that

BugenZhao · 2022-09-27T02:12:58Z

src/frontend/src/handler/query.rs

@@ -69,10 +72,15 @@ pub async fn handle_query(
    };

    let rows_count = match stmt_type {
-        StatementType::SELECT => rows.len() as i32,
+        StatementType::SELECT => 0_i32,


It would be better if we use Option here.

xxchan

Generally LGTM

xxchan · 2022-09-28T09:48:14Z

src/utils/pgwire/src/pg_response.rs

        values: Vec<Row>,
        row_desc: Vec<PgFieldDescriptor>,
    ) -> Self {
        Self {
            stmt_type,
-            row_cnt,
+            row_cnt: row_cnt.unwrap_or(0),


Also change the field to Option?

github-actions bot added the type/feature label Sep 26, 2022

ZENOTME force-pushed the zj/stream_query branch 2 times, most recently from ba2e65b to 3f22b73 Compare September 26, 2022 11:07

ZENOTME requested review from BowenXiao1999, BugenZhao and xxchan September 26, 2022 11:48

BowenXiao1999 approved these changes Sep 26, 2022

View reviewed changes

TennyZhuang changed the title ~~feat(frontend):change query_hanlde to return data stream~~ feat(frontend):change query_handle to return data stream Sep 26, 2022

change query_hanlde to return data_stream

9efe992

ZENOTME force-pushed the zj/stream_query branch from 3f22b73 to 9efe992 Compare September 26, 2022 14:57

BugenZhao reviewed Sep 27, 2022

View reviewed changes

tranfer Vec<Row> instead of Row

699ad82

ZENOTME force-pushed the zj/stream_query branch from 8d8afe3 to 6093a37 Compare September 28, 2022 09:03

xxchan approved these changes Sep 28, 2022

View reviewed changes

use Option<i32> in row_cnt

5fc18fa

ZENOTME force-pushed the zj/stream_query branch from 6093a37 to 5fc18fa Compare September 28, 2022 10:20

ZENOTME added the mergify/can-merge label Sep 28, 2022

Merge branch 'main' into zj/stream_query

bdbf753

ZENOTME linked an issue Sep 28, 2022 that may be closed by this pull request

bug: Frontend should not collect all data before responding. #5233

Closed

ZENOTME mentioned this pull request Sep 28, 2022

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

Closed

mergify bot merged commit 960e11b into main Sep 28, 2022

mergify bot deleted the zj/stream_query branch September 28, 2022 12:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(frontend):change query_handle to return data stream #5556

feat(frontend):change query_handle to return data stream #5556

ZENOTME commented Sep 26, 2022 •

edited

Loading

codecov bot commented Sep 26, 2022 •

edited

Loading

BowenXiao1999 left a comment

BowenXiao1999 Sep 26, 2022

ZENOTME Sep 26, 2022

BugenZhao Sep 27, 2022

BugenZhao Sep 27, 2022

ZENOTME Sep 27, 2022

xxchan Sep 27, 2022

ZENOTME Sep 27, 2022

xxchan Sep 27, 2022 •

edited

Loading

BugenZhao Sep 28, 2022

ZENOTME Sep 28, 2022

xxchan Sep 28, 2022

BugenZhao Sep 27, 2022

xxchan left a comment

xxchan Sep 28, 2022

feat(frontend):change query_handle to return data stream #5556

feat(frontend):change query_handle to return data stream #5556

Conversation

ZENOTME commented Sep 26, 2022 • edited Loading

What's changed and what's your intention?

Checklist

Refer to a related PR or issue link (optional)

codecov bot commented Sep 26, 2022 • edited Loading

Codecov Report

BowenXiao1999 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxchan Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxchan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZENOTME commented Sep 26, 2022 •

edited

Loading

codecov bot commented Sep 26, 2022 •

edited

Loading

xxchan Sep 27, 2022 •

edited

Loading