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(streaming): watermark on append only table #8207

Merged
merged 3 commits into from
Feb 28, 2023

Conversation

yuhao-su
Copy link
Contributor

@yuhao-su yuhao-su commented Feb 27, 2023

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

  • Allow define a watermark on append only table created with source.

Checklist For Contributors

Checklist For Reviewers

  • I have requested macro/micro-benchmarks as this PR can affect performance substantially, and the results are shown.

Documentation

  • My PR DOES NOT contain user-facing changes.
Click here for Documentation

Types of user-facing changes

Please keep the types that apply to your changes, and remove the others.

  • Installation and deployment
  • Connector (sources & sinks)
  • SQL commands, functions, and operators
  • RisingWave cluster configuration changes
  • Other (please specify in the release note below)

Release note

@codecov
Copy link

codecov bot commented Feb 27, 2023

Codecov Report

Merging #8207 (4794682) into main (cb7e029) will decrease coverage by 0.01%.
The diff coverage is 89.24%.

@@            Coverage Diff             @@
##             main    #8207      +/-   ##
==========================================
- Coverage   71.70%   71.70%   -0.01%     
==========================================
  Files        1131     1131              
  Lines      182341   182390      +49     
==========================================
+ Hits       130748   130775      +27     
- Misses      51593    51615      +22     
Flag Coverage Δ
rust 71.70% <89.24%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
src/frontend/src/handler/create_table_as.rs 0.00% <0.00%> (ø)
...frontend/src/optimizer/plan_node/generic/source.rs 93.47% <ø> (ø)
src/sqlparser/src/parser.rs 91.26% <62.50%> (-0.13%) ⬇️
src/frontend/src/handler/create_table.rs 91.12% <84.84%> (-0.41%) ⬇️
src/sqlparser/src/ast/statement.rs 67.29% <95.23%> (+0.14%) ⬆️
src/frontend/planner_test/src/lib.rs 75.05% <100.00%> (+0.11%) ⬆️
src/frontend/src/handler/alter_table.rs 87.09% <100.00%> (+0.51%) ⬆️
src/frontend/src/handler/create_source.rs 45.09% <100.00%> (ø)
src/frontend/src/handler/explain.rs 75.38% <100.00%> (+0.58%) ⬆️
src/frontend/src/handler/mod.rs 61.17% <100.00%> (+0.30%) ⬆️
... and 9 more

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

Copy link
Contributor

@st1page st1page left a comment

Choose a reason for hiding this comment

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

LGTM! we can add some e2e tests to test if we can filter the outdated data. (also fine in next pr)

@@ -13,3 +13,13 @@
└─StreamRowIdGen { row_id_index: 1 }
└─StreamWatermarkFilter { watermark_descs: [idx: 0, expr: (v1 - '00:00:01':Interval)] }
└─StreamSource { source: "t", columns: ["v1", "_row_id"] }
- name: watermark on append only table
sql: |
explain create table t (v1 timestamp with time zone, watermark for v1 as v1 - INTERVAL '1' SECOND) with (connector = 'kafka', kafka.topic = 'kafka_3_partition_topic', kafka.brokers = '127.0.0.1:1234', kafka.scan.startup.mode='earliest', appendonly=true) ROW FORMAT JSON;
Copy link
Contributor

@st1page st1page Feb 27, 2023

Choose a reason for hiding this comment

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

emm, our usual usage is without the external connector. So maybe removing it is better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can keep both behaviors undocumented for test. I'll add a new planner test.

Copy link
Contributor

@xx01cyx xx01cyx left a comment

Choose a reason for hiding this comment

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

Generally LGTM. BTW where is the "append only" part of this PR? 👀

Comment on lines +468 to +475
if !append_only && !watermark_descs.is_empty() {
return Err(ErrorCode::NotSupported(
"Defining watermarks on table requires the table to be append only.".to_owned(),
"Set the option `appendonly=true`".to_owned(),
)
.into());
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xx01cyx append only part

@yuhao-su yuhao-su added this pull request to the merge queue Feb 28, 2023
@yuhao-su yuhao-su changed the title feat(streaming): watermark on append only table created with source feat(streaming): watermark on append only table Feb 28, 2023
Merged via the queue into main with commit 44e98a3 Feb 28, 2023
@yuhao-su yuhao-su deleted the yuhao/watermark_append_only_table branch February 28, 2023 04:10
Comment on lines +361 to +365
if let Some(catalog) = self.source_catalog() && !catalog.watermark_descs.is_empty() && !self.core.for_table{
plan = StreamWatermarkFilter::new(plan, catalog.watermark_descs.clone()).into();
}

assert!(!(self.core.gen_row_id && self.core.for_table));
Copy link
Member

Choose a reason for hiding this comment

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

May add some comments here.

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.

None yet

4 participants