-
-
Notifications
You must be signed in to change notification settings - Fork 25
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
use gms engine instead of mysql handler #677
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! A lot of work, but I definitely think it'll pay off for us!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an improvement, in that it puts more of the basic wire protocol in our control, rather than relying on the underlying GMS handler that Dolt instantiates. What bothers me: we're still instantiating a GMS handler on the Dolt side, which we use here or not depending on a bunch of factors.
If we've outgrown the Handler interface and need to do our own thing for better accuracy, that's great. But this only gets us partway there. Ideally Dolt no longer installs a Handler at all, and uses ours instead (or knows nothing about it). Next best thing would be that Dolt installs a Handler but we don't use it at all. This PR is at least 2 steps removed from that ideal.
I'm fine with this directionally, but we need to move quickly to a single code path managed by this package with clearly demarcated lines of responsibility, and this doesn't get us there. This makes the interaction between the packages substantially harder to reason about. Who owns getting us a cleaner set of abstractions and separation of concerns? You, or somebody else? The last major revision to this set of interactions (me, to support prepared statements) introduced a bunch of new abstractions and changes in the GMS layer to cope with. I expected to see some of that here, or the outline for a plan, or something, and I don't. It's likely that we need to move some of the functionality in the GMS Handler to a different layer of abstraction so that it can be re-used in both places that need it.
Other than that primary concern, the fact that some of the tests are now using string values instead of numerics is concerning to me. I don't see anywhere in the testing framework code explaining why this should be the case.
I don't want to block forward progress on this work, but I do need to know there is a plan to clean up the issues above and that this isn't the final state of this interaction. Daylon's OK on the overall PR for correctness etc. is fine by me.
Regressions:date
numeric
Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the changes
No description provided.