-
Notifications
You must be signed in to change notification settings - Fork 171
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
[BREAKING] Stored procedure/functions support for MsSQL, MySQL and PostrgeSQL #341
Conversation
@@ -28,7 +28,6 @@ db: db_sqlite | |||
usql pg://postgres:pgpass@localhost:55432/testdb?sslmode=disable -f testdata/ddl/postgres95.sql | |||
usql pg://postgres:pgpass@localhost:55413/testdb?sslmode=disable -f testdata/ddl/postgres.sql | |||
usql my://root:mypass@localhost:33306/testdb -f testdata/ddl/mysql56.sql | |||
usql my://root:mypass@localhost:33308/testdb -f testdata/ddl/mysql.sql |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how to execute such script with stored procedure and function via usql
. So it is moved to be executed during container creation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that CREATE PROCEDURE could not be executed using usql
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't able to execute this file with the following code using usql
DELIMITER //
CREATE PROCEDURE GetAllComments()
BEGIN
SELECT * FROM comments;
END//
DELIMITER ;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@YauhenPylAurea GREAT!!!!!!! Thank you!!!
I made a few comments, most of them LTGM.
Let me think about it some more 🙏
Thank you!!!
drivers/mssql/mssql.go
Outdated
@@ -467,9 +536,17 @@ func (m *Mssql) Info() (*schema.Driver, error) { | |||
return nil, err | |||
} | |||
|
|||
dct := dict.New() | |||
dct.Merge(map[string]string{ | |||
"Subroutines": "Stored procedures and functions", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if Subroutines
is an appropriate name for the section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. Should I rename it to ProceduresAndUDF
or something like that?
3e3b8b7
to
4cebb4a
Compare
drivers/mysql/mysql.go
Outdated
@@ -487,9 +544,17 @@ func (m *Mysql) Info() (*schema.Driver, error) { | |||
name = "mariadb" | |||
} | |||
|
|||
dct := dict.New() | |||
dct.Merge(map[string]string{ | |||
"Subroutines": "Stored procedures and functions", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Subroutines": "Stored procedures and functions", | |
"Functions": "Stored procedures and functions", |
I've given it a lot of thought.
How about I proceed with the following naming?
Because, the word "Function" is considered to cover the word "Procedure", and the new word "Subroutine" is not used. I tried not to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be happy to have the other "Subroutine" changed as well!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -67,6 +67,7 @@ | |||
"virtual": false | |||
} | |||
], | |||
"subroutines": null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compared to https://github.com/k1LoW/tbls/pull/341/files#diff-7dde8826bb86aa9a1addcc1aedcda0cb76b0b57e2d20d5415cf8fd5ac6baa3b6R49, there seems to be a mixture of null and [], do you know why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in f0a2289. Is it ok to have empty array?
drivers/mssql/mssql.go
Outdated
@@ -459,6 +466,68 @@ ORDER BY i.index_id | |||
return nil | |||
} | |||
|
|||
const query = `select schema_name(obj.schema_id) as schema_name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have the extra power, it would be great if you could change the reserved words in the SQL to uppercase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reverts commit 315231c.
updated per the review. Thank you very much to take a look at this PR! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GREAT!!!!!!!! Thank you!!!!!!! 🎸
New section in README.md to show store procedures or user-defined functions, e.g.
Subroutines