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

Fixed INT8 handling, support for testing statements, proper connection error handling #55

Merged
merged 1 commit into from
Nov 22, 2023

Conversation

Hydrocharged
Copy link
Collaborator

@Hydrocharged Hydrocharged commented Nov 22, 2023

This primarily implements four things:

  1. We now discard all messages until we reach a Sync message when an error occurs, which is consistent with the documentation.
  2. Proper support for boolean results. The sqllogictests make use of them, and GMS doesn't have a native boolean type, so we treat all booleans as Int32 types. You can read more in the comments.
  3. Proper error handling is the multi-message test case. Our conn.Query calls in tests only return specific kinds of errors, and all other errors are put into the returned rows. This differs from conn.Exec, which returns all errors. Previously, we never checked for an error in rows (since I never knew an error would be put there), so it was getting swallowed. We now properly handle those errors.
  4. Expanded statement support for Describe messages. I noticed in the zachmu/err-handling-repro branch that the test file testing/go/prepared_statement_test.go is inconsistent with the general test framework, as it runs the setup statements using conn.Query rather than conn.Execute. We don't support Describe at all right now, as GMS currently requires analyzing a statement to determine the output schema, and analyzing some statements produces execution side effects. As a workaround, we start a transaction, execute the query, record the schema, and rollback the transaction. This does not work for any commands that implicitly commit the transaction, so this adds another hack to fake it by running an equivalent DROP to select CREATE statements. This does cause an implicit commit, but it's better than failing for now.

@Hydrocharged Hydrocharged requested a review from zachmu November 22, 2023 13:58
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

LGTM

@zachmu
Copy link
Member

zachmu commented Nov 22, 2023

I'm gonna merge this, but notes for next round of improvements:

  1. Error handling is probably wrong for non-extended query syntax (we'll block waiting for a sync that never comes)
  2. The describe solution right now is a hot mess. We can't be execute statements just to get their result set descriptions, we need to expose analyzer capabilities to this layer. Might fix that next.

@zachmu zachmu merged commit 70844fd into main Nov 22, 2023
6 checks passed
@zachmu zachmu deleted the daylon/test-type-improvements branch November 22, 2023 18:51
@Hydrocharged
Copy link
Collaborator Author

It handles non-extended syntax as well, as all of those messages will mark that they are the end.

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.

2 participants