ColumnType interfaces #667

julienschmidt · 2017-09-18T09:37:52Z

Description

Fixes #495

Checklist

Code compiles correctly
Created tests which fail without the change (if possible)
All tests passing
Extended the README / documentation, if necessary
Added myself / the copyright holder to the AUTHORS file

julienschmidt · 2017-09-18T09:40:30Z

We should probably add sanity-checks for the index, as in #664

arnehormann · 2017-09-18T21:51:12Z

fields.go

+	fieldTypeMediumBLOB: "MEDIUMBLOB",
+	fieldTypeLongBLOB:   "LONGBLOB",
+	fieldTypeBLOB:       "BLOB",
+	fieldTypeVarString:  "VARSTRING", // correct?


I didn't see these in MySQL anywhere. The docs, e.g. https://dev.mysql.com/doc/refman/5.7/en/string-types.html don't mention them either. I don't think they are correct, should probably be varchar.

https://github.com/PyMySQL/PyMySQL/blob/e027812ab0eab4d0557fa2ab66520d5eb38fc28f/pymysql/connections.py#L1502-L1507

There are no document about it.
After seeing some response, I decided to handle charsetnr == 63 is bytes, not string.

Of course, string can contain bytes.

arnehormann · 2017-09-18T21:53:14Z

rows.go

+}
+
+func (rows *mysqlRows) ColumnTypeNullable(i int) (nullable, ok bool) {
+	return rows.rs.columns[i].flags&flagNotNULL == 0, true


If we add the sanity checks here, it should also check for a negative index for ok.

arnehormann · 2017-09-18T21:57:07Z

const.go

@@ -87,8 +87,10 @@ const (
 )

 // https://dev.mysql.com/doc/internals/en/com-query-response.html#packet-Protocol::ColumnType
+type fieldType byte


I'm not convinced this helps taking all the casting happening in packets.go ...

Implementation for time.Time not yet complete!

julienschmidt · 2017-10-07T15:42:49Z

Removed the ColumnTypeLength implementation for now and added a section to the README

arnehormann · 2017-10-07T20:38:36Z

I think it's the storage length for strings. Number of bytes for the default utf8 "dialect" with 3 bytes per character. utf8_mb4 probably has a value 4 times as long. Calling it utf8 at all is not quite accurate. If you know the encoding, you know the correction factor.

julienschmidt · 2017-10-07T22:13:50Z

That's what I assumed. There is also a character_set field for every column right before the length.
But do we really want to keep track of the factor for every single charset? Is it even possible or do some charsets have a non-constant factor?

We can work on this later, but for now I'd like to get this PR merged without the length before it gets too big.

methane · 2017-10-12T08:31:53Z

but for now I'd like to get this PR merged without the length before it gets too big.

Completely agree.

methane · 2017-10-16T19:19:35Z

Code looks OK.
But I don't know much about real world usage of the API.
Should we distinguish BLOB and TEXT?

But maybe, it can be implemented later.

julienschmidt · 2017-10-17T05:33:08Z

How would you distinguish BLOB and TEXT?
AFAIK we currently treat both just as a sequence of bytes on the driver level.

And regarding usage: Wherever not all required information about the db schema is known at compile-time. Think for example of ORMs or applications like phpMyAdmin (goMyAdmin?).

methane · 2017-10-17T05:54:16Z

How would you distinguish BLOB and TEXT?

charsetnr can be used for it. It is 63 for binary columns.
https://github.com/PyMySQL/PyMySQL/blob/dc01c6e7cd06cf523d67140b26afea365c130192/pymysql/connections.py#L1502-L1507

julienschmidt · 2017-10-17T06:23:31Z

I mean, how would we treat it differently?

methane · 2017-10-17T06:29:19Z

DatabaseType() returns TEXT or VARCHAR instead of BLOB or VARBINARY.

julienschmidt · 2017-10-17T12:26:56Z

@methane I'd suggest we implement that in another PR. The MySQL server also seems to treat them the same internally.
I'd also replace the map with a case-switch construct, in where it would be easier to making this further differentiation.

methane · 2017-10-17T12:39:28Z

I agree with you.
We can change it after see feedback from users.

LGTM for now.

molon · 2017-10-17T15:59:07Z

fields.go

-			return scanTypeString
-		}
-		return scanTypeNullString
+		return scanTypeRawBytes


Do I have to check nullable to decide converting to sql.NullString or string myself?

sql.RawBytes can also be nil, which is distinguishable from an empty string (""). I.e. if rawBytes == nil then it was a NULL value.
If you want to known if the column itself may contain NULL values, then you should check ColumnType.Nullable()

* rows: implement driver.RowsColumnTypeScanType Implementation for time.Time not yet complete! * rows: implement driver.RowsColumnTypeNullable * rows: move fields related code to fields.go * fields: use NullTime for nullable datetime fields * fields: make fieldType its own type * rows: implement driver.RowsColumnTypeDatabaseTypeName * fields: fix copyright year * rows: compile time interface implementation checks * rows: move tests to versioned driver test files * rows: cache parseTime in resultSet instead of mysqlConn * fields: fix string and time types * rows: implement ColumnTypeLength * rows: implement ColumnTypePrecisionScale * rows: fix ColumnTypeNullable * rows: ColumnTypes tests part1 * rows: use keyed composite literals in ColumnTypes tests * rows: ColumnTypes tests part2 * rows: always use NullTime as ScanType for datetime * rows: avoid errors through rounding of time values * rows: remove parseTime cache * fields: remove unused scanTypes * rows: fix ColumnTypePrecisionScale implementation * fields: sort types alphabetical * rows: remove ColumnTypeLength implementation for now * README: document ColumnType Support

Latest release for mysql driver, 1.3, does not support ColumnType.DatabaseTypeName. See go-sql-driver/mysql#667 Signed-off-by: Carlos Martín <[email protected]>

julienschmidt added enhancement Go1.8 labels Sep 18, 2017

julienschmidt added this to the v1.4 milestone Sep 18, 2017

julienschmidt self-assigned this Sep 18, 2017

julienschmidt mentioned this pull request Sep 18, 2017

Add support for the ColumnTypeNullable interface. #664

Closed

5 tasks

arnehormann reviewed Sep 18, 2017

View reviewed changes

julienschmidt force-pushed the columntype branch from 01e7b59 to 925d497 Compare September 29, 2017 22:27

julienschmidt added 11 commits October 3, 2017 18:09

rows: implement driver.RowsColumnTypeScanType

4f5c0b7

Implementation for time.Time not yet complete!

rows: implement driver.RowsColumnTypeNullable

0950d1b

rows: move fields related code to fields.go

2f97a23

fields: use NullTime for nullable datetime fields

1b786bd

fields: make fieldType its own type

571f082

rows: implement driver.RowsColumnTypeDatabaseTypeName

b6124b5

fields: fix copyright year

3ed8bb2

rows: compile time interface implementation checks

1820148

rows: move tests to versioned driver test files

0570286

rows: cache parseTime in resultSet instead of mysqlConn

3240650

fields: fix string and time types

163ddcd

julienschmidt force-pushed the columntype branch from 925d497 to 163ddcd Compare October 3, 2017 16:10

julienschmidt added 9 commits October 4, 2017 20:20

rows: implement ColumnTypeLength

91e72b0

rows: implement ColumnTypePrecisionScale

6a18c41

rows: fix ColumnTypeNullable

0a5e4cb

rows: ColumnTypes tests part1

2042d73

rows: use keyed composite literals in ColumnTypes tests

5dc4b61

rows: ColumnTypes tests part2

bb35faa

rows: always use NullTime as ScanType for datetime

b1a9d25

rows: avoid errors through rounding of time values

d03077c

rows: remove parseTime cache

65f1dfb

julienschmidt mentioned this pull request Oct 5, 2017

rows.ColumnTypes() not working ? #595

Closed

julienschmidt added 2 commits October 7, 2017 17:33

rows: remove ColumnTypeLength implementation for now

c60820c

README: document ColumnType Support

6416689

julienschmidt added the code review label Oct 12, 2017

julienschmidt requested review from methane and arnehormann October 12, 2017 19:56

julienschmidt removed the code review label Oct 16, 2017

methane approved these changes Oct 16, 2017

View reviewed changes

julienschmidt merged commit c6c4e3c into master Oct 17, 2017

julienschmidt deleted the columntype branch October 17, 2017 12:46

molon reviewed Oct 17, 2017

View reviewed changes

kwoodhouse93 mentioned this pull request Dec 19, 2017

Differentiate between BINARY/CHAR #723

Closed

carlosms mentioned this pull request Apr 25, 2018

Query endpoint src-d/gitbase-web#8

Merged

hundredwatt mentioned this pull request Jun 20, 2020

ColumnType.Length() Support #1132

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ColumnType interfaces #667

ColumnType interfaces #667

julienschmidt commented Sep 18, 2017 •

edited

Loading

julienschmidt commented Sep 18, 2017

arnehormann Sep 18, 2017 •

edited

Loading

methane Sep 19, 2017

methane Sep 19, 2017

arnehormann Sep 18, 2017

arnehormann Sep 18, 2017

julienschmidt commented Oct 7, 2017

arnehormann commented Oct 7, 2017 via email

julienschmidt commented Oct 7, 2017

methane commented Oct 12, 2017

methane commented Oct 16, 2017

julienschmidt commented Oct 17, 2017

methane commented Oct 17, 2017

julienschmidt commented Oct 17, 2017

methane commented Oct 17, 2017

julienschmidt commented Oct 17, 2017

methane commented Oct 17, 2017

molon Oct 17, 2017

julienschmidt Oct 17, 2017 •

edited

Loading

ColumnType interfaces #667

ColumnType interfaces #667

Conversation

julienschmidt commented Sep 18, 2017 • edited Loading

Description

Checklist

julienschmidt commented Sep 18, 2017

arnehormann Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

methane Sep 19, 2017

Choose a reason for hiding this comment

methane Sep 19, 2017

Choose a reason for hiding this comment

arnehormann Sep 18, 2017

Choose a reason for hiding this comment

arnehormann Sep 18, 2017

Choose a reason for hiding this comment

julienschmidt commented Oct 7, 2017

arnehormann commented Oct 7, 2017 via email

julienschmidt commented Oct 7, 2017

methane commented Oct 12, 2017

methane commented Oct 16, 2017

julienschmidt commented Oct 17, 2017

methane commented Oct 17, 2017

julienschmidt commented Oct 17, 2017

methane commented Oct 17, 2017

julienschmidt commented Oct 17, 2017

methane commented Oct 17, 2017

molon Oct 17, 2017

Choose a reason for hiding this comment

julienschmidt Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

julienschmidt commented Sep 18, 2017 •

edited

Loading

arnehormann Sep 18, 2017 •

edited

Loading

julienschmidt Oct 17, 2017 •

edited

Loading