Skip to content

Auto-generated insert procedures v1 #261

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

Merged
merged 18 commits into from
Jan 19, 2024
Merged

Auto-generated insert procedures v1 #261

merged 18 commits into from
Jan 19, 2024

Conversation

soupi
Copy link
Contributor

@soupi soupi commented Jan 17, 2024

What

We'd like to auto-generate insert procedures for tables.

How

Soon, tl;dr:

  1. We add the relevant INSERT INTO sql structure to the relevant parts
  2. We generate insert procedures from the metadata information about tables and columns
  3. We expose in the scheme by creating a procedure for each generated insert procedure, and create an object type for the table's insertable object
  4. We add some custom-tables.sql to postgres so we can test other stuff not exposed by chinook

todo: add engine metadata for v1_insert_custom_dog.

Copy link
Contributor

@SamirTalwar SamirTalwar left a comment

Choose a reason for hiding this comment

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

The implementation seems fine, I just have a couple of nitpicks.

Comment on lines 113 to 116
CASE WHEN attgenerated_exists
THEN CASE WHEN attgenerated::text = 's' THEN 'isGenerated' ELSE 'notGenerated' END
ELSE 'notGenerated'
END as is_generated
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a strange way to implement booleans. Why not true and false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just liked being explicit, wouldn't mind changing it to bool if you want me to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes me uneasy because there's the potential of many values here, rather than the two we care about.

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've changed this so instead of returning isGenerated, it will return stored, so that if and when postgres adds additional values to this, it will be strictly additive.

@soupi soupi force-pushed the gil/gen-insert-v1 branch from 87308c9 to cc8b272 Compare January 18, 2024 12:54
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a v1 config snapshot! Generated because we added a new table (custom.dog).

@soupi soupi marked this pull request as ready for review January 19, 2024 09:13
Copy link
Contributor

@i-am-tom i-am-tom left a comment

Choose a reason for hiding this comment

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

Looks good to me

@i-am-tom i-am-tom added this pull request to the merge queue Jan 19, 2024
Merged via the queue into main with commit 2567175 Jan 19, 2024
@i-am-tom i-am-tom deleted the gil/gen-insert-v1 branch January 19, 2024 15:49
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