implement ExecContext #16

nineinchnick · 2020-12-23T10:41:10Z

This PR implements driver.ExecContext() to be able to run queries like USE <catalog>.<schema> and possibly other DDL/DML queries. While this is rarely used in application code, I noticed this when trying to use the usql CLI client.

I also included few minor changes to satisfy golangci-lint linter, like adding comments and removing dead code.

When working on this I was reading https://github.com/prestosql/presto/blob/master/presto-jdbc/src/main/java/io/prestosql/jdbc/PrestoStatement.java as a reference.

cla-bot · 2020-12-23T10:41:12Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/prestosql/cla.

losipiuk · 2020-12-23T11:04:56Z

Thanks @nineinchnick!

I also included few minor changes to satisfy golangci-lint linter, like adding comments and removing dead code.

Can you please split PR into multiple commits so logically separate changes are in separate commits?

As for the "adding comments" part. I do not believe we want all of those. I know that some notable golang projects require every public member to be documented. But I strongly believe that it does not improve code quality. Rather does the opposite. We should use common sense and add comments where they are useful.

cla-bot · 2020-12-23T11:27:19Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/prestosql/cla.

nineinchnick · 2020-12-23T11:30:10Z

Can you please split PR into multiple commits so logically separate changes are in separate commits?

Done!

As for the "adding comments" part. I do not believe we want all of those. I know that some notable golang projects require every public member to be documented. But I strongly believe that it does not improve code quality. Rather does the opposite. We should use common sense and add comments where they are useful.

Fully agree, I tried to make them as relevant as possible.

presto/presto_test.go

presto/presto.go

presto/presto_test.go

presto/presto.go

losipiuk · 2020-12-23T13:26:41Z

presto/presto.go

+				if catalog != "" {
+					c.httpHeaders.Set(prestoCatalogHeader, catalog)
+				}
+				schema := resp.Header.Get(prestoSetSchemaHeader)


should we handle X-Set-Path here too?

I'm not sure. It's not set in the initial connection. In what cases the server would send it and expect it to be set in future requests?

LMK if you think we should add this for consistency.

Actually there are more of those:

https://github.com/trinodb/trino/blob/83bcdffc4c8dd0ce377c5f12609c69b9088ba6a0/presto-client/src/main/java/io/prestosql/client/PrestoHeaders.java#L27-L32

I think we do not have to support all of those initially. But we should at least notice that they are present in the response and return an error to the caller, if we get an unsupported X-Prest-Set-... header. Otherwise we will pretend that we support SET SESSION ... syntax, but we would not send set session properties in followup queries.

Could you please add validation and test coverage?

Done! Added this as a separate commit.

losipiuk

Thanks. Mostly looks good. Some cleanup comments.

And also please capitalize commit messages.

cla-bot · 2020-12-23T14:50:53Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/prestosql/cla.

cla-bot · 2020-12-29T16:30:46Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

cla-bot · 2020-12-29T16:50:46Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

losipiuk

Awesome. Minor comments.

trino/trino.go

losipiuk · 2020-12-31T11:49:02Z

trino/trino_test.go

@@ -321,6 +321,23 @@ func TestExec(t *testing.T) {
 	}
 }

+func TestUnsupportedHeader(t *testing.T) {


Can you also add and integration test calling out to

SET SESSION grouped_execution=true

SET ROLE dummy

SET PATH dummy

RESET SESSION grouped_execution (not sure if this one will work; may not send X-Presto-Clear-Session header if it was not set before, which we cannot do)

Done! Actually only SET PATH failed since there's some client capability negotiation mechanism, this client doesn't report any capabilities and setting PATH is not allowed by default.

trino/trino_test.go

cla-bot · 2020-12-31T12:24:43Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

losipiuk · 2021-01-04T10:51:45Z

@cla-bot check

cla-bot · 2021-01-04T10:51:47Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

cla-bot · 2021-01-04T10:51:48Z

The cla-bot has been summoned, and re-checked this pull request!

losipiuk · 2021-01-12T15:07:58Z

/cla-bot check

losipiuk · 2021-01-12T15:08:14Z

@cla-bot check

cla-bot · 2021-01-12T15:08:16Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

cla-bot · 2021-01-12T15:08:17Z

The cla-bot has been summoned, and re-checked this pull request!

nineinchnick · 2021-01-15T06:09:23Z

@cla-bot check

cla-bot · 2021-01-15T06:09:25Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

cla-bot · 2021-01-15T06:09:26Z

The cla-bot has been summoned, and re-checked this pull request!

nineinchnick · 2021-01-15T06:40:51Z

@cla-bot check

cla-bot · 2021-01-15T06:40:53Z

The cla-bot has been summoned, and re-checked this pull request!

losipiuk · 2021-01-15T08:41:52Z

Merged, thanks!