Fix mysql parse default b'0' #1584
Conversation
|
nice :)!! As always, test would be great ;) |
| @@ -80,6 +80,17 @@ val SimpleA = CustomTyping.SimpleA | |||
| }) | |||
| }, | |||
| new UUIDConfig("CG10", StandardTestDBs.H2Mem, "H2Mem", Seq("/dbs/uuid-h2.sql")), | |||
| new Config("MySQL", StandardTestDBs.MySQL, "MySQL", Seq("/dbs/mysql.sql")){ | |||
There was a problem hiding this comment.
@cvogt Whats wrong with this test?
There was a problem hiding this comment.
actually looks great. Overlooked that somehow. Great.
There was a problem hiding this comment.
seem like it is failing on travis though? can it be anything like the version, some mysql configuration, the environment (os, 64/32 bit, etc)?
| else l | ||
| override def length: Option[Int] = super.length match{ | ||
| case Some(l) if varying && l >= 65535 => None | ||
| case l => l |
There was a problem hiding this comment.
not a big deal. but the idiomatic way to do this would be super.length.filter(!varying || _ < 64435)
|
How does 772fa7b fix the test failure? |
|
@cvogt I had previously used reference equality check so switched to == instead. v eq "1" to v == "1" |
|
@cvogt Is there anything I should change here ? |
|
@cvogt What should I change here ? |
| case ("1","Boolean") => Some(Some(true)) | ||
| case ("0","Boolean") => Some(Some(false)) | ||
| case ( BooleanPattern(v) , "Boolean") => Some( Some(v == "1")) | ||
| case ( BitPattern(v) , "Boolean") => Some( Some( v == "1")) |
There was a problem hiding this comment.
may be simpler:
case ( "0" | "b'0'", "Boolean" ) => Some( Some(false) )
case ( "1" | "b'1'", "Boolean" ) => Some( Some(true) )
(and maybe even more correct because the regexes didn't use a trailing $, which mean BooleanPattern may have matched 12345 as true, not only 1).
trevorsibanda
force-pushed
the
fix-issue-1268
branch
from
772fa7b
to
dc00b42
Compare
trevorsibanda
force-pushed
the
fix-issue-1268
branch
4 times, most recently
from
799d00d
to
e69ea6f
Compare
|
@cvogt this is ready. Travis CI tests timing out... problem with Oracle |
trevorsibanda
force-pushed
the
fix-issue-1268
branch
from
e69ea6f
to
e6667e9
Compare
trevorsibanda
force-pushed
the
fix-issue-1268
branch
2 times, most recently
from
3913ae8
to
8d69943
Compare
|
@cvogt this is ready |
| if(tpe == "String" && varying && l == Some(65535)) None | ||
| else l | ||
| } | ||
| override def length: Option[Int] = super.length.filter(_ => !varying || dbType == Some("VARCHAR")) |
There was a problem hiding this comment.
Could you explain this change? Just trying to understand the implications :). Ideally anything you found out about the old code here and what the implications of the new code are / what it fixes. Ideally this would be in the commit message, so future contributors can find it using git blame.
| else if(l.length <= 16777215 ) "MEDIUMTEXT" | ||
| if(l.length < 65535 ) s"VARCHAR(${l.length})" | ||
| else if(l.length >= 65535 && l.length < 16777215) "TEXT" | ||
| else if(l.length <= 16777215) "MEDIUMTEXT" |
There was a problem hiding this comment.
Why exclude the 65535 case from VARCHAR? MEDIUMTEXT now only seems to be used for length exactly = 16777215. Was that intended?
trevorsibanda
force-pushed
the
fix-issue-1268
branch
from
83e1f2f
to
ae815d1
Compare
| if(l.length <= 65535 ) s"VARCHAR(${l.length})" | ||
| else if(l.length <= 16777215 ) "MEDIUMTEXT" | ||
| if(l.length < 65535) s"VARCHAR(${l.length})" | ||
| else if(l.length == 65535) "TEXT" |
There was a problem hiding this comment.
mysql jdbc driver sets type TEXT to length 65535. Setting TEXT for length 65535 is safer since there is a row size limit .
[error] Test slick.test.codegen.GeneratedCodeTest.allTests failed: com.mysql.jdbc.exceptions.jdbc4.MySQLSyntaxErrorException: Row size too large. The maximum row size for the used table type, not counting BLOBs, is 65535. This includes storage overhead, check the manual. You have to change some columns to TEXT or BLOBs, took 51.986 sec
trevorsibanda
force-pushed
the
fix-issue-1268
branch
from
ae815d1
to
474c04a
Compare
|
@cvogt can you take a look at this |
|
LGTM! |
#1268
Possible tweak might be to combine the two regexp for match BooleanPattern and BitPattern into one
Maybe