Merge pull request #3109 from dolthub/elianddb/9544-fix-time-fk-constraints

elianddb · web-flow · commit 76b20d51836f · 2025-07-25T15:37:52.000-07:00
dolthub/dolt#9544 - Fix time type foreign key constraints
diff --git a/enginetest/queries/script_queries.go b/enginetest/queries/script_queries.go
@@ -11088,81 +11088,68 @@ where
 		},
 		Assertions: []ScriptTestAssertion{
 			{
-				Skip:  true,
 				Query: "create table child_datetime0 (dt datetime, foreign key (dt) references parent_datetime6(dt));",
 				Expected: []sql.Row{
 					{types.NewOkResult(0)},
 				},
 			},
 			{
-				Skip:        true,
 				Query:       "insert into child_datetime0 values ('2001-02-03 12:34:56');",
 				ExpectedErr: sql.ErrForeignKeyChildViolation,
 			},
 			{
-				Skip:  true,
 				Query: "create table child_datetime6 (dt datetime(6), foreign key (dt) references parent_datetime0(dt));",
 				Expected: []sql.Row{
 					{types.NewOkResult(0)},
 				},
 			},
 			{
-				Skip:        true,
 				Query:       "insert into child_datetime6 values ('2001-02-03 12:34:56');",
 				ExpectedErr: sql.ErrForeignKeyChildViolation,
 			},
 
 			{
-				Skip:  true,
 				Query: "create table child1_timestamp0 (ts timestamp, foreign key (ts) references parent_datetime0(dt));",
 				Expected: []sql.Row{
 					{types.NewOkResult(0)},
 				},
 			},
 			{
-				Skip:        true,
 				Query:       "insert into child1_timestamp0 values ('2001-02-03 12:34:56');",
 				ExpectedErr: sql.ErrForeignKeyChildViolation,
 			},
 			{
-				Skip:  true,
 				Query: "create table child2_timestamp0 (ts timestamp, foreign key (ts) references parent_datetime6(dt));",
 				Expected: []sql.Row{
 					{types.NewOkResult(0)},
 				},
 			},
 			{
-				Skip:        true,
 				Query:       "insert into child2_timestamp0 values ('2001-02-03 12:34:56');",
 				ExpectedErr: sql.ErrForeignKeyChildViolation,
 			},
 
 			{
-				Skip:  true,
 				Query: "create table child1_timestamp6 (ts timestamp(6), foreign key (ts) references parent_datetime0(dt));",
 				Expected: []sql.Row{
 					{types.NewOkResult(0)},
 				},
 			},
 			{
-				Skip:        true,
 				Query:       "insert into child1_timestamp6 values ('2001-02-03 12:34:56');",
 				ExpectedErr: sql.ErrForeignKeyChildViolation,
 			},
 			{
-				Skip:  true,
 				Query: "create table child2_timestamp6 (ts timestamp(6), foreign key (ts) references parent_datetime6(dt));",
 				Expected: []sql.Row{
 					{types.NewOkResult(0)},
 				},
 			},
 			{
-				Skip:        true,
 				Query:       "insert into child2_timestamp6 values ('2001-02-03 12:34:56');",
 				ExpectedErr: sql.ErrForeignKeyChildViolation,
 			},
 			{
-				Skip:        true,
 				Query:       "insert into child2_timestamp6 values ('2001-02-03 12:34:56.123456');",
 				ExpectedErr: sql.ErrForeignKeyChildViolation,
 			},
@@ -11198,81 +11185,68 @@ where
 		},
 		Assertions: []ScriptTestAssertion{
 			{
-				Skip:  true,
 				Query: "create table child_timestamp0 (ts timestamp, foreign key (ts) references parent_timestamp6(ts));",
 				Expected: []sql.Row{
 					{types.NewOkResult(0)},
 				},
 			},
 			{
-				Skip:        true,
 				Query:       "insert into child_timestamp0 values ('2001-02-03 12:34:56');",
 				ExpectedErr: sql.ErrForeignKeyChildViolation,
 			},
 			{
-				Skip:  true,
 				Query: "create table child_timestamp6 (ts timestamp(6), foreign key (ts) references parent_timestamp0(ts));",
 				Expected: []sql.Row{
 					{types.NewOkResult(0)},
 				},
 			},
 			{
-				Skip:        true,
 				Query:       "insert into child_timestamp6 values ('2001-02-03 12:34:56');",
 				ExpectedErr: sql.ErrForeignKeyChildViolation,
 			},
 
 			{
-				Skip:  true,
 				Query: "create table child1_datetime0 (dt datetime, foreign key (dt) references parent_timestamp0(ts));",
 				Expected: []sql.Row{
 					{types.NewOkResult(0)},
 				},
 			},
 			{
-				Skip:        true,
 				Query:       "insert into child1_datetime0 values ('2001-02-03 12:34:56');",
 				ExpectedErr: sql.ErrForeignKeyChildViolation,
 			},
 			{
-				Skip:  true,
 				Query: "create table child2_datetime0 (dt datetime, foreign key (dt) references parent_timestamp6(ts));",
 				Expected: []sql.Row{
 					{types.NewOkResult(0)},
 				},
 			},
 			{
-				Skip:        true,
 				Query:       "insert into child2_datetime0 values ('2001-02-03 12:34:56');",
 				ExpectedErr: sql.ErrForeignKeyChildViolation,
 			},
 
 			{
-				Skip:  true,
 				Query: "create table child1_datetime6 (dt datetime(6), foreign key (dt) references parent_timestamp0(ts));",
 				Expected: []sql.Row{
 					{types.NewOkResult(0)},
 				},
 			},
 			{
-				Skip:        true,
 				Query:       "insert into child1_datetime6 values ('2001-02-03 12:34:56');",
 				ExpectedErr: sql.ErrForeignKeyChildViolation,
 			},
 			{
-				Skip:  true,
 				Query: "create table child2_datetime6 (dt datetime(6), foreign key (dt) references parent_timestamp6(ts));",
 				Expected: []sql.Row{
 					{types.NewOkResult(0)},
 				},
 			},
 			{
-				Skip:        true,
 				Query:       "insert into child2_datetime6 values ('2001-02-03 12:34:56');",
 				ExpectedErr: sql.ErrForeignKeyChildViolation,
 			},
 			{
-				Skip:        true,
 				Query:       "insert into child2_datetime6 values ('2001-02-03 12:34:56.123456');",
 				ExpectedErr: sql.ErrForeignKeyChildViolation,
 			},
@@ -11314,9 +11288,9 @@ where
 				},
 			},
 			{
-				Skip:        true,
 				Query:       "insert into child_time0 values ('12:34:56');",
 				ExpectedErr: sql.ErrForeignKeyChildViolation,
+				Skip:        true, // TODO: Fix TIME precision handling in foreign key constraints (https://github.com/dolthub/dolt/issues/9544)
 			},
 			{
 				Query: "create table child_time6 (t time(6), foreign key (t) references parent_time0(t));",
@@ -11325,9 +11299,9 @@ where
 				},
 			},
 			{
-				Skip:        true,
 				Query:       "insert into child_time6 values ('12:34:56');",
 				ExpectedErr: sql.ErrForeignKeyChildViolation,
+				Skip:        true, // TODO: Fix TIME precision handling in foreign key constraints (https://github.com/dolthub/dolt/issues/9544)
 			},
 		},
 	},
diff --git a/sql/plan/alter_foreign_key.go b/sql/plan/alter_foreign_key.go
@@ -651,6 +651,11 @@ func foreignKeyComparableTypes(ctx *sql.Context, type1 sql.Type, type2 sql.Type)
 	t1 := type1.Type()
 	t2 := type2.Type()
 
+	// MySQL allows time-related types to reference each other in foreign keys
+	if (types.IsTime(type1) || types.IsTimespan(type1)) && (types.IsTime(type2) || types.IsTimespan(type2)) {
+		return true
+	}
+
 	// Handle same-type cases for special types
 	if t1 == t2 {
 		switch t1 {
diff --git a/sql/plan/foreign_key_editor.go b/sql/plan/foreign_key_editor.go
@@ -507,7 +507,7 @@ func (reference *ForeignKeyReferenceHandler) CheckReference(ctx *sql.Context, ro
 	}
 	defer rowIter.Close(ctx)
 
-	_, err = rowIter.Next(ctx)
+	parentRow, err := rowIter.Next(ctx)
 	if err != nil && err != io.EOF {
 		// For SET types, conversion failures during foreign key validation should be treated as foreign key violations
 		if sql.ErrConvertingToSet.Is(err) || sql.ErrInvalidSetValue.Is(err) {
@@ -518,9 +518,10 @@ func (reference *ForeignKeyReferenceHandler) CheckReference(ctx *sql.Context, ro
 	}
 	if err == nil {
 		// We have a parent row, but check for type-specific validation
-		if validationErr := reference.validateDecimalConstraints(row); validationErr != nil {
+		if validationErr := reference.validateColumnTypeConstraints(ctx, row, parentRow); validationErr != nil {
 			return validationErr
 		}
+
 		// We have a parent row so throw no error
 		return nil
 	}
@@ -548,33 +549,46 @@ func (reference *ForeignKeyReferenceHandler) CheckReference(ctx *sql.Context, ro
 		reference.ForeignKey.ParentTable, reference.RowMapper.GetKeyString(row))
 }
 
-// validateDecimalConstraints checks that decimal foreign key columns have compatible scales.
-func (reference *ForeignKeyReferenceHandler) validateDecimalConstraints(row sql.Row) error {
-	if reference.RowMapper.Index == nil {
+// validateColumnTypeConstraints enforces foreign key type validation between child and parent columns in a foreign key relationship.
+func (reference *ForeignKeyReferenceHandler) validateColumnTypeConstraints(ctx *sql.Context, childRow sql.Row, parentRow sql.Row) error {
+	mapper := reference.RowMapper
+	if mapper.Index == nil {
 		return nil
 	}
-	indexColumnTypes := reference.RowMapper.Index.ColumnExpressionTypes()
-	for parentIdx, parentCol := range indexColumnTypes {
-		if parentIdx >= len(reference.RowMapper.IndexPositions) {
+
+	for parentIdx, parentCol := range mapper.Index.ColumnExpressionTypes() {
+		if parentIdx >= len(mapper.IndexPositions) {
 			break
 		}
+
 		parentType := parentCol.Type
-		childColIdx := reference.RowMapper.IndexPositions[parentIdx]
-		childType := reference.RowMapper.SourceSch[childColIdx].Type
-		childDecimal, ok := childType.(sql.DecimalType)
-		if !ok {
-			continue
+		childType := mapper.SourceSch[mapper.IndexPositions[parentIdx]].Type
+		hasViolation := false
+
+		// For decimal types, scales must match exactly
+		childDecimal, childOk := childType.(sql.DecimalType)
+		parentDecimal, parentOk := parentType.(sql.DecimalType)
+		if childOk && parentOk {
+			hasViolation = childDecimal.Scale() != parentDecimal.Scale()
 		}
-		parentDecimal, ok := parentType.(sql.DecimalType)
-		if !ok {
-			continue
+
+		// For time types, require exact type matching (including precision)
+		// TODO: The TIME type currently normalizes all precisions to TIME(6) internally,
+		// which means TIME and TIME(n) are all treated as TIME(6). This prevents proper
+		// precision validation between different TIME types in foreign keys.
+		// See time.go:"TIME is implemented as TIME(6)."
+		isChildTime := types.IsTime(childType) || types.IsTimespan(childType)
+		isParentTime := types.IsTime(parentType) || types.IsTimespan(parentType)
+		if isChildTime && isParentTime {
+			hasViolation = hasViolation || !childType.Equals(parentType)
 		}
-		if childDecimal.Scale() != parentDecimal.Scale() {
+
+		if hasViolation {
 			return sql.ErrForeignKeyChildViolation.New(
 				reference.ForeignKey.Name,
 				reference.ForeignKey.Table,
 				reference.ForeignKey.ParentTable,
-				reference.RowMapper.GetKeyString(row),
+				mapper.GetKeyString(childRow),
 			)
 		}
 	}
@@ -638,6 +652,7 @@ func (mapper *ForeignKeyRowMapper) GetIter(ctx *sql.Context, row sql.Row, refChe
 		}
 
 		targetType := mapper.SourceSch[rowPos].Type
+
 		// Transform the type of the value in this row to the one in the other table for the index lookup, if necessary
 		if mapper.TargetTypeConversions != nil && mapper.TargetTypeConversions[rowPos] != nil {
 			var err error
