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

Add Support for Check Constraint - Backend code changes #945

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

taherkl
Copy link

@taherkl taherkl commented Nov 27, 2024

Backend Support for Check Constraint

Fixes #<issue_number_goes_here>

It's a good idea to open an issue first for discussion.

  • Tests pass
  • Appropriate changes to README are included in PR

Backend code changes to support CHECK CONSTRAINTS

  1. Added DB Collation regex on the constants
  2. Added helper function for generating Check constraints ID
  3. Updated Conv schema to support Check Constraints
  4. CRUD operation for check constraints expressions to Spanner DDL
  5. Added regex pattern validation to check constraints name, table name and database collation

Backend Support for Check Constraint
@taherkl taherkl changed the title Check constraint backend (#9) Add Support for Check Constraint - Backend code changes Nov 27, 2024
sources/common/toddl.go Outdated Show resolved Hide resolved
spanner/ddl/ast.go Outdated Show resolved Hide resolved
spanner/ddl/ast_test.go Outdated Show resolved Hide resolved
webv2/api/schema.go Outdated Show resolved Hide resolved
webv2/api/schema.go Outdated Show resolved Hide resolved
oldName := conv.SrcSchema[tableId].ColDefs[colId].Name

for i := range conv.SpSchema[tableId].CheckConstraints {
originalString := conv.SpSchema[tableId].CheckConstraints[i].Expr
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if there were two columns id_name and id. id gets updated to id2, this code will cause errors

Choose a reason for hiding this comment

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

added regular expression to match the exact column name.

Copy link
Collaborator

Choose a reason for hiding this comment

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

please add test cases for this flow.

@asthamohta
Copy link
Collaborator

There are two things missing in this PR:

  1. Either a separate PR or in the same PR we need ot make setting instance and project name mandatory on the page before schema migration.
  2. The validation call is not called when prepare migration is clicked.

internal/reports/report_helpers.go Show resolved Hide resolved
// getQuery returns the appropriate SQL query based on the existence of CHECK_CONSTRAINTS.
func (isi InfoSchemaImpl) getQuery(tableExists bool) string {
if tableExists {
return `SELECT k.COLUMN_NAME, t.CONSTRAINT_TYPE, COALESCE(c.CHECK_CLAUSE, '') AS CHECK_CLAUSE
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need this fork in the logic here ? The query would return empty in case the table does not exist.

Choose a reason for hiding this comment

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

we tried multiple iterations of the query, it was failing on the older versions

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a comment - which versions of MySQL go in which flow ? This is not clear from the code.

sources/mysql/infoschema.go Outdated Show resolved Hide resolved
sources/mysql/infoschema.go Outdated Show resolved Hide resolved
spanner/ddl/ast.go Outdated Show resolved Hide resolved
spanner/ddl/ast.go Outdated Show resolved Hide resolved
webv2/api/schema.go Show resolved Hide resolved
@bharadwaj-aditya
Copy link
Collaborator

Can you please update the PR description with what is being done, and make it it a regular PR from draft ?

switch constraintType {
case "PRIMARY KEY":
*primaryKeys = append(*primaryKeys, col)
case "CHECK":
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you please use the full name here ? "CHECK" is not very clear.

@@ -103,14 +103,16 @@ var PGSQL_TO_STANDARD_TYPE_TYPEMAP = map[string]string{

// PGDialect keyword list
// Assumption is that this list PGSQL dialect uses the same keywords
var PGSQL_RESERVED_KEYWORD_LIST = []string{"ALL", "ANALYSE", "ANALYZE", "AND", "ANY", "ARRAY", "AS", "ASC", "ASYMMETRIC", "AUTHORIZATION", "BETWEEN", "BIGINT", "BINARY", "BIT", "BOOLEAN", "BOTH", "CASE", "CAST",
var PGSQL_RESERVED_KEYWORD_LIST = []string{
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like some whitespaces are added here, can you revert this ?


func isCheckConstraintsNameExist(spcks []ddl.CheckConstraint, targetName string) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: doesCheckConstraintNameExist

oldName := conv.SrcSchema[tableId].ColDefs[colId].Name

for i := range conv.SpSchema[tableId].CheckConstraints {
originalString := conv.SpSchema[tableId].CheckConstraints[i].Expr
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add test cases for this flow.

@taherkl taherkl marked this pull request as ready for review December 10, 2024 11:14
@taherkl taherkl requested a review from a team as a code owner December 10, 2024 11:14
@taherkl taherkl requested review from asthamohta and VardhanThigle and removed request for a team December 10, 2024 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants