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

WIP on constraintSetGroups #99

Merged
merged 8 commits into from
Dec 12, 2022
Merged

WIP on constraintSetGroups #99

merged 8 commits into from
Dec 12, 2022

Conversation

tpolecat
Copy link
Member

@tpolecat tpolecat commented Sep 7, 2022

This adds Query/constraintSetGroup and a pretty-printer for queries.

@tpolecat tpolecat marked this pull request as draft September 7, 2022 13:45
@mergify mergify bot added the service label Sep 7, 2022
@@ -23,6 +23,7 @@ val postgresVersion = "42.5.1"
val skunkVersion = "0.3.2"
val lucumaSsoVersion = "0.4.4"
val testcontainersScalaVersion = "0.40.12"
val paigesVersion = "0.4.2"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is for the GraphQL query pretty-printer, which we should upstream to Grackle once we're sure we like it. I added code to the main mapping to print the query out on each request.

@@ -0,0 +1,3 @@
-- https://stackoverflow.com/questions/54372666/create-an-immutable-clone-of-concat-ws/54384767#54384767
CREATE OR REPLACE FUNCTION immutable_concat_ws(text, VARIADIC text[])
RETURNS text AS 'text_concat_ws' LANGUAGE internal IMMUTABLE PARALLEL SAFE;
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm using a generated column below, which is defined as a pure expression based on other fields in the row. I need to use the text_concat_ws function but it's not declared immutable for some reason . This basically just redeclares it with the right modifier.

coalesce(c_hour_angle_max::text, 'null')
)
) stored,

Copy link
Member Author

Choose a reason for hiding this comment

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

To group observations by conditions I concatenate all the relevant fields together and use that as the key. I need to do this so the constraint set group has a key in the obs table that it can join with.

@@ -4699,6 +4695,9 @@ type Query {

# Selects the first `LIMIT` matching observations based on the provided `WHERE` parameter, if any.
observations(

programId: ProgramId
Copy link
Member Author

Choose a reason for hiding this comment

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

At some point we decided that top-level queries for observations needed to specify the program id. I think we should get rid of these anyway and make the user traverse down to the program.

@@ -5500,8 +5499,6 @@ type TargetEnvironmentGroupSelectResult {
}

type TargetGroup {
# IDs of observations that use the same constraints
observationIds: [ObservationId!]!
Copy link
Member Author

Choose a reason for hiding this comment

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

This wasn't being used in the client and it's redundant with observations/matches/id so I removed it (same with other group types).

@@ -52,6 +54,9 @@ object GraphQLRoutes {
{
for {
user <- OptionT(client.get(a))
// If the user has never hit the ODB using http then there will be no user
// entry in the database. So go ahead and [re]canonicalize here to be sure.
_ <- OptionT.liftF(userSvc.canonicalizeUser(user))
Copy link
Member Author

Choose a reason for hiding this comment

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

I ran into this issue when we tested with Explore, which never hits the server via plain HTTP and thus never went through the normal user-canonicalizing code.

// Override `fetch` to log the query. This is optional.
// Override `defaultRootCursor` to log the GraphQL query. This is optional.
override def defaultRootCursor(query: Query, tpe: Type, env: Env): F[Result[(Query, Cursor)]] =
Logger[F].info("\n\n" + PrettyPrinter.query(query).render(100) + "\n") *> super.defaultRootCursor(query, tpe, env)
Copy link
Member Author

Choose a reason for hiding this comment

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

As with the override of fetch below this just prints stuff out and you can comment it out if it bothers you.

import org.typelevel.paiges.Doc.text

/** Paiges combinators for Grackle queries. */
object PrettyPrinter {
Copy link
Member Author

Choose a reason for hiding this comment

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

This has turned out to be helpful for debugging. We can upstream it once we're happy with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, nice.

@tpolecat tpolecat marked this pull request as ready for review December 12, 2022 18:01
@tpolecat
Copy link
Member Author

Ok this is ready for review, at long last.

Copy link
Contributor

@swalker2m swalker2m left a comment

Choose a reason for hiding this comment

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

Excellent, I never would have figured all of that out. 👍

import org.typelevel.paiges.Doc.text

/** Paiges combinators for Grackle queries. */
object PrettyPrinter {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, nice.

@tpolecat tpolecat merged commit 0714c08 into main Dec 12, 2022
@tpolecat tpolecat deleted the constraintSetGroups branch December 12, 2022 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants