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

bug: select a mv panic #16219

Open
chenzl25 opened this issue Apr 9, 2024 · 2 comments · May be fixed by #20138
Open

bug: select a mv panic #16219

chenzl25 opened this issue Apr 9, 2024 · 2 comments · May be fixed by #20138
Assignees
@hzxa21
Labels
type/bug Something isn't working
Milestone
release-2.2

Comments

@chenzl25
Copy link
Contributor

chenzl25 commented Apr 9, 2024

Describe the bug

create table t (a varchar);
create materialized view mv as select * from t order by a desc;
select * from mv where a < ''; -- panic

Error message/log

No response

To Reproduce

No response

Expected behavior

No response

How did you deploy RisingWave?

No response

The version of RisingWave

No response

Additional context

No response
@chenzl25 chenzl25 added the type/bug Something isn't working label Apr 9, 2024
@github-actions github-actions bot added this to the release-1.9 milestone Apr 9, 2024
@hzxa21
Copy link
Collaborator

hzxa21 commented Apr 10, 2024

Per offline discussion, this is due to an overflow issue in

risingwave/src/storage/src/table/batch_table/storage_table.rs

Lines 548 to 562 in c49bc37

Excluded(k) => {
let pk_prefix_serializer = pk_serializer.prefix(pk_prefix.len() + k.len());
let key = pk_prefix.chain(k);
let serialized_key = serialize_pk(&key, &pk_prefix_serializer);
if is_start_bound {
// Storage doesn't support excluded begin key yet, so transform it to
// included.
// We always serialize a u8 for null of datum which is not equal to '\xff',
// so we can assert that the next_key would never be empty.
let next_serialized_key = next_key(&serialized_key);
assert!(!next_serialized_key.is_empty());
Included(Bytes::from(next_serialized_key))
} else {
Excluded(serialized_key)
}

The reason why storage table needs to convert exclusive start key to an inclusive key with next_key is because user key iterator doesn't support excluded start key . I think the reason why we didn't support excluded start key is just because the 1st version of user key iterator written 3yrs ago (how time flies!) only supports inclusive start key and we follow this assumption in the development.

In general, I think calculating next_key is risky so I think we can relax the inclusive start key requirement in user key iterator and support exclusive start key. The implementation can be straight-forward: for exclusive start key, we use start_key | MIN_EPOCH to construct the full key to seek and other logics remain unchanged.

@fuyufjh fuyufjh modified the milestones: release-1.9, release-1.10 May 14, 2024
@hzxa21 hzxa21 assigned Little-Wallace and unassigned hzxa21 May 30, 2024
@hzxa21
Copy link
Collaborator

hzxa21 commented May 30, 2024

Related fix: #17018

@hzxa21 hzxa21 assigned hzxa21 and unassigned Little-Wallace Jul 10, 2024
@hzxa21 hzxa21 modified the milestones: release-1.10, release-1.11 Jul 10, 2024
@hzxa21 hzxa21 modified the milestones: release-2.0, future-release-2.2 Oct 8, 2024
@hzxa21 hzxa21 linked a pull request Jan 13, 2025 that will close this issue
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hzxa21 hzxa21

Labels
type/bug Something isn't working
Projects
None yet
Milestone
release-2.2
Development

Successfully merging a pull request may close this issue.

fix(storage): support excluded bound in storage table risingwavelabs/risingwave
4 participants
@hzxa21 @chenzl25 @fuyufjh @Little-Wallace