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(stream): support non-append-only process time temporal join #16286

Merged
merged 15 commits into from
Apr 18, 2024

Conversation

chenzl25
Copy link
Contributor

@chenzl25 chenzl25 commented Apr 12, 2024

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

What's changed and what's your intention?

  • Resolve Feat: support non-append-only process time temporal join  #15218
  • Support non-append-only process time temporal join which will introduce a state called memo_table to the temporal join.
  • memo_table's pk is join_key + left_pk + right_pk. And its columns are almost the same as the right table.
  • Write pattern:
    for each left input row (with insert op), construct the memo table pk and insert the row.
    Read pattern:
    for each left input row (with delete op), construct pk prefix (join_key + left_pk) to fetch rows.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added test labels as necessary. See details.
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
  • All checks passed in ./risedev check (or alias, ./risedev c)
  • My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)
  • My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

  • My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

  • Support non-append-only temporal join, that is the outer side of the temporal join doesn't require to be append-only anymore.

@BugenZhao
Copy link
Member

This looks good.

Correct me if I'm wrong: we face the exactly same issue with indeterministic UDFs. Is it possible to make the memoization(materialization) into a separate operator? So that we can reuse it for these similar cases.

@TennyZhuang
Copy link
Contributor

we face the exactly same issue with indeterministic UDFs. Is it possible to make the memoization(materialization) into a separate operator

There should be an abstraction for the logic, but IMO it shouldn’t be a separate operator.

@chenzl25
Copy link
Contributor Author

chenzl25 commented Apr 16, 2024

This looks good.

Correct me if I'm wrong: we face the exactly same issue with indeterministic UDFs. Is it possible to make the memoization(materialization) into a separate operator? So that we can reuse it for these similar cases.

Even though indeterministic UDFs also need a memo table, I don't think we can use a shared operator to handle both cases. Because the memo table access pattern and the way how to reconstruct rows from the memo table are totally different.

@BugenZhao
Copy link
Member

Because the memo table access pattern and the way how to reconstruct rows from the memo table are totally different.

You're right. I was solely focused on using temporal join for point-lookup. 😄

@fuyufjh fuyufjh self-requested a review April 17, 2024 02:58
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, maybe we can notice user their will be a memo table state for non-append-only temporal join to prevent silently generating a stream plan with bad performance.(One case I can come up with is #13549)

src/stream/src/executor/temporal_join.rs Outdated Show resolved Hide resolved
// The null rows generated by outer join and the other condition somehow is a stateless operation which means we can handle them without the memo table.
let memo_table = memo_table.as_mut().unwrap();
match op {
Op::Insert | Op::UpdateInsert => {
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to handle the case that a row is updated while the join key is not changed. This can be done in a separate PR, but overall I think it's a must-have enhancement

proto/stream_plan.proto Outdated Show resolved Hide resolved
@chenzl25 chenzl25 enabled auto-merge April 18, 2024 08:58
@chenzl25 chenzl25 added the user-facing-changes Contains changes that are visible to users label Apr 18, 2024
@chenzl25 chenzl25 added this pull request to the merge queue Apr 18, 2024
Merged via the queue into main with commit aa9d4ba Apr 18, 2024
30 of 31 checks passed
@chenzl25 chenzl25 deleted the dylan/support_non_append_only_temporal_join branch April 18, 2024 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature user-facing-changes Contains changes that are visible to users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feat: support non-append-only process time temporal join
5 participants