adapter: Refactor `RelationDesc` #29231

ParkMyCar · 2024-08-26T21:16:01Z

This PR refactors RelationDesc to support adding columns to tables, it does a few things:

Creates a RelationDescBuilder and moves a few methods, namely with_column(...) onto the new builder. The goal here was to disambiguate adding a column to a new version of the RelationDesc and the initial construction of a RelationDesc.
Refactors the internals of RelationDesc from a Vec<ColumnName> to a BTreeMap<ColumnIndex, ColumnMetadata>. There are two primary things this enables:
1. Stable tracking of column positions with ColumnIndex
2. ColumnMetadata tracks the version at which a column was added and optionally dropped.

Motivation

Progress towards #28082

Tips for reviewer

These changes are split into two separate commits:

Adding RelationDescBuilder, this is more or less code movement.
Refactoring the internals of RelationDesc. This shouldn't cause any behavior changes but is a decent amount of changes.

Checklist

This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

shepherdlybot · 2024-08-26T21:38:32Z

Mitigations

Completing required mitigations increases Resilience Coverage.

(Required) Code Review 🔍 Detected
(Required) Feature Flag
(Required) Integration Test 🔍 Detected
(Required) Observability
(Required) QA Review
(Required) Run Nightly Tests
Unit Test 🔍 Detected

Risk Summary:

The pull request has a high risk score of 81, primarily driven by the predictors "Sum Bug Reports Of Files" and "Delta of Executable Lines," along with modifications in one file hotspot. Historically, PRs with these predictors are 109% more likely to cause a bug than the repository baseline. Notably, the observed bug trend in the repository is decreasing.

Note: The risk score is not based on semantic analysis but on historical predictors of bug occurrence in the repository. The attributes above were deemed the strongest predictors based on that history. Predictors and the score may change as the PR evolves in code, time, and review activity.

Bug Hotspots:
What's This?

File	Percentile
../durable/persist.rs	98

ParkMyCar · 2024-08-26T21:38:55Z

src/repr/src/relation.rs

+    /// Index into a [`RelationType`] for this column.
+    typ_idx: usize,


I wanted to put ColumnType here instead of an index, but RelationType is passed around so much in the compute code that this would be a large refactor

ParkMyCar · 2024-08-26T21:40:38Z

src/repr/src/relation.rs

 /// Diffs that can be generated proptest and applied to a [`RelationDesc`] to
 /// exercise schema migrations.
 #[derive(Debug)]
 pub enum PropRelationDescDiff {
    AddColumn { name: ColumnName, typ: ColumnType },
    DropColumn { name: ColumnName },
-    MovePosition { name: ColumnName, new_pos: usize },


This is only test code, after this change we can't really move the position of a column

ParkMyCar · 2024-08-27T14:36:48Z

src/catalog/src/builtin.rs

-        .with_column("region", ScalarType::String.nullable(true))
-        .with_column("access_key_id", ScalarType::String.nullable(true))
-        .with_column("access_key_id_secret_id", ScalarType::String.nullable(true))
-        .with_column(
-            "secret_access_key_secret_id",
-            ScalarType::String.nullable(true),
-        )
-        .with_column("session_token", ScalarType::String.nullable(true))
-        .with_column("session_token_secret_id", ScalarType::String.nullable(true))
-        .with_column("assume_role_arn", ScalarType::String.nullable(true))
-        .with_column(
-            "assume_role_session_name",
-            ScalarType::String.nullable(true),
-        )


note: I didn't mean to delete these columns, fixed this in a later commit

jkosh44 · 2024-08-27T14:45:49Z

src/repr/src/row/encoding2.rs

+                        (ColumnName::from("foo"), ScalarType::Int64.nullable(true)),
+                        (ColumnName::from("bar"), ScalarType::String.nullable(false)),


Did you mean to swap the nullability of the columns here?

Argh I did not, sorry about the miss, fixed!

jkosh44 · 2024-08-27T15:04:53Z

src/repr/src/relation.rs

+    /// Drops the column `name` from this [`RelationDesc`]. If there are multiple columns with
+    /// `name` drops the left-most one.


If there are multiple columns with
/// name drops the left-most one.

Is it possible to have multiple columns with the same name?

It is something we want to support, e.g.

CREATE TABLE t1 (a int); ALTER TABLE t1 DROP COLUMN a; ALTER TABLE t1 ADD COLUMN a text;

We would keep the original a int as ColumnIndex(0) and have a text as ColumnIndex(1)

In that case if I tried to do something like

CREATE TABLE t1 (a int); ALTER TABLE t1 DROP COLUMN a; ALTER TABLE t1 ADD COLUMN a text; ALTER TABLE ts DROP COLUMN a;

then the second drop would panic on line 988?

Maybe the description should read something like

If there are multiple columns with name drops the left-most non-dropped one.

and line 984 should be something like .find(|meta| meta.name == name && meta.dropped.is_none())

src/repr/src/relation.rs

jkosh44 · 2024-08-27T15:10:24Z

src/repr/src/relation.rs

+        // N.B. At this point we need to be careful because col_idx might not
+        // equal typ_idx.


Why's that? I don't think I understood how col_idx and typ_idx could diverge.

Oh because you're not retaining dropped columns in column_metas but they are still influencing col_idx and not typ_idx?

Oh because you're not retaining dropped columns in column_metas but they are still influencing col_idx and not typ_idx?

Exactly! Ideally we would track ColumnType in the BTreeMap<...> but RelationType is used so widely in compute that it's a big refactor. I added an example in the comment and this is exercised in the snapshot tests below, let me know if I should add more!

jkosh44 · 2024-08-27T15:13:32Z

src/repr/src/relation.rs

+
+        let relation_type = RelationType {
+            column_types,
+            keys: self.inner.typ.keys.clone(),


What happens if keys reference a dropped column or a column that was added after version?

Good callout! I don't think we'll allow dropping a column that is a key, at least not v0, so I made that panic. I didn't consider the case of keys getting re-ordered though so thank you for calling that out, I fixed this and added a test case

jkosh44 · 2024-08-27T15:18:05Z

src/repr/src/relation.rs

+    ///
+    /// Panics if a constraint is not satisfied.
+    fn validate(&self) {
+        fn validate_inner(desc: &RelationDesc) -> Result<(), anyhow::Error> {


Are there any validations to add on desc.typ.keys? Maybe that each index is valid and that a single key doesn't have duplicate indexes.

Good call, added some validation

bkirwi · 2024-08-27T17:27:49Z

src/repr/src/relation.rs

+
+/// Metadata (other than type) for a column in a [`RelationDesc`].
+#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize, Hash, MzReflect)]
+struct ColumnMetadata {


Questions about the data model:

When do we set ColumnMetadata::dropped, and when do we remove the entry from the map entirely?

At what point do we expect the caller to call version_at? Does that always happen by the time it's passed to Persist?

(Sorry if these are redundant; I may be missing context!)

Great questions!

When do we set ColumnMetadata::dropped

We set ColumnMetadata::dropped when a user runs ALTER TABLE ... DROP COLUMN .... We aren't planning to support this immediately but I figured while I was here I would add support.

...when do we remove the entry from the map entirely?

We won't ever drop the column entirely, but if a user runs at_version(...) with a version where the column doesn't exist, it won't be included in the returned RelationDescs map.

At what point do we expect the caller to call version_at? Does that always happen by the time it's passed to Persist?

This should get called during planning! Persist should never have to worry about calling at_version(...). In fact, I put at_version(...) on a newtype so it shouldn't ever be possible to call at_version(...) outside of planning

bkirwi · 2024-08-27T17:29:57Z

src/repr/src/relation.rs

+        N: Into<ColumnName>,
+        T: Into<ColumnType>,
+    {
+        let latest_version = self.latest_version();


This makes the cost linear in the number of existing fields. (So adding N fields is quadratic.) Not sure if we care?

Yeah we could cache the latest version, but this should only get called when a user runs ALTER TABLE ... so I'm not concerned about the runtime here

jkosh44

LGTM

jkosh44 · 2024-08-27T20:48:34Z

src/repr/src/relation.rs

+    /// Drops the column `name` from this [`RelationDesc`]. If there are multiple columns with
+    /// `name` drops the left-most one.


In that case if I tried to do something like

CREATE TABLE t1 (a int); ALTER TABLE t1 DROP COLUMN a; ALTER TABLE t1 ADD COLUMN a text; ALTER TABLE ts DROP COLUMN a;

then the second drop would panic on line 988?

Maybe the description should read something like

If there are multiple columns with name drops the left-most non-dropped one.

and line 984 should be something like .find(|meta| meta.name == name && meta.dropped.is_none())

* Add RelationDescBuilder that is used to construct a RelationDesc * Remove the RelationDesc::with_column method

ParkMyCar · 2024-08-29T13:45:09Z

@jkosh44 good call with the duplicate columns, it was pretty easy to support that. Updated the comments and added tests!

* refactors RelationDesc to internally have a BTreeMap<ColumnIndex, ColumnMetadata> * track added and dropped versions in ColumnMetadata

…lationDesc * Remove the RelationDesc::with_column method

…with version from before we added the metadata field

* Update comments around handling of columns with the same name * Update handling of keys, make sure we remap keys to handle dropped columns

ParkMyCar force-pushed the adapter/refactor-relation-desc branch from c112553 to 9e20857 Compare August 26, 2024 21:28

ParkMyCar marked this pull request as ready for review August 26, 2024 21:37

ParkMyCar requested review from a team as code owners August 26, 2024 21:37

ParkMyCar requested a review from jkosh44 August 26, 2024 21:37

ParkMyCar commented Aug 26, 2024

View reviewed changes

ParkMyCar commented Aug 27, 2024

View reviewed changes

jkosh44 reviewed Aug 27, 2024

View reviewed changes

bkirwi reviewed Aug 27, 2024

View reviewed changes

jkosh44 approved these changes Aug 27, 2024

View reviewed changes

start

f493aeb

* Add RelationDescBuilder that is used to construct a RelationDesc * Remove the RelationDesc::with_column method

ParkMyCar force-pushed the adapter/refactor-relation-desc branch from fef33bb to c8a5976 Compare August 29, 2024 13:42

ParkMyCar enabled auto-merge (squash) August 29, 2024 13:43

ParkMyCar added 9 commits August 29, 2024 10:22

fix issue after rebase

4a7333d

start, refactor RelationDesc

c57f97c

* refactors RelationDesc to internally have a BTreeMap<ColumnIndex, ColumnMetadata> * track added and dropped versions in ColumnMetadata

doc fix

25044f3

fixup! start * Add RelationDescBuilder that is used to construct a Re…

10795de

…lationDesc * Remove the RelationDesc::with_column method

* make sure the serialization format of ProtoRelationDesc roundtrips …

efacb6f

…with version from before we added the metadata field

feedback from GitHub

073bfca

* Update comments around handling of columns with the same name * Update handling of keys, make sure we remap keys to handle dropped columns

* add validation for keys

8be270d

handle dropping then adding a column of the same name

3cf0715

update comment

92d8e6a

ParkMyCar force-pushed the adapter/refactor-relation-desc branch from 287fbb5 to 92d8e6a Compare August 29, 2024 14:23

ParkMyCar merged commit ee73b77 into MaterializeInc:main Aug 29, 2024
83 checks passed

github-actions bot locked and limited conversation to collaborators Aug 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adapter: Refactor `RelationDesc` #29231

adapter: Refactor `RelationDesc` #29231

ParkMyCar commented Aug 26, 2024 •

edited

Loading

shepherdlybot bot commented Aug 26, 2024 •

edited

Loading

ParkMyCar Aug 26, 2024

ParkMyCar Aug 26, 2024

ParkMyCar Aug 27, 2024

jkosh44 Aug 27, 2024

ParkMyCar Aug 27, 2024

jkosh44 Aug 27, 2024

ParkMyCar Aug 27, 2024

jkosh44 Aug 27, 2024

jkosh44 Aug 27, 2024

jkosh44 Aug 27, 2024

ParkMyCar Aug 27, 2024

jkosh44 Aug 27, 2024

ParkMyCar Aug 27, 2024

jkosh44 Aug 27, 2024

ParkMyCar Aug 27, 2024

bkirwi Aug 27, 2024

bkirwi Aug 27, 2024

ParkMyCar Aug 27, 2024

bkirwi Aug 27, 2024

ParkMyCar Aug 27, 2024

jkosh44 left a comment

jkosh44 Aug 27, 2024

ParkMyCar commented Aug 29, 2024

		/// Index into a [`RelationType`] for this column.
		typ_idx: usize,

		(ColumnName::from("foo"), ScalarType::Int64.nullable(true)),
		(ColumnName::from("bar"), ScalarType::String.nullable(false)),

		/// Drops the column `name` from this [`RelationDesc`]. If there are multiple columns with
		/// `name` drops the left-most one.

		// N.B. At this point we need to be careful because col_idx might not
		// equal typ_idx.

adapter: Refactor RelationDesc #29231

adapter: Refactor RelationDesc #29231

Conversation

ParkMyCar commented Aug 26, 2024 • edited Loading

Motivation

Tips for reviewer

Checklist

shepherdlybot bot commented Aug 26, 2024 • edited Loading

Mitigations

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

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

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

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

jkosh44 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParkMyCar commented Aug 29, 2024

adapter: Refactor `RelationDesc` #29231

adapter: Refactor `RelationDesc` #29231

ParkMyCar commented Aug 26, 2024 •

edited

Loading

shepherdlybot bot commented Aug 26, 2024 •

edited

Loading