feat(storage): iterator support min_epoch check #3542

Li0k · 2022-06-29T03:42:05Z

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

Some change about TTL as title

Please explain IN DETAIL what the changes are in this PR and why they are needed:

~~keyspace adapt table properties with CatalogTable~~
UserIterator add check through min_epoch
ReadOptions add Option and support to calculate min_epoch for iterator

Checklist

I have written necessary docs and comments
I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

feature: support epoch base TTL on hummock #3298

codecov · 2022-06-29T04:28:39Z

Codecov Report

Merging #3542 (e1e27e5) into main (edab3f5) will increase coverage by 0.02%.
The diff coverage is n/a.

❗ Current head e1e27e5 differs from pull request most recent head 1516a71. Consider uploading reports for the commit 1516a71 to get more accurate results

@@            Coverage Diff             @@
##             main    #3542      +/-   ##
==========================================
+ Coverage   74.39%   74.41%   +0.02%     
==========================================
  Files         771      770       -1     
  Lines      108714   108659      -55     
==========================================
- Hits        80875    80858      -17     
+ Misses      27839    27801      -38

Flag	Coverage Δ
rust	`74.41% <0.00%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
src/stream/src/executor/aggregation/row_count.rs	`78.72% <0.00%> (-1.72%)`	⬇️
src/stream/src/executor/top_n_executor.rs	`51.51% <0.00%> (-1.61%)`	⬇️
src/expr/src/expr/template.rs	`68.04% <0.00%> (-0.65%)`	⬇️
...ontend/src/optimizer/plan_node/stream_hash_join.rs	`97.79% <0.00%> (-0.57%)`	⬇️
src/common/src/array/column_proto_readers.rs	`77.96% <0.00%> (-0.37%)`	⬇️
src/common/src/array/mod.rs	`72.17% <0.00%> (-0.09%)`	⬇️
src/sqlparser/src/ast/mod.rs	`89.32% <0.00%> (-0.05%)`	⬇️
src/batch/src/executor/join/hash_join_state.rs	`88.91% <0.00%> (-0.03%)`	⬇️
src/batch/src/executor/join/hash_join.rs	`84.78% <0.00%> (-0.03%)`	⬇️
src/common/src/util/chunk_coalesce.rs	`96.66% <0.00%> (-0.02%)`	⬇️
... and 19 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

BowenXiao1999 · 2022-06-29T10:08:18Z

UserIterator add check through min_epoch

How is it different from previous (with/with out min_epoch)?

BowenXiao1999

Seems like easy to understand. LGTM

hzxa21 · 2022-06-29T10:05:12Z

src/storage/src/hummock/iterator/backward_user.rs

@@ -53,6 +53,8 @@ pub struct BackwardUserIterator {
    /// Only reads values if `epoch <= self.read_epoch`.
    read_epoch: Epoch,

+    min_epoch: Epoch,


Please add some comments for the usage of this field. Should explicitly mentioned that this is only be used for ttl.

hzxa21 · 2022-06-29T10:06:23Z

src/common/src/catalog/mod.rs

+impl From<&TableOption> for risingwave_pb::hummock::TableOption {
+    fn from(table_option: &TableOption) -> Self {
+        Self {
+            ttl: table_option.ttl.unwrap_or(0),


-> unwrap_or(TABLE_OPTION_DUMMY_TTL)

We use 0 as the special value. Do we need to disallow user to set ttl to 0?

its a good idea to disallow user to set ttl to 0 in frontend (but now frontend had not distinguish with_option value type) even if the logic is masked in the storage layer.
conculution: if the ttl be set to 0 , we can cover it in the storage layer

hzxa21 · 2022-06-29T10:11:07Z

src/storage/src/store.rs

+impl ReadOptions {
+    pub fn min_epoch(&self) -> u64 {
+        match self.ttl {
+            Some(ttl_u32) => self.epoch - ttl_u32 as u64,


only the upper 48bit of the epoch represents physical time so you cannot simpy subtract ttl from epoch to construct min_epoch. See epoch.rs for more details.

Li0k · 2022-06-29T10:12:06Z

UserIterator add check through min_epoch

How is it different from previous (with/with out min_epoch)?

At first min_epoch means that user could not access keys which under min_epoch. It use for consistent for ttl. Without min_epoch, user may access the key which under compaction, lead to Inconsistency between two reads

BowenXiao1999 · 2022-06-29T10:14:01Z

UserIterator add check through min_epoch

How is it different from previous (with/with out min_epoch)?

At first min_epoch means that user could not access keys which under min_epoch. It use for consistent for ttl. Without min_epoch, user may access the key which under compaction, lead to Inconsistency between two reads

But I remembered that iter will always need a epoch as parameter? User only care about data in specific epoch

Li0k · 2022-06-29T11:33:50Z

UserIterator add check through min_epoch

How is it different from previous (with/with out min_epoch)?

At first min_epoch means that user could not access keys which under min_epoch. It use for consistent for ttl. Without min_epoch, user may access the key which under compaction, lead to Inconsistency between two reads

But I remembered that iter will always need a epoch as parameter? User only care about data in specific epoch

yes, user only care about the read_epoch. (read_epoch is a upper_bound, min_epoch is a lower_bound when use ttl). an actual scene, user hold a window which width 30min, so that the ttl can be set to 40min. in this case , user never access the key which epoch under min_epoch (read_epoch - ttl), even though the key is the only one in hummock

hzxa21 · 2022-06-30T10:43:09Z

src/common/src/util/epoch.rs

@@ -70,6 +70,16 @@ impl Epoch {
    pub fn as_system_time(&self) -> SystemTime {
        *UNIX_SINGULARITY_DATE_EPOCH + Duration::from_millis(self.physical_time())
    }
+
+    pub fn min_epoch_with_interval(&self, interval: u64) -> Self {


Calling it min_epoch_with_interval is a bit confusing since the Epoch struct should be unaware of the usage of epoch. I suggest rename it to something like subtract_ms(&self, relative_time_ms: u64) and add some docs explaining the usage.

hzxa21 · 2022-06-30T10:45:18Z

src/storage/src/store.rs

+    pub fn min_epoch(&self) -> u64 {
+        let epoch = Epoch(self.epoch);
+        match self.ttl {
+            Some(ttl_u32) => epoch.min_epoch_with_interval(ttl_u32 as u64).0,


ttl_u32 is measure in second? we should convert it to ms to do the epoch subtraction.

unify the unit of ttl to second

fix(storage): fix clippy chore(storage): some unit-test for min_epoch check chore(storage): remove table_option in keyspace

hzxa21

Rest LGTM

hzxa21 · 2022-06-30T12:47:14Z

src/storage/src/hummock/compactor_tests.rs

@@ -514,6 +523,7 @@ mod tests {
            .map(|table_info| table_info.id)
            .collect::<Vec<_>>();

+        // println!("table_ids_from_version {:?}", table_ids_from_version);


remove commented line

fix(storage): unify the unit of ttl to second fix(storage): doc comment fix(storage): use TableOption parse to handle ttl which zero

xxchan · 2022-06-30T16:19:54Z

src/storage/src/hummock/compactor_tests.rs

        for (k, _) in scan_result {
            let table_id = get_table_id(&k).unwrap();
            let epoch = get_epoch(&k);
            assert_eq!(table_id, existing_table_id);
-            assert!(epoch >= (watermark - ttl_expire as u64));
+            assert!(epoch >= min_epoch.0);


main ci failed 🤔 https://buildkite.com/singularity-data/main/builds/598#0181b4ee-420e-4785-9b4d-f8f806f37344/228-1759 @Li0k

* feat(storage): keyspace support table_properties * feat(storage): iterator shadow keys which epoch under min_epoch * feat(storage): TableOption for common * feat(storage): ReadOption add ttl and calculate min_epoch function * chore(storage): adapt some test with ReadOption fix(storage): fix clippy chore(storage): some unit-test for min_epoch check chore(storage): remove table_option in keyspace * fix(storage): fix min_epoch check in compaction_filter and test fix(storage): unify the unit of ttl to second fix(storage): doc comment fix(storage): use TableOption parse to handle ttl which zero

github-actions bot added the type/feature label Jun 29, 2022

Li0k force-pushed the li0k/feat_keyspace_table_properties branch from 6891217 to 68704b9 Compare June 29, 2022 04:16

Li0k force-pushed the li0k/feat_keyspace_table_properties branch from 68704b9 to 8eb6fcb Compare June 29, 2022 07:39

Li0k marked this pull request as ready for review June 29, 2022 07:40

Li0k requested review from hzxa21, Little-Wallace and BowenXiao1999 June 29, 2022 07:40

Li0k changed the title ~~feat(storage): keyspace adapt table properties and iterator support min_epoch check~~ feat(storage): iterator support min_epoch check Jun 29, 2022

Li0k force-pushed the li0k/feat_keyspace_table_properties branch from 8eb6fcb to 565eb54 Compare June 29, 2022 09:57

BowenXiao1999 reviewed Jun 29, 2022

View reviewed changes

hzxa21 reviewed Jun 29, 2022

View reviewed changes

Li0k force-pushed the li0k/feat_keyspace_table_properties branch 2 times, most recently from d8a84d6 to 2ca781e Compare June 30, 2022 02:50

hzxa21 reviewed Jun 30, 2022

View reviewed changes

Li0k added 5 commits June 30, 2022 19:45

feat(storage): keyspace support table_properties

d6c3de2

feat(storage): iterator shadow keys which epoch under min_epoch

2f97d6c

feat(storage): TableOption for common

667c50e

feat(storage): ReadOption add ttl and calculate min_epoch function

d239999

chore(storage): adapt some test with ReadOption

d6759bf

fix(storage): fix clippy chore(storage): some unit-test for min_epoch check chore(storage): remove table_option in keyspace

Li0k force-pushed the li0k/feat_keyspace_table_properties branch 2 times, most recently from 1516a71 to ee00898 Compare June 30, 2022 11:53

hzxa21 approved these changes Jun 30, 2022

View reviewed changes

fix(storage): fix min_epoch check in compaction_filter and test

861773d

fix(storage): unify the unit of ttl to second fix(storage): doc comment fix(storage): use TableOption parse to handle ttl which zero

Li0k force-pushed the li0k/feat_keyspace_table_properties branch from ee00898 to 861773d Compare June 30, 2022 13:52

Li0k added the mergify/can-merge Indicates that the PR can be added to the merge queue label Jun 30, 2022

mergify bot merged commit 2d5c68f into main Jun 30, 2022

mergify bot deleted the li0k/feat_keyspace_table_properties branch June 30, 2022 14:05

xxchan reviewed Jun 30, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage): iterator support min_epoch check #3542

feat(storage): iterator support min_epoch check #3542

Li0k commented Jun 29, 2022 •

edited

codecov bot commented Jun 29, 2022 •

edited

BowenXiao1999 commented Jun 29, 2022

BowenXiao1999 left a comment

hzxa21 Jun 29, 2022

hzxa21 Jun 29, 2022

Li0k Jun 29, 2022

hzxa21 Jun 29, 2022

Li0k Jun 29, 2022 •

edited

Li0k commented Jun 29, 2022

BowenXiao1999 commented Jun 29, 2022 •

edited

Li0k commented Jun 29, 2022

hzxa21 Jun 30, 2022

hzxa21 Jun 30, 2022

Li0k Jun 30, 2022

hzxa21 left a comment

hzxa21 Jun 30, 2022

xxchan Jun 30, 2022 •

edited

feat(storage): iterator support min_epoch check #3542

feat(storage): iterator support min_epoch check #3542

Conversation

Li0k commented Jun 29, 2022 • edited

What's changed and what's your intention?

Checklist

Refer to a related PR or issue link (optional)

codecov bot commented Jun 29, 2022 • edited

Codecov Report

BowenXiao1999 commented Jun 29, 2022

BowenXiao1999 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Li0k Jun 29, 2022 • edited

Choose a reason for hiding this comment

Li0k commented Jun 29, 2022

BowenXiao1999 commented Jun 29, 2022 • edited

Li0k commented Jun 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzxa21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxchan Jun 30, 2022 • edited

Choose a reason for hiding this comment

Li0k commented Jun 29, 2022 •

edited

codecov bot commented Jun 29, 2022 •

edited

Li0k Jun 29, 2022 •

edited

BowenXiao1999 commented Jun 29, 2022 •

edited

xxchan Jun 30, 2022 •

edited