[Ruby]: MULTI statement support

[paarthmadan]

Closes #34

This PR adds MULTI statement support on the Trilogy Ruby binding.

API

After some iteration, I think the nicest interface for multi statement is introducing two methods:

1. A predicate method (ie. more_results_exist?, name subject to change)
2. A next_result cursor method which iterates through result sets

We could boil this down to a single next_result method and let clients use the nullability (or error response) of next_result to infer stoppage, but I think a predicate method better appeals to how Ruby is written.

I've also considered:

1. Exposing a method which returns an enumerator that yields a new result set
2. Automatically querying for subsequent result sets should they exist and collecting them into an array of results to be returned as the result of the call to query. This would be a breaking change as the result of trilogy_query would now be an array.

I started with this API because it's simplest and grants the user the most control. With that said, if we take the stance that the binding should do the heavy lifting on behalf of the user, a solution like (2) might work out better.

Error Flow

A key decision with this API is how and when errors are handled.

more_results_exist? is currently a non-destructive method that will always resolve to a boolean. It will check the server status flag to see if there are more results and return true or false accordingly.

next_result, on the other hand can return: nil, a result set, or raise an error. It will return nil if the server doesn't have any more results (ie. it's idempotent). It will return a result set should one exist, though acquiring the response may result in an error which will be raised and forwarded to the user.

For reviewers

I'd love to receive feedback on

1. The proposed API
2. Quality of the C code: my C skills are fairly rusty, particularly in the context of Ruby C-extensions. Feel free to nitpick on anything: code-structure, naming, conventions etc.
3. Naming

[adrianna-chang-shopify]

Really excited to see this work getting started, well done Paarth! 😍

I can't offer too much feedback on the C code, unfortunately. My only comment was going to be that there's some duplication between rb_trilogy_query and rb_next_result, but sounds like you've already got a plan to tackle that 👍

Some more general comments:

• Extremely pedantic, but naming: I'm wondering if we ought to name the methods #next_result? and #get_next_result so that they're more symmetrical. #more_results_exist? is a better description of what the method is doing, but I find it a bit verbose 😅 I could see something like

while client.next_result? do
  client.get_next_result
end

working. Anyways, this is trivial, just wanted to make note.

• Can we add tests for:
  • Calling #next_result when there are no more results to be iterated over?
  • Calling #more_results_exist? when one of the results has an error (if we're matching Mysql2 we should raise, which I believe we're already doing)
  • Calling #next_result when one of the results has an error (if we're matching Mysql2 we should return nil)

[adrianna-chang-shopify]

Mysql2's #store_result method returns nil when called in the case where the client has enumerated all of the results. I think it makes sense to match this behaviour.

[adrianna-chang-shopify]

Actually, a follow up question: I'm assuming ctx->conn.server_status & TRILOGY_SERVER_STATUS_MORE_RESULTS_EXISTS means "the server status is non-zero AND we have more results available". And if I'm understanding correctly, the situation in this conditional is the one where we've called #next_result without more results existing.

Should this conditional be if ctx->conn.server_status & !TRILOGY_SERVER_STATUS_MORE_RESULTS_EXISTS instead, ie. "if the server status is non-zero AND we have no more results, then return nil"?

[paarthmadan]

You're right in that what we're after is returning nil if #next_result is called despite the server not having any more results.

I think perhaps you've misinterpreted the bitwise AND (&) operator for a boolean AND operation instead.

In this case, ctx->conn.server_status is an unsigned 16-bit integer which can be encoded with a series of SERVER_STATUS integers through bitwise OR'ing (|) them into the status.

ctx->conn.server_status & TRILOGY_SERVER_STATUS_MORE_RESULTS_EXISTS

This conditional is effectively checking if TRILOGY_SERVER_STATUS_MORE_RESULTS_EXISTS (0x0008) is present in the server_status because it will only return a non-zero result if those bits are flipped on in the status.

Then, we negate this to effectively check if it wasn't present, which is what we're after.

[adrianna-chang-shopify]

Ah, yep that makes sense! I indeed misread that line 🤦‍♀️ I didn't realize that ctx->conn.server_status was the result of bitwise OR'ing SERVER_STATUS flags, and I was thrown off by what ctx->conn.server_status was representing.

Thanks for elaborating! ❤️

[paarthmadan]

Sounds good @adrianna-chang-shopify, for now I'll add some more test coverage and work on the refactoring of how these methods work internally.

I'm going to defer the renaming bit until we have more folks chime in, because naming is usually a contentious point. That said, I hear your argument and I'm happy to circle back on that 👍

[paarthmadan]

You're right in that what we're after is returning nil if #next_result is called despite the server not having any more results.

I think perhaps you've misinterpreted the bitwise AND (&) operator for a boolean AND operation instead.

In this case, ctx->conn.server_status is an unsigned 16-bit integer which can be encoded with a series of SERVER_STATUS integers through bitwise OR'ing (|) them into the status.

ctx->conn.server_status & TRILOGY_SERVER_STATUS_MORE_RESULTS_EXISTS

This conditional is effectively checking if TRILOGY_SERVER_STATUS_MORE_RESULTS_EXISTS (0x0008) is present in the server_status because it will only return a non-zero result if those bits are flipped on in the status.

Then, we negate this to effectively check if it wasn't present, which is what we're after.

[HParker]

I tested this change with with the flag disabled and found no regressions.

I like the current API & the naming matches the names discussed in the issue so I think the current API is the right choice in this case. as to the alternatives that you considered, I believe you made the right choice. perhaps an enumerator API could be exposed at a different level, but here it seems inappropriate.

There might be future work we can do here after more people play with it, but I like the implementation enough I am confident we can build on top of this with at most minor tweaks.