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

Grant/bru-1049 #16

Merged
merged 9 commits into from
Mar 29, 2024
Merged

Grant/bru-1049 #16

merged 9 commits into from
Mar 29, 2024

Conversation

glpierce
Copy link
Contributor

@glpierce glpierce commented Mar 28, 2024

Implement table relations creation.

Copy link
Contributor

@tevonsb tevonsb left a comment

Choose a reason for hiding this comment

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

Requested small changes, let's discuss what to do with multiple schemata, any thoughts?

@@ -2,3 +2,5 @@ export * from "./database";
export * from "./table";
export * from "./user-query";
export * from "./postgres-adapter";
export * from "./relation";
export * from "./pipeline";
Copy link
Contributor

Choose a reason for hiding this comment

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

We likely want to change the name pipeline given this new context. What do you think? Is there clearly better name or is this good enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some options:

  • JsonQuery
  • StepQuery
  • PreQuery
  • PreSQL
  • ProtoSQL
  • ProtoQuery


// A relation used in a pipeline. Includes the API path to the relation from the base object defintiion and the object definition ID of the related object defintion
export const InferredSchemaRelationSchema = z.object({
api_path: z.string(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is API path still relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not. Additional changes to these schemas and types (including removing unneeded ones) will be made as part of the query builder PR.

// A relation used in a pipeline. Includes the API path to the relation from the base object defintiion and the object definition ID of the related object defintion
export const InferredSchemaRelationSchema = z.object({
api_path: z.string(),
object_definition_id: z.string(),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is definitely not relevant anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. We'll have a new object for inferred schema relations as part of query builder.

assert(database, "Database not found");

try {
const result = await client.query(
Copy link
Contributor

Choose a reason for hiding this comment

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

May be worth commenting this, at least what the expected Val is cause it's pretty impossible to tell just by reading it

tc.table_name,
tc.constraint_type,
kcu.column_name;`,
[database.schema || "public"],
Copy link
Contributor

Choose a reason for hiding this comment

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

we'll need to update this since we're using multiple schema now.

Let's talk about this.

@glpierce glpierce changed the base branch from master to query-builder March 29, 2024 16:51
@glpierce glpierce merged commit 971a27a into query-builder Mar 29, 2024
2 checks passed
@glpierce glpierce deleted the grant/bru-1049 branch March 29, 2024 16:52
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.

3 participants