Bring ndc-postgres in line with ndc-spec RC15 #296

i-am-tom · 2024-02-14T10:17:42Z

What

This PR updates ndc-postgres to adhere to the RC15 version of ndc-spec.

How

This required a bit more work than we'd originally expected: because the format of procedure results changed, we now need to generate a new object type for each procedure (namely, the result type: a product of affected_rows (int4) and returning (whatever the underlying collection's row type is).

crates/query-engine/translation/src/translation/mutation/translate.rs

crates/query-engine/sql/src/sql/helpers.rs

crates/configuration/Cargo.toml

SamirTalwar · 2024-02-15T16:10:26Z

crates/connectors/ndc-postgres/src/schema.rs

+    // If int4 doesn't exist anywhere else in the schema, we need to add it here. However, a user
+    // can't filter or aggregate based on the affected rows of a procedure, so we don't need to add
+    // any aggregate functions or comparison operators. However, if int4 exists elsewhere in the
+    // schema and has already been added, it will also already contain these functions and
+    // operators.
+    scalar_types
+        .entry("int4".to_string())
+        .or_insert(models::ScalarType {
+            aggregate_functions: BTreeMap::new(),
+            comparison_operators: BTreeMap::new(),
+        });


Is this also required when generating the configuration?

If it is, then it will need to be moved to occurring_scalar_types.

Thinking about it, even if it's not, that still feels like a better place for it. I plan on making that function private and putting the scalar types into the configuration at some point soon, so this function can just read them directly.

It's not really part of the configuration. Even if the user omits int4 from the configuration we still need to expose this type for the schema or else procedures will break, so I don't see a way out of defining it here.

I am convinced.

Any reason not to add it in up-front rather than mutating later?

Only that, if no mutations are generated, it's a type without a purpose, which we seem to be avoiding everywhere else? idk, I'm not sure what the danger is in having all scalar types declared upfront and not having the ocurring_scalar_types check, but doing it this way means we're still respecting that approach. Probably a wider question

I think it's better to keep the cause and effect together. We generate the type because the procedure needs it, and we don't if we don't. It's better to place them together instead of in two separate places.

SamirTalwar

If you want to go straight to ndc-spec v0.1.0-rc.16, here's the places you need to hit.

I suggest not accepting these comments because you'll also need to regenerate Cargo.lock.

SamirTalwar · 2024-02-16T11:21:31Z

crates/configuration/Cargo.toml

@@ -6,7 +6,7 @@ edition = "2021"
 [dependencies]
 query-engine-metadata = { path = "../query-engine/metadata" }

-ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "02d26c1" }
+ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "e0e9629" }


We can update to RC 16 directly if you like.

Suggested change

ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "e0e9629" }

ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "ee52bae" }

SamirTalwar · 2024-02-16T11:21:37Z

crates/connectors/ndc-postgres/Cargo.toml

@@ -21,7 +21,7 @@ query-engine-metadata = { path = "../../query-engine/metadata" }
 query-engine-sql = { path = "../../query-engine/sql" }
 query-engine-translation = { path = "../../query-engine/translation" }

-ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "02d26c1" }
+ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "e0e9629" }


Suggested change

ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "e0e9629" }

ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "ee52bae" }

SamirTalwar · 2024-02-16T11:21:43Z

crates/connectors/ndc-postgres/Cargo.toml

+ndc-client = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.15" }
+ndc-test = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.15" }


Suggested change

ndc-client = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.15" }

ndc-test = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.15" }

ndc-client = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.16" }

ndc-test = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.16" }

SamirTalwar · 2024-02-16T11:21:53Z

crates/query-engine/translation/Cargo.toml

@@ -5,7 +5,7 @@ edition = "2021"
 license = "Apache-2.0"

 [dependencies]
-ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "02d26c1" }
+ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "e0e9629" }


Suggested change

ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "e0e9629" }

ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "ee52bae" }

SamirTalwar · 2024-02-16T11:22:05Z

crates/tests/tests-common/Cargo.toml

+ndc-client = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.15" }
+ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "e0e9629" }
+ndc-test = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.15" }


Suggested change

ndc-client = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.15" }

ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "e0e9629" }

ndc-test = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.15" }

ndc-client = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.16" }

ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "ee52bae" }

ndc-test = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.16" }

SamirTalwar · 2024-02-16T11:22:23Z

crates/tests/databases-tests/Cargo.toml

@@ -23,7 +23,7 @@ postgres = []
 openapi-generator = { path = "../../documentation/openapi" }
 ndc-postgres = { path = "../../connectors/ndc-postgres" }
 ndc-postgres-configuration = { path = "../../configuration" }
-ndc-test = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.14" }
+ndc-test = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.15" }


Suggested change

ndc-test = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.15" }

ndc-test = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.16" }

soupi · 2024-02-16T12:32:35Z

I think separate steps are better.

i-am-tom assigned i-am-tom and soupi Feb 14, 2024

Base automatically changed from gil/ndc-spec-rc14 to main February 14, 2024 11:34

i-am-tom added 2 commits February 14, 2024 17:04

--wip-- [skip ci]

aac59d3

It lives!

c32cd87

soupi force-pushed the gjm-tjh/ndc-spec-rc15 branch from 33658c4 to c32cd87 Compare February 14, 2024 15:17

i-am-tom force-pushed the gjm-tjh/ndc-spec-rc15 branch from a425203 to c32cd87 Compare February 14, 2024 15:31

soupi reviewed Feb 15, 2024

View reviewed changes

crates/configuration/Cargo.toml Outdated Show resolved Hide resolved

i-am-tom added 7 commits February 15, 2024 14:21

--wip-- [skip ci]

950b877

Add more object types

51c90f1

Tidying up

48cc5a0

More docs

8da5bee

More refactoring

7cf1695

Update the hashes

c1c27d6

just format

9d7897f

i-am-tom changed the title ~~ndc-spec RC15 [WIP]~~ Bring ndc-postgres in line with ndc-spec RC15 Feb 15, 2024

i-am-tom marked this pull request as ready for review February 15, 2024 14:36

SamirTalwar reviewed Feb 15, 2024

View reviewed changes

i-am-tom enabled auto-merge February 16, 2024 10:40

SamirTalwar reviewed Feb 16, 2024

View reviewed changes

SamirTalwar approved these changes Feb 16, 2024

View reviewed changes

i-am-tom added this pull request to the merge queue Feb 16, 2024

Merged via the queue into main with commit e47a46e Feb 16, 2024
28 checks passed

i-am-tom deleted the gjm-tjh/ndc-spec-rc15 branch February 16, 2024 11:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring ndc-postgres in line with ndc-spec RC15 #296

Bring ndc-postgres in line with ndc-spec RC15 #296

i-am-tom commented Feb 14, 2024 •

edited

Loading

SamirTalwar Feb 15, 2024

i-am-tom Feb 15, 2024

SamirTalwar Feb 15, 2024

soupi Feb 16, 2024

SamirTalwar Feb 16, 2024

i-am-tom Feb 16, 2024

soupi Feb 16, 2024

SamirTalwar left a comment

SamirTalwar Feb 16, 2024

SamirTalwar Feb 16, 2024

SamirTalwar Feb 16, 2024

SamirTalwar Feb 16, 2024

SamirTalwar Feb 16, 2024

SamirTalwar Feb 16, 2024

soupi commented Feb 16, 2024

	ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "e0e9629" }
	ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "ee52bae" }

		ndc-client = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.15" }
		ndc-test = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.15" }

Bring ndc-postgres in line with ndc-spec RC15 #296

Bring ndc-postgres in line with ndc-spec RC15 #296

Conversation

i-am-tom commented Feb 14, 2024 • edited Loading

What

How

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamirTalwar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soupi commented Feb 16, 2024

i-am-tom commented Feb 14, 2024 •

edited

Loading