Read Native Query SQL from files #372

soupi · 2024-03-20T15:35:08Z

What

Currently we can specify native queries by inlining their SQL as a string in the sql field.
We'd like to be able to reference a .sql file instead, so we can avoid escaping characters and can use newlines.

The new config format will accept the following structures:

"sql": "<inline sql>" (as before, but also:)

"sql": { "inline": "<inline sql>" }

"sql": { "file": "<relative file path>" }

How

We parse the NativeQuerySql which represents the SQL field using an intermediate new type NativeQuerySqlExternal.
as part of the parse_configuration step and after serde deserialization, we traverse the metadata and convert NativeQuerySqlExternal to NativeQuerySql by reading from file (performing IO) if needed, and parsing to NativeQueryParts. We use the configuration directory that the user supplied to locate the relative path.

We replace the existing sql field in nativequeryinfo with an Either external or internal representation. This is not ideal because it breaks the parse don't validate principle.
We would like to mitigate this in the future by separating the RawConfiguration types and the (execution time) Configuration types completely.
We will do that as part of a larger ticket of redesigning the configuration format so it can be split across multiple files.

soupi · 2024-03-21T08:42:50Z

crates/configuration/src/configuration.rs

+        fs::read_to_string(&configuration_file)
+            .await
+            .map_err(|err| {
+                Error::IoErrorButStringified(format!("{}: {}", &configuration_file.display(), err))


I just want rust to tell me which file it couldn't read, so I added these everywhere. Sorry.

Perhaps we could instead change Error::IoError to take an Option<PathBuf> in addition.

soupi · 2024-03-21T08:43:11Z

crates/configuration/src/configuration.rs

        serde_json::from_str(&configuration_file_contents).map_err(|error| Error::ParseError {
            file_path: configuration_file.clone(),
            line: error.line(),
            column: error.column(),
            message: error.to_string(),
        })?;
+    // look for native query sql file references and read from disk.
+    for native_query_sql in configuration.metadata.native_queries.0.values_mut() {


step after serialization - read from disk.

soupi · 2024-03-21T08:52:00Z

crates/query-engine/metadata/src/metadata/native_queries.rs

-    }
-}
-
-struct NQVisitor;


@plcplc taught me that we can avoid all of this cruft by just implementing From/Into.

soupi · 2024-03-21T08:52:37Z

crates/query-engine/metadata/src/metadata/native_queries.rs

+    #[test]
+    fn parse_inline_untagged() {
+        assert_eq!(
+            serde_json::from_str::<NativeQuerySqlExternal>(r#""select 1""#).unwrap(),


We still want to be able to parse untagged inline expressions just as before.

soupi · 2024-03-21T08:53:56Z

crates/query-engine/translation/src/translation/query/native_queries.rs

@@ -31,6 +31,8 @@ pub fn translate(env: &Env, state: State) -> Result<Vec<sql::ast::CommonTableExp
        let sql: Vec<sql::ast::RawSql> = native_query
            .info
            .sql
+            .sql()
+            .map_err(Error::InternalError)?


This might fail because of our types shape, we'll fix this in the future.

soupi · 2024-03-21T08:54:25Z

...napshots/databases_tests__postgres__configuration_tests__get_rawconfiguration_v3_schema.snap

@@ -819,7 +819,45 @@ expression: schema
        }
      }
    },
-    "Native_query_sql": {
+    "NativeQuerySql": {


The new schema for the sql field.

soupi · 2024-03-21T08:56:03Z

crates/tests/tests-common/src/ndc_metadata/configuration.rs

+
+    let new_ndc_metadata_dir = temp_deploys_path.join(db_name);
+
+    copy_dir::copy_dir(&ndc_metadata_dir_path, &new_ndc_metadata_dir).map_err(|err| {


We now also copy the entire configuration directory.

soupi · 2024-03-21T08:56:38Z

static/postgres/v3-chinook-ndc-metadata/configuration.json

@@ -1484,7 +1502,7 @@
        "description": "A native query used to test support for composite types"
      },
      "summarize_organizations": {
-        "sql": "SELECT 'The organization ' || org.name || ' has ' || no_committees::text || ' committees, ' || 'the largest of which has ' || max_members || ' members.' AS result FROM (SELECT orgs.* FROM unnest({{organizations}}) as orgs) AS org JOIN LATERAL ( SELECT count(committee.*) AS no_committees, max(members_agg.no_members) AS max_members FROM unnest(org.committees) AS committee JOIN LATERAL ( SELECT count(*) as no_members FROM unnest(committee.members) AS members) AS members_agg ON true) AS coms ON true",
+        "sql": { "file": "./native_queries/summarize_organizations.sql" },


soupi · 2024-03-21T08:56:50Z

static/postgres/v3-chinook-ndc-metadata/native_queries/summarize_organizations.sql

plcplc

Good stuff! A few easy (stylistic) improvements in comments.

We'll get back to parse-don't-validate eventually 🌺

plcplc · 2024-03-21T10:52:50Z

crates/query-engine/metadata/src/metadata/native_queries.rs

+}
+
+/// A Native Query SQL after file resolution.
+/// This is the file that is expected in the metadata when translating requests.


Suggested change

/// This is the file that is expected in the metadata when translating requests.

/// This is the `NativeQuerySqlEither`-variant that is expected in the metadata when translating requests.

Thanks! Rephrased the comment.

plcplc · 2024-03-21T10:57:29Z

crates/query-engine/metadata/src/metadata/native_queries.rs

+    /// used for deserialization.
+    fn from(value: NativeQuerySql) -> Self {
+        match value {
+            NativeQuerySql::Inline { sql } => NativeQuerySqlExternal::Inline { inline: sql },


So, serialization/deserialization roundtripping a configuration turns a

sql: "SELECT.."

into

sql: { inline: "SELECT.." }

...which is fine

Yes, this is intended so that we could phase out raw string eventually.

plcplc · 2024-03-21T11:13:24Z

crates/query-engine/metadata/src/metadata/native_queries.rs

+    }
+}
+
+impl JsonSchema for NativeQuerySqlEither {


If you want to, you can avoid this schema impl with the #[schemars(with="NativeQuerySqlExternal")] attribute on NativeQuerySqlEither and #[schemars(rename="NativeQuerySql")] on NativeQuerySqlExternal.

Although it's arguably mostly a matter of style for this example.

I think this doesn't work because it doesn't want to generate a JsonSchema without NativeQuerySql also having a JsonSchema instance.

Thanks for the suggestions though, I would definitely prefer something like this :)

plcplc · 2024-03-21T11:14:00Z

crates/query-engine/metadata/src/metadata/native_queries.rs

-    }
-}
-
-struct NQVisitor;


plcplc · 2024-03-21T11:15:31Z

crates/query-engine/metadata/src/metadata/native_queries.rs

-}
-
-impl JsonSchema for NativeQuerySql {
+impl JsonSchema for NativeQueryParts {


You can probably also avoid this instance with a #[schemars(rename=..)] attribute.

I think this doesn't work because it attempts to create a schema for Vec<NativeQueryPart>, even if I add with = "String".

plcplc · 2024-03-21T11:24:58Z

static/postgres/v3-chinook-ndc-metadata/native_queries/summarize_organizations.sql

@@ -0,0 +1,22 @@
+SELECT


Ah, how I wished I could have had a separate file when I made this query. Boy is it easier on the eyes without string escaping~

SamirTalwar

Sorry for the delay here. I only have some minor comments on delaying error stringification.

SamirTalwar · 2024-03-21T15:52:46Z

crates/configuration/src/configuration.rs

+        fs::read_to_string(&configuration_file)
+            .await
+            .map_err(|err| {
+                Error::IoErrorButStringified(format!("{}: {}", &configuration_file.display(), err))


Perhaps we could instead change Error::IoError to take an Option<PathBuf> in addition.

SamirTalwar · 2024-03-21T15:54:55Z

crates/query-engine/metadata/src/metadata/native_queries.rs

+pub fn parse_native_query_from_file(file: std::path::PathBuf) -> Result<NativeQuerySql, String> {
+    let contents: String = match fs::read_to_string(&file) {
+        Ok(ok) => Ok(ok),
+        Err(err) => Err(format!("{}: {}", &file.display(), err)),


Again, here seems like a good place for Error::IoError { path: Option<PathBuf>, inner: io::Error }.

I would approve such a PR :)

Gil Mizrahi added 6 commits March 20, 2024 16:43

sql files

0ce7614

better code

5392578

lint

ceb9304

better info errors

0f744df

fix copy

ef95f21

changelog

dd33a57

soupi changed the title ~~Gil/sql files~~ Read Native Query SQL from files Mar 21, 2024

separate deserialization and disk read steps

dc9695b

soupi commented Mar 21, 2024

View reviewed changes

self review

0e4d09a

soupi commented Mar 21, 2024

View reviewed changes

remove deps

92cb0b2

soupi marked this pull request as ready for review March 21, 2024 08:57

soupi requested a review from plcplc March 21, 2024 08:57

plcplc approved these changes Mar 21, 2024

View reviewed changes

fix comment

ca3c922

soupi enabled auto-merge March 21, 2024 13:17

soupi added this pull request to the merge queue Mar 21, 2024

Merged via the queue into main with commit b94fd3a Mar 21, 2024
35 checks passed

soupi deleted the gil/sql-files branch March 21, 2024 13:30

SamirTalwar reviewed Mar 21, 2024

View reviewed changes

soupi mentioned this pull request Mar 27, 2024

update pg native query sql definition hasura/ddn-docs#321

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read Native Query SQL from files #372

Read Native Query SQL from files #372

soupi commented Mar 20, 2024 •

edited

Loading

soupi Mar 21, 2024

SamirTalwar Mar 21, 2024

soupi Mar 21, 2024

soupi Mar 21, 2024

plcplc Mar 21, 2024

soupi Mar 21, 2024

soupi Mar 21, 2024

soupi Mar 21, 2024

soupi Mar 21, 2024

soupi Mar 21, 2024

soupi Mar 21, 2024

plcplc left a comment

plcplc Mar 21, 2024

soupi Mar 21, 2024

plcplc Mar 21, 2024

soupi Mar 21, 2024

plcplc Mar 21, 2024

soupi Mar 21, 2024

plcplc Mar 21, 2024

plcplc Mar 21, 2024

soupi Mar 21, 2024

plcplc Mar 21, 2024

SamirTalwar left a comment

SamirTalwar Mar 21, 2024

SamirTalwar Mar 21, 2024

soupi Mar 21, 2024


		let new_ndc_metadata_dir = temp_deploys_path.join(db_name);

		copy_dir::copy_dir(&ndc_metadata_dir_path, &new_ndc_metadata_dir).map_err(\|err\| {

	/// This is the file that is expected in the metadata when translating requests.
	/// This is the `NativeQuerySqlEither`-variant that is expected in the metadata when translating requests.

Read Native Query SQL from files #372

Read Native Query SQL from files #372

Conversation

soupi commented Mar 20, 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

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

plcplc 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

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

soupi commented Mar 20, 2024 •

edited

Loading