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

feat: allow for tests over flightsql #2240

Merged
merged 38 commits into from
Dec 13, 2023
Merged

feat: allow for tests over flightsql #2240

merged 38 commits into from
Dec 13, 2023

Conversation

universalmind303
Copy link
Contributor

@universalmind303 universalmind303 commented Dec 8, 2023

closes #2234

There's still a bit of cleanup to do, but it seems to work.

I'm planning on refactoring the test suites a bit, currently they always spin up a pg instance, which is not needed at all for the flightsql tests.

My plan is to refactor them so the following commands are accepted

> just slt 'path/to/test' # default pg behavior
> just slt 'path/to/test' --protocol=rpc # equivalent to the current `just slt 'path/to/test' --rpc-test`
> just slt 'path/to/test' --protocol=flightsql # run the tests over the flightsql wire protocol
> just slt 'path/to/test' --protocol=postgres # run the tests over the postgres wire protocol. (same as the default)

12/12 updates.

There are a few additional changes to the following files that I introduced in this PR worth highlighting.

crates/sqlexec/src/session.rs

  • rename a few methods to make the intent clearer
  • add a few new helper methods for the flightsql client

crates/glaredb/src/server.rs
There were some bigger changes in here, namely to ComputeServer. To make the tests easier to run in isolation (pg/rpc/flightsql), I decided to refactor the compute server a bit to make the postgres server optional.

Previously, we would always start up a postgres instance, now it is fully optional. You can specify what protocols you want to be exposed -- TODO: cleanup the cli args for this --


12/12 updates pt 2.

This PR really got away from me.. A lot bigger than I had originally anticipated. sorry reviewers

Updated the CLI args to allow for postgres to be optional. Just to be safe, I added a bunch of additional tests to make sure nothing broke in the process. So there are some changes to the cli args to handle the new functionality.

The only breaking change is to how you run the tests. just slt 'path/to/test' --protocol=rpc instead of just slt 'path/to/test' --rpc-test.

Copy link
Contributor

@tychoish tychoish left a comment

Choose a reason for hiding this comment

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

seems great. would think that the package upgrades should probably be handled as workspace packages.

crates/glaredb/src/server.rs Outdated Show resolved Hide resolved
crates/protogen/proto/metastore/catalog.proto Outdated Show resolved Hide resolved
@universalmind303 universalmind303 changed the title feat!: allow for tests over flightsql feat: allow for tests over flightsql Dec 12, 2023
@universalmind303 universalmind303 marked this pull request as ready for review December 12, 2023 23:21
use super::*;

#[derive(Parser, Debug)]
#[derive(Args, Debug)]
#[group(id="query_input", args = ["query", "file"], multiple = false)]
Copy link
Member

Choose a reason for hiding this comment

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

What does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can revert this as it's not really relevant to this PR, Just something i found while testing to make sure i didnt break any of the flags.

Clap has builtin macros for mutually exclusive arguments that we were previously checking manually. They give much nicer error messages, so i decided to throw this in here.

crates/glaredb/src/args/server.rs Show resolved Hide resolved
crates/glaredb/src/args/server.rs Show resolved Hide resolved
crates/glaredb/src/server.rs Outdated Show resolved Hide resolved
crates/glaredb/src/server.rs Outdated Show resolved Hide resolved
testdata/sqllogictests/functions/kdl.slt Outdated Show resolved Hide resolved
testdata/sqllogictests/functions/postgres.slt Show resolved Hide resolved
crates/testing/tests/sqllogictests/main.rs Outdated Show resolved Hide resolved
crates/testing/tests/sqllogictests/main.rs Outdated Show resolved Hide resolved
crates/rpcsrv/src/flight_handler.rs Outdated Show resolved Hide resolved
@scsmithr
Copy link
Member

Previously, we would always start up a postgres instance [...]

We should make sure to be clear what we mean by this. We're starting up a server implementing the pg protocol. By saying "postgres instance", that can lead to confusion as we're not starting up a postgres instance.

@universalmind303
Copy link
Contributor Author

Previously, we would always start up a postgres instance [...]

We should make sure to be clear what we mean by this. We're starting up a server implementing the pg protocol. By saying "postgres instance", that can lead to confusion as we're not starting up a postgres instance.

yeah sorry, i meant a postgres compatible api here.

@universalmind303 universalmind303 merged commit b63ac15 into main Dec 13, 2023
8 of 9 checks passed
@universalmind303 universalmind303 deleted the flightsql-pt2 branch December 13, 2023 17:31
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.

flightsql: add slts
3 participants