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

Fix explicit DEFAULT value in insert query #519

Merged
merged 6 commits into from
Aug 10, 2021

Conversation

Wuvist
Copy link
Contributor

@Wuvist Wuvist commented Aug 6, 2021

It seems that go-mysql-server is not compatible with insert query like:

INSERT INTO `users` (`id`, `deleted`) VALUES (1, DEFAULT)

Whereas DEFAULT is just to specify deleted columns should use column default value explicitly.

The issue could be demostrated using below code:

package main

import (
	dbsql "database/sql"

	sqle "github.com/dolthub/go-mysql-server"
	"github.com/dolthub/go-mysql-server/auth"
	"github.com/dolthub/go-mysql-server/memory"
	"github.com/dolthub/go-mysql-server/server"
	_ "github.com/go-sql-driver/mysql"
)

func main() {
	db, _ := dbsql.Open("mysql", "root:@tcp(127.0.0.1:3307)/test")
	defer db.Close()

	query := `CREATE TABLE users (
  id bigint(20) unsigned NOT NULL AUTO_INCREMENT,
  deleted tinyint(1) unsigned NOT NULL DEFAULT '0',
  PRIMARY KEY (id)
) ENGINE=InnoDB AUTO_INCREMENT=1 DEFAULT CHARSET=utf8mb4`

	stmt, _ := db.Prepare(query)
	stmt.Exec()

	_, err := db.Exec("INSERT INTO `users` (`id`, `deleted`) VALUES (1, DEFAULT)")
	if err != nil {
		panic(err.Error())
	}

	stmtOut, _ := db.Prepare("SELECT `deleted` FROM `users` WHERE `id` = 1")
	defer stmtOut.Close()

	deleted := true
	err = stmtOut.QueryRow().Scan(&deleted)
	if err != nil {
		panic(err.Error())
	}

	if deleted == true {
		panic("Wrong deleted value")
	}
}

var engine *sqle.Engine

func init() {
	engine = sqle.NewDefault()
	db := memory.NewDatabase("test")
	engine.AddDatabase(db)

	config := server.Config{
		Protocol: "tcp",
		Address:  "localhost:3307",
		Auth:     auth.NewNativeSingle("root", "", auth.AllPermissions),
	}

	s, _ := server.NewDefaultServer(config, engine)

	go s.Start()
}
`db.Exec("INSERT INTO `users` (`id`, `deleted`) VALUES (1, DEFAULT)")` will result in error:

Error 1105: plan is not resolved because of node '*plan.Values'

This pull request should avoid such issue by turning explicit DEFAULT in insert query to implicit.

Copy link
Member

@zachmu zachmu 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 the contribution!

This looks good, but please add tests:

  1. parse_test.go
  2. insert_queries.go

And don't forget to run ./format_repo.sh when you're done.


if e, ok := src.(*plan.Values); ok {
var needCols []sql.Expression
for i, s := range e.ExpressionTuples[0] {
Copy link
Member

Choose a reason for hiding this comment

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

There can be more than 1 element in ExpressionTuples, need to account for that

@Wuvist Wuvist force-pushed the defaultPleaceHolder branch from e46e3aa to 3d9065a Compare August 6, 2021 21:58
@Wuvist
Copy link
Contributor Author

Wuvist commented Aug 6, 2021

@zachmu Updated pull request with tests and handles multiple ExpressionTuples.

However, as my method is just to turn explicit DEFAULT to implicit. So, it won't work insertion query test case like this:

{
	Name: "explicit DEFAULT with multiple values different order",
	SetUpScript: []string{
		"CREATE TABLE mytable(id int PRIMARY KEY, v2 int NOT NULL DEFAULT '2', v3 int NOT NULL DEFAULT '3')",
	},
	Assertions: []ScriptTestAssertion{
		{
			Query: "INSERT INTO mytable (id, v2, v3)values (1, DEFAULT, 0), (2, 0, DEFAULT)",
			Expected: []sql.Row{
				{sql.OkResult{RowsAffected: 2}},
			},
		},
		{
			Query: "SELECT * FROM mytable",
			Expected: []sql.Row{
				{1, 2, 0},
				{2, 0, 3},
			},
		},
	},
},

Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes!

@zachmu zachmu merged commit 0ba4092 into dolthub:master Aug 10, 2021
@zachmu
Copy link
Member

zachmu commented Aug 10, 2021

#527

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.

2 participants