fix(compaction): fix table_id mask #18325
Conversation
…nto li0k/storage_fix_table_id_mask
…nto li0k/storage_fix_table_id_mask
| @@ -396,13 +395,14 @@ async fn test_failpoints_compactor_iterator_recreate() { | |||
| .await | |||
| .unwrap(); | |||
|
|
|||
| info.table_ids.push(table_id as u32); | |||
There was a problem hiding this comment.
why not make the put_sst implementation populae the table_ids field correctly?
| stats: &StoreLocalStatistic, | ||
| task_progress: Arc<TaskProgress>, | ||
| sstable_store: SstableStoreRef, | ||
| max_io_retry_times: usize, | ||
| ) -> Self { | ||
| let existing_table_ids = HashSet::from_iter(sstable_info.table_ids.iter().cloned()); |
There was a problem hiding this comment.
nits: let's name it sst_table_ids to make things more explicit. People familiar with the previous codes can easily be confused and think existing_table_ids is the table ids of the compaction group.
| sstable: TableHolder, | ||
| iter: Option<BlockIterator>, | ||
| task_progress: Arc<TaskProgress>, | ||
|
|
||
| existing_table_ids: HashSet<StateTableId>, |
| @@ -234,6 +271,7 @@ impl ConcatSstableIterator { | |||
| self.cur_idx = idx; | |||
| if self.cur_idx < self.sstables.len() { | |||
| let sstable_info = &self.sstables[self.cur_idx]; | |||
| let existing_table_ids = HashSet::from_iter(sstable_info.table_ids.iter().cloned()); | |||
| @@ -158,6 +159,18 @@ impl BlockDataStream { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| pub fn block_index(&self) -> usize { | |||
There was a problem hiding this comment.
Why do we need this method? This is exactly the same as next_block_index
| Some((data, _uncompressed_size)) => { | ||
| block_meta = self | ||
| .block_stream | ||
| .block_meta(self.block_stream.block_index()) |
There was a problem hiding this comment.
Are you sure this correct? Based on the BlockDataStream implementation, block_index is pointing to the next block not the returning block of next_block_impl
There was a problem hiding this comment.
I think the logic can be easily messed up by using tbe wrong block idx. Please add UTs to make sure things are working as expected.
| let filter_block = self | ||
| .sstable | ||
| .filter_reader | ||
| .get_block_raw_filter(self.next_block_index); | ||
| self.next_block_index += 1; | ||
| .get_block_raw_filter(self.block_stream.block_index()); |
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
After github.com//pull/18053,SstableInfo contains "correct" table_ids. Iterator can use SstableInfo's table_ids to mask data access.
This PR fix 2 path:
existing_table_ids(which generate with catalog) withSstableInfo.table_idsSstableInfo.table_idsrelated to #18323
Checklist
./risedev check(or alias,./risedev c)Documentation
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.