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.

Sign up for GitHub

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

Jump to bottom

fix(stream): stream hop window ignore the null time #8146

Merged
merged 2 commits into from
Feb 23, 2023
Merged

fix(stream): stream hop window ignore the null time #8146

merged 2 commits into from
Feb 23, 2023

Conversation

st1page
Copy link
Contributor

@st1page st1page commented Feb 23, 2023 •
edited
Loading

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

What's changed and what's your intention?

fix #8130

Checklist For Contributors

  • I have written necessary rustdoc comments
  • All checks passed in ./risedev check (or alias, ./risedev c)

Checklist For Reviewers

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

@st1page st1page requested review from TennyZhuang and lmatz February 23, 2023 08:20
@github-actions github-actions bot added type/fix Bug fix user-facing-changes Contains changes that are visible to users labels Feb 23, 2023
st1page
st1page commented Feb 23, 2023
View reviewed changes
src/frontend/planner_test/tests/testdata/nexmark_source.yaml
Comment on lines -332 to +335
| └─BatchProject { exprs: [auction, date_time] }
| └─BatchSource { source: "bid", columns: ["auction", "bidder", "price", "channel", "url", "date_time", "extra", "_row_id"], filter: (None, None) }
| └─BatchFilter { predicate: IsNotNull(date_time) }
| └─BatchProject { exprs: [auction, date_time] }
| └─BatchFilter { predicate: IsNotNull(date_time) }
| └─BatchSource { source: "bid", columns: ["auction", "bidder", "price", "channel", "url", "date_time", "extra", "_row_id"], filter: (None, None) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chenzl25 PTAL, is the redundant Filter generated by predicate pushdown across the Share node?

Copy link
Contributor

@chenzl25 chenzl25 Feb 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I will separate the batch and stream logical optimization in the future so that plan-sharing wouldn't affect batch queries.

@codecov
Copy link

codecov bot commented Feb 23, 2023 •
edited
Loading

Codecov Report

Merging #8146 (c5be62d) into main (7cadc39) will increase coverage by 0.46%.
The diff coverage is 60.11%.

@@            Coverage Diff             @@
##             main    #8146      +/-   ##
==========================================
+ Coverage   71.16%   71.62%   +0.46%     
==========================================
  Files        1129     1131       +2     
  Lines      181606   182002     +396     
==========================================
+ Hits       129232   130364    +1132     
+ Misses      52374    51638     -736
Flag Coverage Δ
rust 71.62% <60.11%> (+0.46%) ⬆️

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

Impacted Files Coverage Δ
src/common/src/util/mod.rs 0.00% <ø> (ø)
src/connector/src/parser/avro/schema_resolver.rs 4.00% <0.00%> (-0.06%) ⬇️
src/connector/src/parser/debezium/avro_parser.rs 45.37% <0.00%> (-0.29%) ⬇️
src/connector/src/parser/mod.rs 50.00% <0.00%> (-4.25%) ⬇️
src/connector/src/source/base.rs 60.57% <ø> (ø)
src/connector/src/source/kafka/mod.rs 0.00% <ø> (ø)
src/connector/src/source/kafka/source/message.rs 0.00% <0.00%> (ø)
src/connector/src/source/kafka/source/reader.rs 0.00% <0.00%> (ø)
src/expr/src/expr/expr_array_to_string.rs 0.00% <0.00%> (ø)
src/frontend/src/handler/create_source.rs 44.25% <0.00%> (-9.64%) ⬇️
... and 157 more

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

liurenjie1024
liurenjie1024 approved these changes Feb 23, 2023
View reviewed changes
Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot merged commit b39ae68 into main Feb 23, 2023
@mergify mergify bot deleted the sts/hop_window_ignore_null_time branch February 23, 2023 10:51
lmatz pushed a commit that referenced this pull request Feb 23, 2023
@st1page @lmatz
fix(stream): stream hop window ignore the null time (#8146)
c078de7 
fix #8130

Approved-By: liurenjie1024
lmatz added a commit that referenced this pull request Feb 24, 2023
@lmatz
fix: fix the changes I made by mistake when merging #8146
79767e2
@hengm3467
Copy link
Contributor

@st1page The change here is data records that has null time values will be ignored by time window functions. Correct?

@TennyZhuang
Copy link
Contributor

@st1page The change here is data records that has null time values will be ignored by time window functions. Correct?

Yes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chenzl25 chenzl25 chenzl25 left review comments

@liurenjie1024 liurenjie1024 liurenjie1024 approved these changes

@TennyZhuang TennyZhuang Awaiting requested review from TennyZhuang

@lmatz lmatz Awaiting requested review from lmatz

Assignees
No one assigned
Labels
type/fix Bug fix user-facing-changes Contains changes that are visible to users
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: stream hop window executor should ignore rows with null time value
5 participants
@st1page @hengm3467 @TennyZhuang @liurenjie1024 @chenzl25