`CREATE TABLE` doesn't generate `Query OK` confirmation message

[angelamayxie]

Dolt

tmp/main*> create table MY_TABLE (ID int not null primary key, MY_BOOL bool not null);
tmp/main*> insert into MY_TABLE values (0, false);
Query OK, 1 row affected (0.00 sec)

MySQL

mysql> create table MY_TABLE (ID int not null primary key, MY_BOOL bool not null);
Query OK, 0 rows affected (0.01 sec)

mysql> insert MY_TABLE values (0, false);
Query OK, 1 row affected (0.00 sec)

[jycor]

This is specific to dolt sql shell and dolt sql -q "...".

I'm not sure why, but the function processParsedQuery from dolt/go/cmd/dolt/commands/sql.go, special cases DDL statements and returns nil for schema and rowIter?

This might be the fix, not sure what tests it'll break

diff --git a/go/cmd/dolt/commands/sql.go b/go/cmd/dolt/commands/sql.go
index 4c85367b52..4d20ca5277 100644
--- a/go/cmd/dolt/commands/sql.go
+++ b/go/cmd/dolt/commands/sql.go
@@ -1199,15 +1199,11 @@ func processParsedQuery(ctx *sql.Context, query string, qryist cli.Queryist, sql
                }
                return nil, nil, nil, nil
        case *sqlparser.DDL:
-               _, ri, _, err := qryist.Query(ctx, query)
-               if err != nil {
-                       return nil, nil, nil, err
-               }
-               _, err = sql.RowIterToRows(ctx, ri)
+               sch, ri, _, err := qryist.Query(ctx, query)
                if err != nil {
                        return nil, nil, nil, err
                }
-               return nil, nil, nil, nil
+               return sch, ri, nil, nil
        case *sqlparser.Load:
                if s.Local {
                        return nil, nil, nil, fmt.Errorf("LOCAL supported only in sql-server mode")

[aaryansinhaa]

Hey @jycor and @angelamayxie , I tried @jycor fix, and here is what I did

In processParsedQuery inside dolt/go/cmd/dolt/commands/sql.go, DDL statements (like CREATE TABLE) were returning nil for both schema and row iterator, which caused the shell to skip printing the usual confirmation message (Query OK, 0 rows affected).

I modified the DDL case to return the schema and rowIter from qryist.Query(...), allowing the shell to process the result and print the confirmation message just like it does for other statements.

In dolt/go/cmd/dolt/commands/sql.go , inside processParsedQuery :

        return nil, nil, nil, nil
	case *sqlparser.DDL:
		sch, ri, _, err := qryist.Query(ctx, query)
		if err != nil {
			return nil, nil, nil, err
		}
		return sch, ri, nil, nil
	case *sqlparser.Load:

How I tested:

• Ran go test ./cmd/dolt/commands -v and confirmed all tests pass.
• Manually tested in the SQL shell:

I am putting a PR for this fix, would appreciate any feedback

[macneale4]

We also don't show the success confirmation for create trigger.

[angelamayxie]

We also don’t show a confirmation message for alter table and set statements

[macneale4]

Went down a bit of a rabbit hole on this.

SET statements in GMS are not returning the right content. This is the GMS change I made to make that work: https://github.com/dolthub/go-mysql-server/pull/3046, but I realized that this was breaking a bunch of tests in Dolt for reasons I didn't expect, so I reverted it shortly thereafter: https://github.com/dolthub/go-mysql-server/pull/3056

Changing the output of SET by itself will break many changes test in Dolt. Unfortunately I was actually trying to fix this issue, which is about the shell printing the right updates, so that got all jumbled up which let to a deeper issue which is that the dolt sql shell doesn't have the right information to determine when to print the Query OK line. Specifically, mysql shell will not print any of them if it's executing statements in a batch.

This is best shown here:

$ echo "create table t (i int); drop table t;" | mysql -u root -h 127.0.0.1 -P 3306 mydb
$ mysql -u root -h 127.0.0.1 -P 3306 mydb
Welcome to the MySQL monitor.  Commands end with ; or \g.
Your MySQL connection id is 9
Server version: 8.0.33 Dolt

Copyright (c) 2000, 2023, Oracle and/or its affiliates.

Oracle is a registered trademark of Oracle Corporation and/or its
affiliates. Other names may be trademarks of their respective
owners.

Type 'help;' or '\h' for help. Type '\c' to clear the current input statement.

mysql> create table t (i int);
Query OK, 0 rows affected (0.00 sec)

mysql> drop table t;
Query OK, 0 rows affected (0.00 sec)

mysql>

You can see that the output of the first command is empty. MySQL client knows that you are not in an interactive terminal. dolt sql shell doesn't actually know this, and as a result the changes I made to the shell break lots of tests because where ever we stream in statements, we print out Query OK in many places that aren't expected in tests.

The equivalent dolt operations:

$ echo "create table t (i int); drop table t;" | dolt sql


$ dolt sql
# Welcome to the DoltSQL shell.
# Statements must be terminated with ';'.
# "exit" or "quit" (or Ctrl-D) to exit. "\help" for help.
mydb/main> create table t (i int);
Empty set (0.01 sec)

mydb/main*>  drop table t;
Empty set (0.01 sec)

mydb/main>

As you can see, there are empty lines printed in the pipe case, and "Empty Set" returned in the shell case. The changes proposed by James in https://github.com/dolthub/dolt/pull/9291 and ultimately what claude tried to do don't toggle based on if the input is a pipe or interactive, and as a result will always print Query OK - which is not only confusing, but breaks my dolt tests which don't expect them to be there

Next Steps:

• Get dolt sql wise to the way it's being operated. This may require looking at the mysql shell code, or something else. One way or another we need to be aware of when printing these things makes sense.
• Apply the SET change to GMS, and isolate it to fixing tests only in Dolt and Doltgres. Don't modify the shell output at this stage because it breaks a lot of tests and become intractable.
• Update the shell, in the way proposed by James, to get the right information out in the right context.

In some of these changes, we are going to want to toggle behavior with an environment flag such that we won't stop the development train with broken builds. We kind of need better options to optionally enable behaviors while also getting them in the pipeline for testing.

[angelamayxie]

Seems like our output for INSERT differs from mysql as well

mysql

mysql> create table enum_table (i int primary key, e enum('a','b'));
Query OK, 0 rows affected (0.02 sec)

mysql> insert into enum_table values (1, 'a'), (2, 'b');
Query OK, 2 rows affected (0.01 sec)
Records: 2  Duplicates: 0  Warnings: 0

mysql> create table uv (u int primary key, v varchar(10));
Query OK, 0 rows affected (0.01 sec)

mysql> insert into uv values (0, 'bug'), (1, 'ant');
Query OK, 2 rows affected (0.00 sec)
Records: 2  Duplicates: 0  Warnings: 0

dolt

tmp/main*> create table enum_table (i int primary key, e enum('a','b'));
tmp/main*> insert into enum_table values (1, 'a'),(2,'b');
Query OK, 2 rows affected (0.00 sec)
tmp/main*> create table uv (u int primary key, v varchar(10));
tmp/main*> insert into uv values(0, 'bug'),(1,'ant');
Query OK, 2 rows affected (0.00 sec)