From 04cb3bfa1649999cef31a7e35ced9f2ce64a2f3e Mon Sep 17 00:00:00 2001 From: James Cor Date: Wed, 17 Nov 2021 15:22:01 -0800 Subject: [PATCH 1/5] prevent ability to add primary key on columns that contain null values; need to figure out how to test --- .../doltcore/schema/alterschema/addpk.go | 36 ++++++++++++ .../doltcore/schema/alterschema/addpk_test.go | 58 +++++++++++++++++++ 2 files changed, 94 insertions(+) diff --git a/go/libraries/doltcore/schema/alterschema/addpk.go b/go/libraries/doltcore/schema/alterschema/addpk.go index 740e8d768b..f8478ac968 100644 --- a/go/libraries/doltcore/schema/alterschema/addpk.go +++ b/go/libraries/doltcore/schema/alterschema/addpk.go @@ -39,6 +39,7 @@ func AddPrimaryKeyToTable(ctx context.Context, table *doltdb.Table, tableName st return nil, sql.ErrMultiplePrimaryKeysDefined.New() // Also caught in GMS } + // Map function for converting columns to a primary key newCollection := schema.MapColCollection(sch.GetAllCols(), func(col schema.Column) schema.Column { for _, c := range columns { @@ -51,6 +52,41 @@ func AddPrimaryKeyToTable(ctx context.Context, table *doltdb.Table, tableName st return col }) + // Get Row Data out of Table + rowData, err := table.GetRowData(ctx) + if err != nil { + return nil, err + } + + // Go through columns of new table + err = newCollection.Iter(func(tag uint64, col schema.Column) (stop bool, err error) { + // Skip if they are not going to be part if primary key + if !col.IsPartOfPK { + return false, nil + } + + // Go through every row + err = rowData.Iter(ctx, func(key, value types.Value) (stop bool, err error) { + r, err := row.FromNoms(sch, key.(types.Tuple), value.(types.Tuple)) + if err != nil { + return false, err + } + val, ok := r.GetColVal(tag) + if !ok || val == nil || val == types.NullValue { + return true, fmt.Errorf("cannot change column to NOT NULL when one or more values is NULL") + } + return false, nil + }) + if err != nil { + return true, err + } + return false, nil + }) + + if err != nil { + return nil, err + } + newSchema, err := schema.SchemaFromCols(newCollection) if err != nil { return nil, err diff --git a/go/libraries/doltcore/schema/alterschema/addpk_test.go b/go/libraries/doltcore/schema/alterschema/addpk_test.go index 669157cb22..02773e7f40 100644 --- a/go/libraries/doltcore/schema/alterschema/addpk_test.go +++ b/go/libraries/doltcore/schema/alterschema/addpk_test.go @@ -144,4 +144,62 @@ func TestAddPk(t *testing.T) { assert.NoError(t, err) assert.True(t, ok) }) + + t.Run("Add primary key when one more cells contain NULL", func(t *testing.T) { + dEnv := dtestutils.CreateTestEnv() + ctx := context.Background() + + for _, c := range setupAdd { + c.exec(t, ctx, dEnv) + } + + _, err := getTable(ctx, dEnv, "test") + assert.NoError(t, err) + + exitCode := commands.SqlCmd{}.Exec(ctx, "sql", []string{"-q", "ALTER TABLE test ADD PRIMARY KEY (c1)"}, dEnv) + require.Equal(t, 0, exitCode) + + exitCode = commands.SqlCmd{}.Exec(ctx, "sql", []string{"-q", "ALTER TABLE test ADD COLUMN (id INT NULL)"}, dEnv) + require.Equal(t, 0, exitCode) + + exitCode = commands.SqlCmd{}.Exec(ctx, "sql", []string{"-q", "ALTER TABLE test DROP PRIMARY KEY"}, dEnv) + require.Equal(t, 0, exitCode) + + exitCode = commands.SqlCmd{}.Exec(ctx, "sql", []string{"-q", "ALTER TABLE test ADD PRIMARY KEY (id)"}, dEnv) + require.Equal(t, 1, exitCode) + + /* + table, err = getTable(ctx, dEnv, "test") + assert.NoError(t, err) + + sch, err := table.GetSchema(ctx) + assert.NoError(t, err) + + // Assert the new index map is not empty + newMap, err := table.GetRowData(ctx) + assert.NoError(t, err) + assert.False(t, newMap.Empty()) + assert.Equal(t, newMap.Len(), uint64(2)) + + // Assert the noms level encoding of the map by ensuring the correct index values are present + kr1, err := createRow(sch, sch.GetAllCols().Tags, []types.Value{types.Int(1), types.Int(1)}) + assert.NoError(t, err) + kr1Key, err := kr1.NomsMapKey(sch).Value(ctx) + assert.NoError(t, err) + + ok, err := newMap.Has(ctx, kr1Key) + assert.NoError(t, err) + assert.True(t, ok) + + kr2, err := createRow(sch, sch.GetAllCols().Tags, []types.Value{types.Int(2), types.Int(2)}) + assert.NoError(t, err) + kr2Key, err := kr2.NomsMapKey(sch).Value(ctx) + assert.NoError(t, err) + + ok, err = newMap.Has(ctx, kr2Key) + assert.NoError(t, err) + assert.True(t, ok) + */ + }) + } From ad937097d791e7072e905270bd924a9f924e07f8 Mon Sep 17 00:00:00 2001 From: James Cor Date: Wed, 17 Nov 2021 16:17:21 -0800 Subject: [PATCH 2/5] added test for failing on null cell value add --- .../doltcore/schema/alterschema/addpk_test.go | 38 +------------------ 1 file changed, 2 insertions(+), 36 deletions(-) diff --git a/go/libraries/doltcore/schema/alterschema/addpk_test.go b/go/libraries/doltcore/schema/alterschema/addpk_test.go index 02773e7f40..857ade9a96 100644 --- a/go/libraries/doltcore/schema/alterschema/addpk_test.go +++ b/go/libraries/doltcore/schema/alterschema/addpk_test.go @@ -159,47 +159,13 @@ func TestAddPk(t *testing.T) { exitCode := commands.SqlCmd{}.Exec(ctx, "sql", []string{"-q", "ALTER TABLE test ADD PRIMARY KEY (c1)"}, dEnv) require.Equal(t, 0, exitCode) - exitCode = commands.SqlCmd{}.Exec(ctx, "sql", []string{"-q", "ALTER TABLE test ADD COLUMN (id INT NULL)"}, dEnv) + exitCode = commands.SqlCmd{}.Exec(ctx, "sql", []string{"-q", "ALTER TABLE test ADD COLUMN (c2 INT NULL)"}, dEnv) require.Equal(t, 0, exitCode) exitCode = commands.SqlCmd{}.Exec(ctx, "sql", []string{"-q", "ALTER TABLE test DROP PRIMARY KEY"}, dEnv) require.Equal(t, 0, exitCode) - exitCode = commands.SqlCmd{}.Exec(ctx, "sql", []string{"-q", "ALTER TABLE test ADD PRIMARY KEY (id)"}, dEnv) + exitCode = commands.SqlCmd{}.Exec(ctx, "sql", []string{"-q", "ALTER TABLE test ADD PRIMARY KEY (id, c1, c2)"}, dEnv) require.Equal(t, 1, exitCode) - - /* - table, err = getTable(ctx, dEnv, "test") - assert.NoError(t, err) - - sch, err := table.GetSchema(ctx) - assert.NoError(t, err) - - // Assert the new index map is not empty - newMap, err := table.GetRowData(ctx) - assert.NoError(t, err) - assert.False(t, newMap.Empty()) - assert.Equal(t, newMap.Len(), uint64(2)) - - // Assert the noms level encoding of the map by ensuring the correct index values are present - kr1, err := createRow(sch, sch.GetAllCols().Tags, []types.Value{types.Int(1), types.Int(1)}) - assert.NoError(t, err) - kr1Key, err := kr1.NomsMapKey(sch).Value(ctx) - assert.NoError(t, err) - - ok, err := newMap.Has(ctx, kr1Key) - assert.NoError(t, err) - assert.True(t, ok) - - kr2, err := createRow(sch, sch.GetAllCols().Tags, []types.Value{types.Int(2), types.Int(2)}) - assert.NoError(t, err) - kr2Key, err := kr2.NomsMapKey(sch).Value(ctx) - assert.NoError(t, err) - - ok, err = newMap.Has(ctx, kr2Key) - assert.NoError(t, err) - assert.True(t, ok) - */ }) - } From fe98469b5cecbdefb8fb6f77f8343ca947a21462 Mon Sep 17 00:00:00 2001 From: JCOR11599 Date: Thu, 18 Nov 2021 00:27:52 +0000 Subject: [PATCH 3/5] [ga-format-pr] Run go/utils/repofmt/format_repo.sh and go/Godeps/update.sh --- go/libraries/doltcore/schema/alterschema/addpk.go | 1 - 1 file changed, 1 deletion(-) diff --git a/go/libraries/doltcore/schema/alterschema/addpk.go b/go/libraries/doltcore/schema/alterschema/addpk.go index f8478ac968..a21a9c131c 100644 --- a/go/libraries/doltcore/schema/alterschema/addpk.go +++ b/go/libraries/doltcore/schema/alterschema/addpk.go @@ -39,7 +39,6 @@ func AddPrimaryKeyToTable(ctx context.Context, table *doltdb.Table, tableName st return nil, sql.ErrMultiplePrimaryKeysDefined.New() // Also caught in GMS } - // Map function for converting columns to a primary key newCollection := schema.MapColCollection(sch.GetAllCols(), func(col schema.Column) schema.Column { for _, c := range columns { From 99321efb02ec97ce19dfa82ffbaba5c1eac70521 Mon Sep 17 00:00:00 2001 From: James Cor Date: Thu, 18 Nov 2021 09:56:22 -0800 Subject: [PATCH 4/5] added bats test --- .gitignore | 2 ++ .../doltcore/schema/alterschema/addpk.go | 17 +++++++++-------- integration-tests/bats/primary-key-changes.bats | 13 +++++++------ 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/.gitignore b/.gitignore index 1328b5cfd6..28e71e2a0f 100644 --- a/.gitignore +++ b/.gitignore @@ -9,3 +9,5 @@ venv benchmark/perf_tools/dolt-builds/dolt benchmark/perf_tools/dolt-builds/working benchmark/perf_tools/output + +test.sh \ No newline at end of file diff --git a/go/libraries/doltcore/schema/alterschema/addpk.go b/go/libraries/doltcore/schema/alterschema/addpk.go index f8478ac968..014d80d10a 100644 --- a/go/libraries/doltcore/schema/alterschema/addpk.go +++ b/go/libraries/doltcore/schema/alterschema/addpk.go @@ -52,28 +52,29 @@ func AddPrimaryKeyToTable(ctx context.Context, table *doltdb.Table, tableName st return col }) - // Get Row Data out of Table - rowData, err := table.GetRowData(ctx) - if err != nil { - return nil, err - } - // Go through columns of new table err = newCollection.Iter(func(tag uint64, col schema.Column) (stop bool, err error) { - // Skip if they are not going to be part if primary key + // Skip if they are not part of primary key if !col.IsPartOfPK { return false, nil } + // Get Row Data out of Table + rowData, err := table.GetRowData(ctx) + if err != nil { + return true, err + } + // Go through every row err = rowData.Iter(ctx, func(key, value types.Value) (stop bool, err error) { r, err := row.FromNoms(sch, key.(types.Tuple), value.(types.Tuple)) if err != nil { return false, err } + // Check if column value is null val, ok := r.GetColVal(tag) if !ok || val == nil || val == types.NullValue { - return true, fmt.Errorf("cannot change column to NOT NULL when one or more values is NULL") + return true, fmt.Errorf("primary key cannot have NULL values") } return false, nil }) diff --git a/integration-tests/bats/primary-key-changes.bats b/integration-tests/bats/primary-key-changes.bats index f8bce650f5..ca0db7cf06 100644 --- a/integration-tests/bats/primary-key-changes.bats +++ b/integration-tests/bats/primary-key-changes.bats @@ -10,6 +10,13 @@ teardown() { teardown_common } +@test "primary-key-changes: add primary key using null values" { + dolt sql -q "create table t(pk int, val int)" + dolt sql -q "INSERT INTO t (val) VALUES (1)" + run dolt sql -q "ALTER TABLE t ADD PRIMARY KEY (pk)" + [ "$status" -eq 1 ] +} + @test "primary-key-changes: add single primary key" { dolt sql -q "create table t(pk int, val int)" run dolt sql -q "ALTER TABLE t ADD PRIMARY KEY (pk)" @@ -573,11 +580,5 @@ SQL dolt sql -q "create table t (pk int, c1 int)" dolt sql -q "insert into t values (NULL, NULL)" run dolt sql -q "alter table t add primary key(pk)" - skip "This should fail on some sort of constraint error" [ $status -eq 1 ] - - # This is the current failure mode - run dolt sql -q "update t set c1=1" - [ $status -eq 1 ] - [[ "$output" =~ "received nil" ]] || false } From f517fc20e0f98ec6e85e9f0f72c5d2dda84832b7 Mon Sep 17 00:00:00 2001 From: James Cor Date: Thu, 18 Nov 2021 10:53:39 -0800 Subject: [PATCH 5/5] reversed order of iteration for rows and columns --- .../doltcore/schema/alterschema/addpk.go | 31 ++++++++++--------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/go/libraries/doltcore/schema/alterschema/addpk.go b/go/libraries/doltcore/schema/alterschema/addpk.go index d2063cecee..7fd2a8db55 100644 --- a/go/libraries/doltcore/schema/alterschema/addpk.go +++ b/go/libraries/doltcore/schema/alterschema/addpk.go @@ -51,25 +51,26 @@ func AddPrimaryKeyToTable(ctx context.Context, table *doltdb.Table, tableName st return col }) - // Go through columns of new table - err = newCollection.Iter(func(tag uint64, col schema.Column) (stop bool, err error) { - // Skip if they are not part of primary key - if !col.IsPartOfPK { - return false, nil - } + // Get Row Data out of Table + rowData, err := table.GetRowData(ctx) + if err != nil { + return nil, err + } - // Get Row Data out of Table - rowData, err := table.GetRowData(ctx) + // Go through every row + err = rowData.Iter(ctx, func(key, value types.Value) (stop bool, err error) { + r, err := row.FromNoms(sch, key.(types.Tuple), value.(types.Tuple)) if err != nil { - return true, err + return false, err } - // Go through every row - err = rowData.Iter(ctx, func(key, value types.Value) (stop bool, err error) { - r, err := row.FromNoms(sch, key.(types.Tuple), value.(types.Tuple)) - if err != nil { - return false, err + // Go through every column of row + err = newCollection.Iter(func(tag uint64, col schema.Column) (stop bool, err error) { + // Skip if they are not part of primary key + if !col.IsPartOfPK { + return false, nil } + // Check if column value is null val, ok := r.GetColVal(tag) if !ok || val == nil || val == types.NullValue { @@ -77,9 +78,11 @@ func AddPrimaryKeyToTable(ctx context.Context, table *doltdb.Table, tableName st } return false, nil }) + if err != nil { return true, err } + return false, nil })