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

fix(frontend): fix row filter with pk diff order type #19797

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

xxhZs
Copy link
Contributor

@xxhZs xxhZs commented Dec 13, 2024

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

What's changed and what's your intention?

Checklist

  • I have written necessary rustdoc comments.
  • I have added necessary unit tests and integration tests.
  • I have added test labels as necessary.
  • I have added fuzzing tests or opened an issue to track them.
  • My PR contains breaking changes.
  • My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
  • My PR contains critical fixes that are necessary to be merged into the latest release.

Documentation

  • My PR needs documentation updates.
Release note

@github-actions github-actions bot added the type/fix Bug fix label Dec 13, 2024
@xxhZs xxhZs changed the title fix(frontend): fix struct filter with pk diff order type fix(frontend): fix row filter with pk diff order type Dec 16, 2024
@xxhZs xxhZs marked this pull request as ready for review December 16, 2024 04:01
@xxhZs xxhZs requested review from chenzl25, hzxa21 and Li0k December 16, 2024 04:08
Comment on lines +427 to +435
match order_type {
Some(o) => {
if o != column_order.order_type {
all_added = false;
break;
}
}
None => order_type = Some(column_order.order_type),
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the check here will pass if the PKs are all ordered desc:

create materialized view mv1 as select * from t1 order by v1 desc, v2 desc, v3 desc;

However, in this case, I think the optimization is not applicable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We just need to make sure that the order type of all the pk's is the same, in the row_seq_scan_exec will generate the appropriate scan according to the order type of the first pk to scan the data.

let (start_bound, end_bound) = if order_type.is_ascending() {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it. That means when all pks are ordered desc, with the following scan range:

ExprType::GreaterThan => {
    (Bound::Included(pk_struct), Bound::Unbounded)
}

the storage scan will be done by scanning from Bound::Unbounded to Bound::Included(pk_struct)

Copy link
Collaborator

@hzxa21 hzxa21 left a comment

Choose a reason for hiding this comment

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

LGTM

e2e_test/batch/basic/row_filter.slt.part Outdated Show resolved Hide resolved
Comment on lines +427 to +435
match order_type {
Some(o) => {
if o != column_order.order_type {
all_added = false;
break;
}
}
None => order_type = Some(column_order.order_type),
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it. That means when all pks are ordered desc, with the following scan range:

ExprType::GreaterThan => {
    (Bound::Included(pk_struct), Bound::Unbounded)
}

the storage scan will be done by scanning from Bound::Unbounded to Bound::Included(pk_struct)

@xxhZs xxhZs enabled auto-merge December 20, 2024 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants