feat(batch): support system column _rw_timestamp for tables #19232

chenzl25 · 2024-11-01T08:30:21Z

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

What's changed and what's your intention?

Resolve Add system column rw_timestamp for any tables #11629
Support system column _rw_timestamp (datatype is timestamptz) for tables. We will add a hidden column _rw_timestamp to every table catalog when loading it from a proto, but we never persist this column info.
Only support selecting _rw_timestamp in a batch query. Using it in a streaming query will cause an error.
For batch batch queries, we add a field epoch_idx to the storage table which indicates the position where we should put the epoch into.
Since the state store get_row interface doesn't expose any epoch information, we will use the iter interface to support point get if _rw_timestamp is selected.
Lots of change is caused by planner tests, the core change is about 200+ LOC.

Example:

dev=> select *, _rw_timestamp from t;
 id | a |         _rw_timestamp
----+---+-------------------------------
  2 | 2 | 2024-11-05 07:05:24.488+00:00
  1 | 1 | 2024-11-05 07:05:19.487+00:00
(2 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 _rw_timestamp system column for tables. Users can use a batch query to select this column to check the internal epoch/timestamp of each row which is useful when you want to know when the rows have been updated recently.

BugenZhao · 2024-11-04T05:37:19Z

src/frontend/src/binder/update.rs

@@ -219,7 +219,8 @@ impl Binder {
            .iter()
            .enumerate()
            .filter_map(|(i, c)| {
-                (!c.is_generated()).then_some(InputRef::new(i, c.data_type().clone()).into())
+                c.can_dml()


Does it mean that we (previously) also allow DML on __row_id column? Shall we ban this as well?

We allow DML like deletes on _row_id column. I think no need to ban it.

BugenZhao · 2024-11-04T05:46:57Z

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

+        // `get_row` doesn't support select `_rw_timestamp` yet.
+        assert!(self.epoch_idx.is_none());


Can we extend the get interface to also return the user key (thus epoch)?

Yes. We do have a plan to add an extended interface (get_with_epoch) to return the epoch from the storage. Let's leave it for a later PR.

BugenZhao · 2024-11-04T05:49:51Z

src/frontend/src/catalog/table_catalog.rs

+        if !columns.contains(&rw_timestamp_column) {
+            // Add system column `_rw_timestamp` to every table, but notice that this column is never persisted.
+            columns.push(rw_timestamp_column);
+        }


This looks a bit hack to me. Since it's never persisted, can we use a separate fields to avoid confusion with real persisted columns?

I don't want to persist this column because we have many already persisted tables in prod. Users won't want to rebuild their tables to use this feature.

Yes, I agree with that. I'm just wondering if it's possible to not have this column in runtime TableCatalog at all, as all other columns are persisted.

I admit it is a bit hacky. If you take a look at how LogicalScan constructs the schema, it uses table catalog columns directly. If we want to keep the table catalog the same as before, we need to add complexity to the table scan and change how it represents the schema, and I think TableScan schema construction is already quite complex. So how about adding an additional field to ColumnDesc to distinguish the system column from others?

So how about adding an additional field to ColumnDesc to distinguish the system column from others

+1

kwannoel · 2024-11-04T06:06:26Z

src/batch/src/executor/row_seq_scan.rs

+            if let Some(timer) = timer {
+                timer.observe_duration()
+            }


I think the timer should be moved to the outer scope, below the if-else block, to capture the time it takes to get a row, whether from get_row or from the iter interface.

kwannoel · 2024-11-04T06:07:02Z

src/batch/src/executor/row_seq_scan.rs

+                .batch_chunk_iter_with_pk_bounds(
+                    epoch.into(),
+                    &pk_prefix,
+                    range_bounds,
+                    false,
+                    1,
+                    PrefetchOptions::new(false, false),
+                )
+                .await?;
+            pin_mut!(iter);
+            let chunk = iter.next().await.transpose().map_err(BatchError::from)?;
+            if let Some(chunk) = chunk {
+                let row = chunk.row_at(0).0.to_owned_row();
+                Ok(Some(row))
+            } else {
+                Ok(None)
+            }


Refactoring this into a separate method like get_row_with_rw_timestamp, and calling it here seems more readable.

st1page

Lots of change is caused by planner tests, the core change is about 200+ LOC.

Why _rw_timestamp must appear in the LogicalScan. Can we prune it in the LogicalScan?

chenzl25 · 2024-11-04T08:50:58Z

Lots of change is caused by planner tests, the core change is about 200+ LOC.

Why _rw_timestamp must appear in the LogicalScan. Can we prune it in the LogicalScan?

After column pruning, this column will be pruned.

st1page · 2024-11-04T09:21:33Z

Lots of change is caused by planner tests, the core change is about 200+ LOC.

Why _rw_timestamp must appear in the LogicalScan. Can we prune it in the LogicalScan?

After column pruning, this column will be pruned.

Ohhh SORRY for misreading

chenzl25 added 2 commits October 31, 2024 23:33

support rw timestamp

cc537c5

add test and add a rw_timestamp validator

f3e8dd0

chenzl25 requested review from fuyufjh and BugenZhao November 1, 2024 08:30

github-actions bot added the type/feature label Nov 1, 2024

chenzl25 requested review from wenym1, st1page, kwannoel and stdrc November 1, 2024 08:30

chenzl25 added the user-facing-changes Contains changes that are visible to users label Nov 1, 2024

chenzl25 added 3 commits November 1, 2024 16:51

fix test

da60d5c

fmt

6b4519f

fmt

a8c256b

graphite-app bot requested a review from a team November 1, 2024 10:16

chenzl25 and others added 8 commits November 1, 2024 23:05

fix delete

a8432b6

fix tests

c9a23a5

fix tests

deb27db

fix tests

cf18cbd

fix test

2fb8312

fmt

e85c372

fix

f095bc7

Merge branch 'main' into dylan/support_rw_timestamp

16b5545

BugenZhao reviewed Nov 4, 2024

View reviewed changes

kwannoel reviewed Nov 4, 2024

View reviewed changes

st1page reviewed Nov 4, 2024

View reviewed changes

chenzl25 added 2 commits November 4, 2024 17:01

fix sink into table

164b431

fmt

f09c9bd

chenzl25 added 10 commits November 4, 2024 17:41

add system column metadata to column desc

42451f5

add docs

920bc01

fix test

6674169

fix test

ea413d5

fix

778b877

fix

4b105f7

fix tests

928f52d

fix

ae3c30c

fix

c47b2fa

fix tests

7f11c65

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(batch): support system column _rw_timestamp for tables #19232

feat(batch): support system column _rw_timestamp for tables #19232

chenzl25 commented Nov 1, 2024 •

edited

Loading

BugenZhao Nov 4, 2024

chenzl25 Nov 4, 2024

BugenZhao Nov 4, 2024

chenzl25 Nov 4, 2024 •

edited

Loading

BugenZhao Nov 4, 2024

chenzl25 Nov 4, 2024

BugenZhao Nov 4, 2024

chenzl25 Nov 4, 2024

st1page Nov 4, 2024 •

edited

Loading

kwannoel Nov 4, 2024

kwannoel Nov 4, 2024 •

edited

Loading

st1page left a comment

chenzl25 commented Nov 4, 2024

st1page commented Nov 4, 2024

		// `get_row` doesn't support select `_rw_timestamp` yet.
		assert!(self.epoch_idx.is_none());

feat(batch): support system column _rw_timestamp for tables #19232

Are you sure you want to change the base?

feat(batch): support system column _rw_timestamp for tables #19232

Conversation

chenzl25 commented Nov 1, 2024 • edited Loading

What's changed and what's your intention?

Example:

Checklist

Documentation

Release note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenzl25 Nov 4, 2024 • edited Loading

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

st1page Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwannoel Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

st1page left a comment

Choose a reason for hiding this comment

chenzl25 commented Nov 4, 2024

st1page commented Nov 4, 2024

chenzl25 commented Nov 1, 2024 •

edited

Loading

chenzl25 Nov 4, 2024 •

edited

Loading

st1page Nov 4, 2024 •

edited

Loading

kwannoel Nov 4, 2024 •

edited

Loading