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

adds implementation of driver.RowsNextResultSet interface #57

Merged
merged 3 commits into from Jul 8, 2019

Conversation

basvanbeek
Copy link
Contributor

Go's database/sql package supports multiple resultsets.

To enable this feature the driver.RowsNextResultSet interface must be implemented by the database driver. This PR adds the implementation for this.

@abradley201
Copy link
Contributor

Hi basvanbeek,

If you haven't done so already, please review the CLA on this page:
https://github.com/VoltDB/voltdb-client-go/blob/master/CONTRIBUTING.md

Before we merge any commits, all contributors must sign and submit the CLA form to support@voltdb.com. Thank you.

@basvanbeek
Copy link
Contributor Author

Hi @abradley201, already signed and returned the CLA some time ago. If you look at the repo stats you'll notice there is already contributed code of mine merged previously.

@abradley201
Copy link
Contributor

abradley201 commented Oct 1, 2018

Thank you. We will review the code for merging soon.
-Andrew

@benjaminballard
Copy link
Contributor

Hi Bas, yes, we're all set on your CLA. We have it on file.

@basvanbeek
Copy link
Contributor Author

@benjaminballard @abradley201 is there something I can do to expedite this?

@basvanbeek
Copy link
Contributor Author

@benjaminballard friendly ping... I'd rather use the official package instead of a fork but need this for our production code.

@benjaminballard
Copy link
Contributor

I've asked someone in Engineering to look at this today.

@@ -148,7 +148,7 @@ func (c *Conn) QueryTimeout(query string, args []driver.Value, timeout time.Dura
case resp := <-pi.responseCh:
switch e := resp.(type) {
case VoltRows:
return e, nil
return &e, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why you need to change the return type to pointer here?
If so, function return types also need update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I needed to add func (vr *VoltRows) NextResultSet() error which needs to change the current table index. Function return type does not need an update as it is an interface itself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. But I assume the the query result would be stored to some local variable before issue the NextResultSet() method, right? So that nextResultSet still be able to call upon that variable? i.e

rows, err := conn.Query(....)
for rows.HasNextResultSet() {
      .....
      rows.NextResultSet()  // x.m() is shorthand for (&x).m() ? https://golang.org/ref/spec#Calls
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So yes Go has automatic dereferencing of pointers but that's not the issue here.

The problem is that because we must update vr.tableIndex I must use a pointer receiver in the NextResultSet method (func (vr *VoltRows) NextResultSet()). If purely working on the VoltRows object itself this is no issue. However VoltRows needs to implement driver.RowsNextResultSet as this is the enhancement interface sql/database tests against for
this logic. Here lies the problem, if providing the composite literal VoltRows{} instead of a pointer to it &VoltRows{}; it will not satisfy the interface and sql/database will assume the driver does not support multiple result sets and NextResultSet will always return false.

I've added a unit test so this can easily be demonstrated as well as a compile time assertion for the driver.RowsNextResultSet interface. Remove the & and see stuff fail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, that make sense. Thanks for the detailed explanation.

voltdbclient/rows.go Outdated Show resolved Hide resolved
@benjaminballard benjaminballard merged commit 91d980e into VoltDB:master Jul 8, 2019
@basvanbeek basvanbeek deleted the rows-next-result-set branch July 16, 2019 11:17
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.

None yet

4 participants