Add Multi-Results support #537

Merged
merged 4 commits into from Mar 23, 2017

Conversation

Projects
None yet
5 participants
@jszwec
Contributor

jszwec commented Jan 10, 2017

  • Implemented driver.RowsNextResultSet interface

Fixes #420

@methane

This comment has been minimized.

Show comment
Hide comment
@methane

methane Jan 10, 2017

Contributor

Does this patch support query like this?

db.Query("DO 42; SELECT 42")

How about removing emptyRows completely?

Contributor

methane commented Jan 10, 2017

Does this patch support query like this?

db.Query("DO 42; SELECT 42")

How about removing emptyRows completely?

@jszwec

This comment has been minimized.

Show comment
Hide comment
@jszwec

jszwec Jan 10, 2017

Contributor

Does this patch support query like this?

I double checked, currently it works only with selects, I will fix it.

How about removing emptyRows completely?

Good point, thanks!

Contributor

jszwec commented Jan 10, 2017

Does this patch support query like this?

I double checked, currently it works only with selects, I will fix it.

How about removing emptyRows completely?

Good point, thanks!

Multi-Results improvements
- support for binary protocol
- support statements returning no results
- remove emptyRows
+ }
+
+ if rows.NextResultSet() {
+ dbt.Error("unexpected next result set")

This comment has been minimized.

@methane

methane Jan 15, 2017

Contributor

Why next result set is unexpected?
What happens for DO 1; DO 2; DO 3; SELECT 42?
I expect

if !rows.NextResultSet() { dbt.Error("expect 4 resultset, got 1") }
if !rows.NextResultSet() { dbt.Error("expect 4 resultset, got 2") }
if !rows.NextResultSet() { dbt.Error("expect 4 resultset, got 3") }
var v int64
if err := rows.Scan(&v); err != nil {
    dbt.Error(err)
}
if v != 42 {
   dbt.Error("expected 42; got ", v)
}
@methane

methane Jan 15, 2017

Contributor

Why next result set is unexpected?
What happens for DO 1; DO 2; DO 3; SELECT 42?
I expect

if !rows.NextResultSet() { dbt.Error("expect 4 resultset, got 1") }
if !rows.NextResultSet() { dbt.Error("expect 4 resultset, got 2") }
if !rows.NextResultSet() { dbt.Error("expect 4 resultset, got 3") }
var v int64
if err := rows.Scan(&v); err != nil {
    dbt.Error(err)
}
if v != 42 {
   dbt.Error("expected 42; got ", v)
}

This comment has been minimized.

@jszwec

jszwec Jan 15, 2017

Contributor

I followed the example from the std:
https://tip.golang.org/pkg/database/sql/#example_DB_Query_multipleResultSets

which seems to ignore the empty results sets.

@jszwec

jszwec Jan 15, 2017

Contributor

I followed the example from the std:
https://tip.golang.org/pkg/database/sql/#example_DB_Query_multipleResultSets

which seems to ignore the empty results sets.

This comment has been minimized.

@methane

methane Jan 15, 2017

Contributor

Ok, I got it.

@methane

methane Jan 15, 2017

Contributor

Ok, I got it.

This comment has been minimized.

@joegrasse

joegrasse Jan 24, 2017

So what would happen if I have SELECT 1 FROM DUAL WHERE 1 = 0; SELECT 1,2 FROM DUAL WHERE 1 = 1;? That is the first query returns no rows, but the second query does.

I would expect

if !rows.NextResultSet() { dbt.Error("expect 2 resultset, got 1") }

That is, I might not know which query has no resultset. So, I have to go through them all to map processing them correctly.

@joegrasse

joegrasse Jan 24, 2017

So what would happen if I have SELECT 1 FROM DUAL WHERE 1 = 0; SELECT 1,2 FROM DUAL WHERE 1 = 1;? That is the first query returns no rows, but the second query does.

I would expect

if !rows.NextResultSet() { dbt.Error("expect 2 resultset, got 1") }

That is, I might not know which query has no resultset. So, I have to go through them all to map processing them correctly.

This comment has been minimized.

@jszwec

jszwec Jan 25, 2017

Contributor

@joegrasse, sorry - "empty results sets" was inaccurate. Result set is present if column count > 0.
So, in the example that you provided it would work as you expected - it would return two result sets, first would be empty, the other one would have one row.

@jszwec

jszwec Jan 25, 2017

Contributor

@joegrasse, sorry - "empty results sets" was inaccurate. Result set is present if column count > 0.
So, in the example that you provided it would work as you expected - it would return two result sets, first would be empty, the other one would have one row.

@methane

Query() and NextResultSet() has very similar code.
It seems it can be exported to method later.
But it's OK for first implementation.

LGTM, except recursion.
Go optimimze tail call only in very limited situation.
I prefer simple loop over tail call recursion.

rows.go
+ if resLen == 0 {
+ rows.rs.done = true
+ return rows.NextResultSet()
+ }

This comment has been minimized.

@methane

methane Jan 15, 2017

Contributor

I don't like recursion.
Would you rewrite this to loop?

@methane

methane Jan 15, 2017

Contributor

I don't like recursion.
Would you rewrite this to loop?

This comment has been minimized.

@jszwec

jszwec Jan 15, 2017

Contributor

sure, I just pushed a fix

@jszwec

jszwec Jan 15, 2017

Contributor

sure, I just pushed a fix

rows.go
+
+ if resLen == 0 {
+ rows.rs.done = true
+ return rows.NextResultSet()

This comment has been minimized.

@methane

methane Jan 15, 2017

Contributor

same.

@methane

methane Jan 15, 2017

Contributor

same.

jszwec added some commits Jan 15, 2017

Multi-Results: fix hanging rows.Close()
* rows.Close() would hang on readUntilEOF if some results were ignored before calling NextResultSet()
Multi-Results: review suggestion
- get rid of a recursive call
@jrallison

This comment has been minimized.

Show comment
Hide comment
@jrallison

jrallison Feb 24, 2017

Any chance of getting this merged soon now that go1.8 has been released (which adds support for this)?

Thanks!

jrallison commented Feb 24, 2017

Any chance of getting this merged soon now that go1.8 has been released (which adds support for this)?

Thanks!

@julienschmidt julienschmidt added this to the v1.4 milestone Mar 17, 2017

@methane methane merged commit ffa70d4 into go-sql-driver:master Mar 23, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@methane

This comment has been minimized.

Show comment
Hide comment
@methane

methane Mar 23, 2017

Contributor

This cause Travis test fail: https://travis-ci.org/methane/mysql/jobs/214213219
@jszwec any ideas?

Contributor

methane commented Mar 23, 2017

This cause Travis test fail: https://travis-ci.org/methane/mysql/jobs/214213219
@jszwec any ideas?

@methane

This comment has been minimized.

Show comment
Hide comment
@methane

methane Mar 23, 2017

Contributor

I'm sorry, it was my mistake.

Contributor

methane commented Mar 23, 2017

I'm sorry, it was my mistake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment