Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: add assignment cast for UPDATEs #74180

Merged
merged 3 commits into from
Dec 31, 2021

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Dec 21, 2021

opt: fix FK WithScan column type

Previously, the WithScan columns of cascade FK updates were given the
type of the child table's target column. This was incorrect when the
parent column type did not match. This commit fixes the issue by given
the WithScan columns the type of the buffered columns used to update the
parent relation.

This did not cause any known bugs, but it is required to implement
assignment casts with FK cascades.

Release note: None

sql: add assignment cast for UPDATEs

Fixes #70628

Release note: None

opt: give synthesized assignment cast columns descriptive names

Columns synthesized for assignment casts now have more descriptive
metadata names in the form <target_column_name>_cast. This is purely
an aesthetic change and has no effect on semantics.

Release note: None

@mgartner mgartner requested review from RaduBerinde, cucaroach and a team December 21, 2021 23:51
@mgartner mgartner requested a review from a team as a code owner December 21, 2021 23:51
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mgartner mgartner force-pushed the assn-cast-update branch 2 times, most recently from 73ffdb9 to 4cdd42a Compare December 22, 2021 21:00
@mgartner mgartner mentioned this pull request Dec 22, 2021
@mgartner mgartner force-pushed the assn-cast-update branch 2 times, most recently from c090c0e to af62616 Compare December 23, 2021 13:54
Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r1, 22 of 22 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @cucaroach, @mgartner, and @RaduBerinde)


pkg/sql/opt/optbuilder/scope.go, line 320 at r2 (raw file):

}

// getColumn returns the scopeColumn with the given id and reference name

getColumn -> getColumnWithIDAndreferenceName


pkg/sql/logictest/testdata/logic_test/cast, line 384 at r2 (raw file):

EXECUTE update_c('foo'::STRING)

statement

should this be statement ok?


pkg/sql/logictest/testdata/logic_test/cast, line 392 at r2 (raw file):

""

statement

ditto


pkg/sql/logictest/testdata/logic_test/cast, line 409 at r2 (raw file):


statement error \"char\" out of range
UPDATE assn_cast SET qc = 1234

why does this one fail while the previous one succeeds? (maybe add a comment)


pkg/sql/opt/optbuilder/testdata/partial-indexes, line 542 at r2 (raw file):

# Regression test for #61520. The new value for column a should be in-scope when
# building the partial index PUT expression when the value in the FROM clause
# must assignment cast.

nit: must assignment cast -> must be assignment cast ?


pkg/sql/opt/xform/testdata/external/trading-mutation, line 1595 at r2 (raw file):

      │              └── q:37
      └── projections
           ├── assignment-cast: DECIMAL(10,4) [as=column53:53, outer=(23,25), immutable]

can we give the new column a friendly name? (e.g., just keep the name buyprice here)

Previously, the WithScan columns of cascade FK updates were given the
type of the child table's target column. This was incorrect when the
parent column type did not match. This commit fixes the issue by given
the WithScan columns the type of the buffered columns used to update the
parent relation.

This did not cause any known bugs, but it is required to implement
assignment casts with FK cascades.

Release note: None
Columns synthesized for assignment casts now have more descriptive
metadata names in the form `<target_column_name>_cast`. This is purely
an aesthetic change and has no effect on semantics.

Release note: None
Copy link
Collaborator Author

@mgartner mgartner left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @cucaroach, @RaduBerinde, and @rytaft)


pkg/sql/opt/optbuilder/scope.go, line 320 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

getColumn -> getColumnWithIDAndreferenceName

Done.


pkg/sql/logictest/testdata/logic_test/cast, line 384 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

should this be statement ok?

Good catch! Done.


pkg/sql/logictest/testdata/logic_test/cast, line 392 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

ditto

Done.


pkg/sql/logictest/testdata/logic_test/cast, line 409 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

why does this one fail while the previous one succeeds? (maybe add a comment)

Done. integer to "char" casts convert the integer to the corresponding 7-bit ASCII character, so anything greater than 127 is out of range.


pkg/sql/opt/optbuilder/testdata/partial-indexes, line 542 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: must assignment cast -> must be assignment cast ?

Done.


pkg/sql/opt/xform/testdata/external/trading-mutation, line 1595 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

can we give the new column a friendly name? (e.g., just keep the name buyprice here)

I added a new commit which gives them names in the form <target_column_name>_cast.

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 22 of 22 files at r3, 22 of 22 files at r4, 20 of 20 files at r5, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @cucaroach and @RaduBerinde)

@mgartner
Copy link
Collaborator Author

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Dec 30, 2021

Build failed:

@mgartner
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Dec 31, 2021

Build succeeded!

And happy new year! 🎉

@craig craig bot merged commit fa9480a into cockroachdb:master Dec 31, 2021
@mgartner mgartner deleted the assn-cast-update branch December 31, 2021 16:24
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.

sql: non-nullable column with null value
3 participants