Skip to content

Conversation

@spilchen
Copy link
Contributor

Previously, the column compute expression was part of the ColumnType element, which made it difficult to change the compute expression without affecting the other fields in ColumnType. To address this, especially for the upcoming work on adding a temporary compute expression during column type changes that require a backfill (in #127014), we’re moving the compute expression into its own separate element, similar to how default expressions (ColumnDefaultExpression) and ON UPDATE expressions (ColumnOnUpdateExpression) are handled.

Key points to note about this change:

  • Version Gate: The new element must be protected by a version gate. For older versions, we’ll continue using the old compute expression field in ColumnType. There was one instance where the current cluster version wasn’t accessible, so I added a field in ElementCreationMetadata to track if the ColumnType was created in 24.3. This acts as the version gate.
  • IsVirtual Attribute: Although logically, the IsVirtual attribute should belong to ColumnComputeExpression, I left it in ColumnType. During the ADD COLUMN process, we set up the column family for the new column only if it’s not virtual. Moving IsVirtual to ColumnComputeExpression would require a dependency rule to ensure the ColumnComputeExpression is applied before ColumnType to set up the column family correctly.
  • Dropping a Compute Column: When dropping a computed column, I leave a NULL expression in the column descriptor. If the expression is removed entirely, it triggers validation logic that a virtual column must have a compute expression. We will need to revisit this, especially for the ALTER TABLE .. TYPE use case and future support for ALTER TABLE .. DROP EXPRESSION.

Epic: CRDB-25314
Informs: #127014
Release note: None

@spilchen spilchen self-assigned this Aug 13, 2024
@spilchen spilchen requested review from a team as code owners August 13, 2024 14:31
@spilchen spilchen requested review from mgartner and removed request for a team August 13, 2024 14:31
@blathers-crl
Copy link

blathers-crl bot commented Aug 13, 2024

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@fqazi fqazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, just some minor stuff. We also should update scpb.MigrateDescriptorState to convert elements to the new one, after the version gate is active.

Reviewed 104 of 104 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @spilchen)


pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 179 at r1 (raw file):

	if desc.IsComputed() {
		expr := b.WrapExpression(tbl.TableID, b.ComputedColumnExpression(tbl, d))
		if spec.colType.ElementCreationMetadata.In_24_3OrLater {

In the builder I think we can just do: b.EvalCtx().Settings.Version.ActiveVersion()


pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_index.go line 856 at r1 (raw file):

	scpb.ForEachColumnType(elts, func(current scpb.Status, target scpb.TargetStatus, e *scpb.ColumnType) {
		column := mustRetrieveColumnElem(b, e.TableID, e.ColumnID)
		computeExpr := retrieveColumnComputeExpression(b, e.TableID, e.ColumnID)

Nit: Would it be a bit cleaner to always use the new element internally, and convert to the ColumnExpression element

@annrpom
Copy link
Contributor

annrpom commented Aug 14, 2024

We also should update scpb.MigrateDescriptorState to convert elements to the new one, after the version gate is active.

@fqazi Should we also have done this for zone configs in the DSC? Is this needed everytime we modify an existing element?

Copy link
Collaborator

@fqazi fqazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@annrpom I think your safe for zone configs because we didn't support any schema changes with them. This is more about making sure we can re-plan for restore or paused state

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @spilchen)

@annrpom
Copy link
Contributor

annrpom commented Aug 14, 2024

This is more about making sure we can re-plan for restore or paused state

@fqazi ahh ok that makes sense -- ty! (sorry for the PR noise, Matt!)

@spilchen spilchen force-pushed the issue-127014/standalone-compute-expression branch from 5e5f916 to 00a911d Compare August 14, 2024 18:25
Copy link
Contributor Author

@spilchen spilchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look @fqazi. I addressed/replied to your comments.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @fqazi and @mgartner)


pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 179 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

In the builder I think we can just do: b.EvalCtx().Settings.Version.ActiveVersion()

I did try this in an earlier iteration, but I ran into an issue where I couldn't find the current version in one codepath. Specifically, in pkg/sql/schemachanger/scexec/scmutationexec/column.go, I need to check the compute expression in addNewColumnType. Do you have any suggestions on how to get the current active version there? I wasn’t too keen on using ElementCreationMetadata, but it seemed like the only option.


pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_index.go line 856 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Nit: Would it be a bit cleaner to always use the new element internally, and convert to the ColumnExpression element

Good idea.

@spilchen spilchen force-pushed the issue-127014/standalone-compute-expression branch 2 times, most recently from 5006f98 to 8b89b30 Compare August 15, 2024 13:08
Copy link
Contributor Author

@spilchen spilchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fqazi I addressed the final comment you had about migration. PTAL

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @fqazi and @mgartner)

Copy link
Collaborator

@fqazi fqazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great :lgtm_strong:

Reviewed 4 of 4 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)


pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 179 at r1 (raw file):

Previously, spilchen wrote…

I did try this in an earlier iteration, but I ran into an issue where I couldn't find the current version in one codepath. Specifically, in pkg/sql/schemachanger/scexec/scmutationexec/column.go, I need to check the compute expression in addNewColumnType. Do you have any suggestions on how to get the current active version there? I wasn’t too keen on using ElementCreationMetadata, but it seemed like the only option.

Ah this makes, sense. Lets keep the version checks as-is since we need them on the execution side.

…ment

Previously, the column compute expression was part of the ColumnType
element, which made it difficult to change the compute expression
without affecting the other fields in ColumnType. To address this,
especially for the upcoming work on adding a temporary compute
expression during column type changes that require a backfill (in
 cockroachdb#127014), we’re moving the compute expression into its own separate
element, similar to how default expressions (ColumnDefaultExpression)
and ON UPDATE expressions (ColumnOnUpdateExpression) are handled.

Key points to note about this change:
- Version Gate: The new element must be protected by a version gate. For
  older versions, we’ll continue using the old compute expression field
  in ColumnType. There was one instance where the current cluster
  version wasn’t accessible, so I added a field in
  ElementCreationMetadata to track if the ColumnType was created in
  24.3. This acts as the version gate.
- IsVirtual Attribute: Although logically, the IsVirtual attribute
  should belong to ColumnComputeExpression, I left it in ColumnType.
  During the ADD COLUMN process, we set up the column family for the new
  column only if it’s not virtual. Moving IsVirtual to
  ColumnComputeExpression would require a dependency rule to ensure the
  ColumnComputeExpression is applied before ColumnType to set up the
  column family correctly.
- Dropping a Compute Column: When dropping a computed column, I leave a
  NULL expression in the column descriptor. If the expression is removed
  entirely, it triggers validation logic that a virtual column must have
  a compute expression. We will need to revisit this, especially for
  the ALTER TABLE .. TYPE use case and future support for ALTER TABLE ..
  DROP EXPRESSION.

Epic: CRDB-25314
Informs: cockroachdb#127014
Release note: None
@spilchen spilchen force-pushed the issue-127014/standalone-compute-expression branch from 8b89b30 to 51a51d8 Compare August 15, 2024 14:57
@spilchen
Copy link
Contributor Author

TFTR @fqazi

bors r=fqazi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants