handler no comment delimiter

[shootercheng]

dbeaver export or navacat navicat exoport

--
-- Table structure for table `user`
--

DROP TABLE IF EXISTS `user`;
/*!40101 SET @saved_cs_client     = @@character_set_client */;
/*!40101 SET character_set_client = utf8 */;
CREATE TABLE `user` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `name` varchar(11) DEFAULT NULL,
  `password` varchar(20) DEFAULT NULL,
  `age` int(11) DEFAULT NULL,
  `sqsj` datetime DEFAULT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB AUTO_INCREMENT=13 DEFAULT CHARSET=utf8;
/*!40101 SET character_set_client = @saved_cs_client */;

DELIMITER ;;
CREATE DEFINER=`chengdu`@`localhost` PROCEDURE `insert1`()
begin
declare i int;
set i = 1;
while i < 10 do
set i = i + 1;
end while;
end ;;
DELIMITER ;
DROP TABLE IF EXISTS `testdata_1`;
CREATE TABLE `testdata_1` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `foo` varchar(25) DEFAULT NULL,
  `bar` int(11) DEFAULT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB AUTO_INCREMENT=13 DEFAULT CHARSET=utf8;

[harawata]

Hello @shootercheng ,

Do you need this for mybatis-migrations ?

[shootercheng]

Hello @shootercheng ,

Do you need this for mybatis-migrations ?

I just want to execute table structure SQL instead of migrating data

[harawata]

Okay, thanks for the info, @shootercheng !

ScriptRunner is actually created for Migrations and some changes in this PR breaks compatibility, so we cannot merge this, unfortunately.
I would suggest you to copy ScriptRunner.java to your project and make necessary modifications.

[shootercheng]

Okay, thanks for the info, @shootercheng !

ScriptRunner is actually created for Migrations and some changes in this PR breaks compatibility, so we cannot merge this, unfortunately.
I would suggest you to copy ScriptRunner.java to your project and make necessary modifications.

I've tried mybatis migration, but errors will still be reported when executing stored procedures, so the original code does not support stored procedures, which is not a compatibility issue, but a bug fix issue. For more details, please refer to mybatis-migration-demo

[shootercheng]

@harawata Thank you for your advice
When executing the VersionWithoutChangeLog main method, the following error will occur
ERROR: Error executing command. Cause: org.apache.ibatis.jdbc.RuntimeSqlException: Error executing: DELIMITER ; . Cause: java.sql.SQLSyntaxErrorException: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'DELIMITER' at line 1

If we judge the delimiter first, we can execute the stored procedure SQL correctly. Of course, it will also be compatible with the original code

[harawata]

If you use mybatis-migrations or the built-in ScriptRunner, you need to use the special delimiter command (which is different from mysql client's delimiter command).

-- @DELIMITER ;;
CREATE DEFINER=`chengdu`@`localhost` PROCEDURE `insert1`()
begin
declare i int;
set i = 1;
while i < 10 do
insert into testdata(foo, bar) values(concat('chengdu',i), i);
set i = i + 1;
end while;
end ;;
-- @DELIMITER ;

Sorry for not being clear.
I assumed you knew as you modified the regex pattern yourself.

[shootercheng]

If you use mybatis-migrations or the built-in ScriptRunner, you need to use the special delimiter command (which is different from mysql client's delimiter command).

-- @DELIMITER ;;
CREATE DEFINER=`chengdu`@`localhost` PROCEDURE `insert1`()
begin
declare i int;
set i = 1;
while i < 10 do
insert into testdata(foo, bar) values(concat('chengdu',i), i);
set i = i + 1;
end while;
end ;;
-- @DELIMITER ;

Sorry for not being clear.
I assumed you knew as you modified the regex pattern yourself.

I see what you mean. If you export many stored procedure SQL through Navicat or Dbeaver, we need to modify each Delimiter. It's very funny, Is this to modify SQL to adapt the code?Shouldn't the program handle multiple input situations?

[shootercheng]

@harawata I've understood what the original author meant. I've updated the code

[harawata]

Shouldn't the program handle multiple input situations?

To achieve this, we have to have parsers that understand command syntax of various SQL clients.
I don't think it's realistic.

In the initial implementation of delimiter support, the non-comment delimiter was recognized as a delimiter command (see #241 ).
But it was changed later ( #355 ) because DELIMITER is a vendor specific command.
So, we probably won't support this syntax out-of-the-box.

[shootercheng]

@harawata Ha-ha, Now we can define a delimiter handler to support vendor specific commands

[harawata]

Thank you for the update, @shootercheng !

Does it have to be a regex?
I've seen several similar requests demanding a configurable delimiter, but they all are about the output of 'mysqldump' (I am not familiar with dbeaver nor navicat, but the output in the issue description looks the same as mysqldump's).

So, maybe, just a simple prefix matching satisfies most users.
Something like...

if (line.regionMatches(true, 0, delimiterCommandPrefix,
    0, delimiterCommandPrefix.length())) {
  delimiter = line.substring(delimiterCommandPrefix.length()).trim()

Do you have a use case that requires regex matching?

Note that the default implementation has to be regex to maintain backward compatibility.

@h3adache ,
Could you take a look when you have time?
I would like to hear your opinion on this.

[shootercheng]

Thank you for the update, @shootercheng !

Does it have to be a regex?
I've seen several similar requests demanding a configurable delimiter, but they all are about the output of 'mysqldump' (I am not familiar with dbeaver nor navicat, but the output in the issue description looks the same as mysqldump's).

So, maybe, just a simple prefix matching satisfies most users.
Something like...

if (line.regionMatches(true, 0, delimiterCommandPrefix,
    0, delimiterCommandPrefix.length())) {
  delimiter = line.substring(delimiterCommandPrefix.length()).trim()

Do you have a use case that requires regex matching?

Note that the default implementation has to be regex to maintain backward compatibility.

@h3adache ,
Could you take a look when you have time?
I would like to hear your opinion on this.

@harawata Thank you for your advice. Maybe it's the result of inertia thinking, I've updated nocommet handler

[h3adache]

Because this is used only in migrations atm. What do you think of a regex configuration that requires a group.
We can allow any type of delimiter in that case as we only care about the group. For example
delimiter = DELIMITER (..) would regex match any 2 characters after the word delimiter and a space which works for MySQL and simply doing
delimiter = ($$) would work for postgresql
This could handle a lot of the use cases without having to do db specific syntax.

Sorry I haven’t tested the above regex. It’s just a thought atm.

[shootercheng]

The regex expression of the default delimiter handler is the same as befor, it doesn't affect the original logic. harawata is talking about nocomment handler

…

---Original--- From: "Tim Chen"<[email protected]&gt; Date: Mon, Apr 13, 2020 08:05 AM To: "mybatis/mybatis-3"<[email protected]&gt;; Cc: "Mention"<[email protected]&gt;;"James Sun"<[email protected]&gt;; Subject: Re: [mybatis/mybatis-3] handler no comment delimiter (#1890) Because this is used only in migrations atm. What do you think of a regex configuration that requires a group. We can allow any type of delimiter in that case as we only care about the group. For example delimiter = DELIMITER (..) would regex match any 2 characters after the word delimiter and a space which works for MySQL and simply doing delimiter = ($$) would work for postgresql This could handle a lot of the use cases without having to do db specific syntax. Sorry I haven’t tested the above regex. It’s just a thought atm. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[h3adache]

I’m sorry I wasn’t clear. I was talking about seeding a noncomment handler with a delimiter. So that you don’t hardcode DELIMITER and also can handle cases were the delimiter isn’t named or isn’t in the end of the delimiter line (as if it’s quoted). Sorry it’s just a quick thought. I don’t know if it makes it overly complicated as I haven’t tried it yet but seems like a simple way of handling it.

[shootercheng]

You can define different delimiter hander. For example, MysqlHander, PostgresqlHander. In my opinion, Regex matching of every line is unnecessary.

…

---Original--- From: "Tim Chen"<[email protected]&gt; Date: Mon, Apr 13, 2020 09:28 AM To: "mybatis/mybatis-3"<[email protected]&gt;; Cc: "Mention"<[email protected]&gt;;"James Sun"<[email protected]&gt;; Subject: Re: [mybatis/mybatis-3] handler no comment delimiter (#1890) I’m sorry I wasn’t clear. I was talking about seeding a noncomment handler with a delimiter. So that you don’t hardcode DELIMITER and also can handle cases were the delimiter isn’t named or isn’t in the end of the delimiter line (as if it’s quoted). Sorry it’s just a quick thought. I don’t know if it makes it overly complicated as I haven’t tried it yet but seems like a simple way of handling it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[harawata]

Thank you for the comment, @h3adache ,

This needs to be a separate setting than the current delimiter option in the environment file because delimiter is used for all migration scripts which may contain non-procedure statements.

And detecting delimiter change without using explicit delimiter command won't be that easy.
Here is an example script that contains PostgreSQL style create procedure statement.

CREATE TABLE tbl (
  num integer
);

CREATE PROCEDURE insert_data(a integer, b integer)
LANGUAGE SQL AS
$$
INSERT INTO tbl VALUES (a);
INSERT INTO tbl VALUES (b);
$$

To detect the delimiter change, it may require a parser that understands PostgreSQL's CREATE PROCEDURE syntax (it's basically the same request as mybatis/migrations#95).
It requires a major change to Migrations/ScriptRunner and users may have to write the parser by themselves, so I'm a bit skeptical about its usefulness.

I considered this enhancement only because it could be simple, so if you think it's too simple, let's just keep it open and collect feedback. :)

[shootercheng]

@harawata @h3adache
My purpose is to enhance ScriptRunner, not migration. Because we only use ScriptRunner to execute SQL in our project, there is no need to import migration.
In addition, This request is compatible with the previous code, and there is no need to modify anything when using migration

[harawata]

@shootercheng ,
Then, like I suggested first, it is best to copy ScriptRunner.java to your project and make necessary changes there.
It's a very simple JDBC statement executor and does not depend on MyBatis (just replace RuntimeSqlException with RuntimeException).

I personally want to deprecate this class in the core (as I explained, this class belongs to Migrations, not the core).

[shootercheng]

I see a lot of test code using it.Maybe ScriptRunner should move to the test pkg. When you perform the stored procedure test, you still need to deal with the delimiter. So, we need to find the right way to deal with it, Do you think there problems with my handling?

…

---Original--- From: "Iwao AVE!"<[email protected]&gt; Date: Tue, Apr 14, 2020 11:04 AM To: "mybatis/mybatis-3"<[email protected]&gt;; Cc: "Mention"<[email protected]&gt;;"James Sun"<[email protected]&gt;; Subject: Re: [mybatis/mybatis-3] handler no comment delimiter (#1890) @shootercheng , Then, like I suggested first, it is best to copy ScriptRunner.java to your project and make necessary changes there. It's a very simple JDBC statement executor and does not depend on MyBatis (just replace RuntimeSqlException with RuntimeException). I personally want to deprecate this class in the core (as I explained, this class belongs to Migrations, not the core). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[harawata]

Yes, in this repo (=MyBatis core), this class is used as a simple utility for loading test SQLs.
I keep telling you that the right way is to use ScriptRunner's delimiter command (i.e. -- @DELIMITER). XD
We use this command in some tests as well.

Your patch is fine if you modify your own copy of ScriptRunner.java.
But it adds new interface which is not useful to MyBatis' core functionality.

I'll add a comment to make it clear that this is an internal test utility.
I will also make a copy in Migrations to make things simpler.

[shootercheng]

When we use migration to execute Navicat or Dbeaver exported stored procedures, we need&nbsp;to modify each stored procedure. I don't think it's the right way.&nbsp;

…

---Original--- From: "Iwao AVE!"<[email protected]&gt; Date: Tue, Apr 14, 2020 19:50 PM To: "mybatis/mybatis-3"<[email protected]&gt;; Cc: "Mention"<[email protected]&gt;;"James Sun"<[email protected]&gt;; Subject: Re: [mybatis/mybatis-3] handler no comment delimiter (#1890) Yes, in this repo (=MyBatis core), this class is used as a simple utility for loading test SQLs. I keep telling you that the right way is to use ScriptRunner's delimiter command (i.e. -- @delimiter). XD We use this command in some tests as well. Your patch is fine if you modify your own copy of ScriptRunner.java. But it adds new interface which is not useful to MyBatis' core functionality. I'll add a comment to make it clear that this is an internal test utility. I will also make a copy in Migrations to make things simpler. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[harawata]

You have a different requirement than MyBatis.
The ScriptRunner in this repo is an internal testing utility and it is pointless to make it compatible with mysql client.
So, my suggestion is to copy MyBatis version of ScriptRunner.java to your project and create your own version.
It's very easy and clean.
If my explanation is not clear, let me know.

For Migrations, there still is a possibility of customizable delimiter token, however, @h3adache and I have a different view on this subject, so it might take a while.
Hope you understand!

[shootercheng]

Thanks for your explanation! I will wait for your good news.

…

---Original--- From: "Iwao AVE!"<[email protected]&gt; Date: Wed, Apr 15, 2020 21:03 PM To: "mybatis/mybatis-3"<[email protected]&gt;; Cc: "Mention"<[email protected]&gt;;"James Sun"<[email protected]&gt;; Subject: Re: [mybatis/mybatis-3] handler no comment delimiter (#1890) You have a different requirement than MyBatis. The ScriptRunner in this repo is an internal testing utility and it is pointless to make it compatible with mysql client. So, my suggestion is to copy MyBatis version of ScriptRunner.java to your project and create your own version. It's very easy and clean. If my explanation is not clear, let me know. For Migrations, there still is a possibility of customizable delimiter token, however, @h3adache and I have a different view on this subject, so it might take a while. Hope you understand! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[shootercheng]

@harawata I have wrote a project. Would you like to give me some suggestions? https://github.com/CatDou/data-migration

[harawata]

I just took a quick look. Good work, @shootercheng !
Most lines in the ScriptRunner are for Migrations, so your BaseScriptRunner can be much simpler, probably. :)

[shootercheng]

I just took a quick look. Good work, @shootercheng !
Most lines in the ScriptRunner are for Migrations, so your BaseScriptRunner can be much simpler, probably. :)

Ha-ha, We are Standing on the shoulders of giants

[h3adache]

For Migrations, there still is a possibility of customizable delimiter token, however, @h3adache and I have a different view on this subject, so it might take a while.
Hope you understand!

Hi @harawata! My view on it is not very formulated atm. I defer to you of course, I was simply trying to think of ways that we can simplify both comment and non comment delimiter cases into a single handler. So we can check the line to see if it's either an explicit delimiter change OR we can check against a configured pattern for delimiters that we can recognize.

[harawata]

Thanks for the explanation, @h3adache !

From what I see, there are two levels of possible enhancements.

1. Support MySQL's DELIMITER command : this is relatively easy. I just prefer to avoid performing regex pattern matching on every line in a file (currently, regex matching is done only on comment lines) and that was why I proposed a simple prefix matching (i.e. line.indexOf(delimiterPrefix) == 0).
2. Support other DBs' stored procedure declaration (which don't have explicit delimiter command) : this requires more than just checking a single line and may require users to write a SQL parser/analyzer (see my earlier comment).

It's unclear which you are referring to by 'non-comment delimiter case'.
Personally, I am still not convinced if either one of these enhancements is beneficial to Migrations.
Migrations already has -- @UNDO command, so there seems to be no good reason to avoid -- @DELIMITER at the cost of performance.

It is a different story if, like @shootercheng , one just needs a SQL import tool, but then it would make more sense to write a new class from scratch rather than improving the unnecessarily complex ScriptRunner as all they need to do is to read a file and call Statement.execute().