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

feat(frontend): add virtual StreamTableScan node #1024

Merged
merged 3 commits into from
Mar 17, 2022
Merged

Conversation

skyzh
Copy link
Contributor

@skyzh skyzh commented Mar 17, 2022

Signed-off-by: Alex Chi [email protected]

What's changed and what's your intention?

The StreamScan will be converted into chain node and its two inputs when to_prost.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

Comment on lines 10 to 11
/// `StreamScan` is a virtual plan node to represent a stream scan. It will be converted to chain +
/// upstream table scan + batch table scan when converting to MView creation request.
Copy link
Member

Choose a reason for hiding this comment

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

Just FYI, the source of a streaming plan might be either Source or Chain, depending on whether the object to scan is a Source or MaterializedView.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... So we need both SourceScan node and TableScan node?

Copy link
Member

Choose a reason for hiding this comment

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

Under the design of materialized sources, there will all be tables (mviews)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I don't think this design is really settled down. I'll separate StreamScan to two nodes: SourceScan and TableScan...

Signed-off-by: Alex Chi <[email protected]>
@skyzh skyzh requested a review from fuyufjh March 17, 2022 08:07
@@ -69,7 +69,7 @@
BatchScan { table: "t", columns: ["v1"] }
stream_plan: |
StreamProject { exprs: [$0], expr_alias: [Some("v1")] }
StreamTableSource { logical: LogicalScan { table: "t", columns: ["v1"] } }
StreamScan { logical: "LogicalScan { table: \"t\", columns: [\"v1\"] }" }
Copy link
Member

Choose a reason for hiding this comment

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

Emmm, still strange to me... Can we keep the original format 😇

Copy link
Member

@fuyufjh fuyufjh Mar 17, 2022

Choose a reason for hiding this comment

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

Oh, I know the problem... Please don't use logical, but display the fields you want.

e.g.

impl fmt::Display for BatchSort {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        f.debug_struct("BatchSort")
            .field("order", self.order())
            .finish()
    }
}

Copy link
Contributor Author

@skyzh skyzh Mar 17, 2022

Choose a reason for hiding this comment

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

That's true! I was thinking of #1016 previously, and I found that we have to stick to our own println instead of using utilities provided by std::fmt.

Signed-off-by: Alex Chi <[email protected]>
@skyzh skyzh requested a review from fuyufjh March 17, 2022 08:17
@skyzh skyzh changed the title feat(frontend): add virtual StreamScan node feat(frontend): add virtual StreamTableScan node Mar 17, 2022
Copy link
Member

@fuyufjh fuyufjh left a comment

Choose a reason for hiding this comment

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

The Display format is still strange... I'll fix that later. LGTM for the rest

@codecov
Copy link

codecov bot commented Mar 17, 2022

Codecov Report

Merging #1024 (cfd852d) into main (0f23d89) will increase coverage by 0.08%.
The diff coverage is 47.61%.

@@             Coverage Diff              @@
##               main    #1024      +/-   ##
============================================
+ Coverage     71.05%   71.14%   +0.08%     
  Complexity     2766     2766              
============================================
  Files           977      978       +1     
  Lines         57587    57702     +115     
  Branches       1790     1790              
============================================
+ Hits          40921    41053     +132     
+ Misses        15775    15758      -17     
  Partials        891      891              
Flag Coverage Δ
java 61.03% <ø> (ø)
rust 74.97% <47.61%> (+0.10%) ⬆️

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

Impacted Files Coverage Δ
rust/frontend/src/handler/create_mv.rs 0.00% <0.00%> (ø)
rust/frontend/src/optimizer/plan_node/mod.rs 40.35% <0.00%> (-2.25%) ⬇️
...tend/src/optimizer/plan_node/stream_source_scan.rs 0.00% <0.00%> (ø)
...ntend/src/optimizer/plan_node/stream_table_scan.rs 75.00% <75.00%> (ø)
...t/frontend/src/optimizer/plan_node/logical_scan.rs 100.00% <100.00%> (ø)
rust/meta/src/hummock/hummock_manager_tests.rs 93.57% <0.00%> (-1.53%) ⬇️
rust/storage/src/hummock/error.rs 18.18% <0.00%> (-0.87%) ⬇️
rust/storage/src/hummock/local_version_manager.rs 77.45% <0.00%> (-0.55%) ⬇️
...rage/src/hummock/mock/mock_hummock_meta_service.rs 70.68% <0.00%> (ø)
... and 7 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@skyzh skyzh merged commit 20f0a43 into main Mar 17, 2022
@skyzh skyzh deleted the skyzh/virtual-stream-scan branch March 17, 2022 08:38
fn to_stream_prost_body(&self) -> ProstStreamNode {
// TODO: support real serialization
ProstStreamNode::SourceNode(Default::default())
ProstStreamNode::MergeNode(Default::default())
Copy link
Member

Choose a reason for hiding this comment

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

Should be SourceNode?

StreamTableSource::new(self.clone()).into()
StreamTableScan::new(self.clone()).into()
Copy link
Member

Choose a reason for hiding this comment

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

We should dispatch to different physical plan according to whether the "table" is mview or source.
Perhaps we should do this in binder?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants