Skip to content
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

add a case in Next scan to fix issue #190 and issue #316 #468

Closed
wants to merge 1 commit into from

Conversation

gingerhot
Copy link

@gingerhot gingerhot commented Oct 16, 2017

I get a same error as in #190 and #316 when I select a nullable datetime typed column created_at in one table:

SELECT created_at FROM users;

then I use COALESCE and CAST to give it a default value and cast it as text:

SELECT cast(coalesce(created_at, '2017-10-16T08:23:19.120Z') as text) as created_at FROM users;

and I get the same error.

So I add a case to the Next() scan to give datetime typed column a chance to be parsed. After that issue #190/#316 and my problem are all fixed.

I'm not sure if this's the right approach to the problem, here's twice benchmark results run on my computer:

  1. current master version's:
 ~/go/src/github.com/mattn/go-sqlite3 (master) $ go test
BenchmarkExec           200000     151258 req/s
BenchmarkQuery          100000      60628 req/s
BenchmarkParams         100000      55500 req/s
BenchmarkStmt           100000      74674 req/s
BenchmarkRows             2000       1995 req/s
BenchmarkStmtRows         3000       2041 req/s
PASS
ok  	github.com/mattn/go-sqlite3	22.608s
 ~/go/src/github.com/mattn/go-sqlite3 (master) $ go test
BenchmarkExec           200000     134008 req/s
BenchmarkQuery          100000      55242 req/s
BenchmarkParams         100000      56315 req/s
BenchmarkStmt           100000      74565 req/s
BenchmarkRows             3000       2034 req/s
BenchmarkStmtRows         3000       2033 req/s
PASS
ok  	github.com/mattn/go-sqlite3	23.432s
  1. the version of this pull request:
~/go/src/github.com/goonr/go-sqlite3 (master) $ go test
BenchmarkExec           200000     127702 req/s
BenchmarkQuery          100000      57204 req/s
BenchmarkParams         100000      48265 req/s
BenchmarkStmt           100000      73005 req/s
BenchmarkRows             3000       1994 req/s
BenchmarkStmtRows         3000       2034 req/s
PASS
ok  	github.com/goonr/go-sqlite3	23.628s
~/go/src/github.com/goonr/go-sqlite3 (master) $ go test
BenchmarkExec           200000     147327 req/s
BenchmarkQuery          100000      52995 req/s
BenchmarkParams         100000      42885 req/s
BenchmarkStmt           100000      64088 req/s
BenchmarkRows             2000       1990 req/s
BenchmarkStmtRows         3000       2054 req/s
PASS
ok  	github.com/goonr/go-sqlite3	23.691s

Thanks~

@coveralls
Copy link

coveralls commented Oct 16, 2017

Coverage Status

Coverage increased (+0.2%) to 66.931% when pulling 9c8272d on goonr:master into 5160b48 on mattn:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 66.931% when pulling 9c8272d on goonr:master into 5160b48 on mattn:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 66.931% when pulling 9c8272d on goonr:master into 5160b48 on mattn:master.

@mattn
Copy link
Owner

mattn commented Oct 16, 2017

This breaks compatibility. And when the text is likely date format (but not datetime), it will be confusing.

@gingerhot
Copy link
Author

gingerhot commented Oct 16, 2017

About the compatibility, I think maybe or maybe not. Because when you scan a struct that has a nullable timestamp attribute directly, there will be an error as same as in #190, you must do some trick, so now you can remove those tricks.

And for the date format likely text, it would really make confusing. But maybe the user can avoid it by defining such a column as a not NULL field. Anyway, I think it's a problem.

@mattn
Copy link
Owner

mattn commented Nov 21, 2017

Sorry, I'll close this since type of datetime('now') is string and I can not fix this without hack. If you want to scan it as time.Time, Please use sql.Scanner. #190 (comment)

@mattn mattn closed this Nov 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants