Skip to content

Support complex & array types in native query creation cli utility #742

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.

Sign up for GitHub

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

Open
BenoitRanque wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Support complex & array types in native query creation cli utility #742

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
042e727
Support complex & array types in native query creation cli utility
BenoitRanque Apr 3, 2025
dc7fb6b
add changelog
BenoitRanque Apr 3, 2025
b4aa1d2
Add tests for array args and failing test for nested array args
BenoitRanque May 8, 2025
d9b9b6a
update test and add comment re: multi dimensional array being identic…
BenoitRanque May 8, 2025
96005c7
Add missing handling for composite types for config v4
BenoitRanque May 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

### Fixed

- Support composite and array arguments and columns when creating native queries via the cli. We used to assume scalars

## [v2.1.1] - 2025-03-12

### Changed
Expand Down
96 changes: 73 additions & 23 deletions crates/configuration/src/version4/native_operations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ use std::path::Path;
use query_engine_sql::sql;

use ndc_models as models;
use sqlx::postgres::types::Oid;
use sqlx::postgres::PgTypeKind;
use sqlx::Connection;
use sqlx::Executor;
use sqlx::{Column, PgConnection};
Expand All @@ -20,6 +22,37 @@ pub enum Kind {
Mutation,
}

/// Recursively figure out the underlying type oid for a given type
/// Figure out the underlying type oid for an argument or column
/// Usually we just want the oid, but for arrays we need the oid for the underlying (non-array) type
fn underlying_oid(kind: &PgTypeKind, oid: Option<Oid>) -> Option<Oid> {
match kind {
PgTypeKind::Simple
| PgTypeKind::Pseudo
| PgTypeKind::Domain(_)
| PgTypeKind::Enum(_)
| PgTypeKind::Range(_)
| PgTypeKind::Composite(_) => oid,
PgTypeKind::Array(pg_type_info) => underlying_oid(pg_type_info.kind(), pg_type_info.oid()),
}
}

/// Convert a kind and a resolved underlying type name to a metadata type
fn to_metadata_type(kind: &PgTypeKind, underlying_type_name: &str) -> metadata::Type {
match kind {
PgTypeKind::Simple
| PgTypeKind::Pseudo
| PgTypeKind::Domain(_)
| PgTypeKind::Enum(_)
| PgTypeKind::Range(_) => metadata::Type::ScalarType(underlying_type_name.into()),
PgTypeKind::Composite(_) => metadata::Type::CompositeType(underlying_type_name.into()),
PgTypeKind::Array(pg_type_info) => metadata::Type::ArrayType(Box::new(to_metadata_type(
pg_type_info.kind(),
underlying_type_name,
))),
}
}

/// Take a SQL file containing a Native Operation, check against the database that it is valid,
/// and add it to the configuration if it is.
pub async fn create(
Expand Down Expand Up @@ -60,21 +93,23 @@ pub async fn create(
anyhow::bail!("Internal error: Native operation parameter was not a variable.")
};

let the_oid = result_param
.oid()
// get the oid for the underlying type. If the parameter is an array, get the array item's oid
let the_oid = underlying_oid(result_param.kind(), result_param.oid())
.ok_or(anyhow::anyhow!(
"Internal error: All sqlx TypeInfos should have an oid."
))?
.0;

arguments_to_oids.insert(param_name, i64::from(the_oid));
arguments_to_oids.insert(
param_name,
(i64::from(the_oid), result_param.kind().to_owned()),
);
}

// Fill the columns list.
for (index, column) in result.columns.iter().enumerate() {
let the_oid = column
.type_info()
.oid()
// get the oid for the underlying type. If the parameter is an array, get the array item's oid
let the_oid = underlying_oid(column.type_info().kind(), column.type_info().oid())
.ok_or(anyhow::anyhow!(
"Internal error: All sqlx TypeInfos should have an oid."
))?
Expand All @@ -84,44 +119,59 @@ pub async fn create(
true,
);

columns_to_oids.insert(column.name().to_string(), (i64::from(the_oid), is_nullable));
columns_to_oids.insert(
column.name().to_string(),
(
i64::from(the_oid),
column.type_info().kind().to_owned(),
is_nullable,
),
);
}

let mut oids: BTreeSet<i64> = arguments_to_oids.values().copied().collect();
oids.extend::<BTreeSet<i64>>(columns_to_oids.values().copied().map(|x| x.0).collect());
let mut oids: BTreeSet<i64> = arguments_to_oids
.values()
.map(|(oid, _)| oid)
.copied()
.collect();
oids.extend::<BTreeSet<i64>>(
columns_to_oids
.values()
.map(|(oid, _, _)| oid)
.copied()
.collect(),
);
let oids_vec: Vec<_> = oids.into_iter().collect();
let oids_map = oids_to_typenames(configuration, connection_string, &oids_vec).await?;

let mut arguments = BTreeMap::new();
for (name, oid) in arguments_to_oids {
for (name, (oid, kind)) in arguments_to_oids {
let underlying_type_name = oids_map
.get(&oid)
.ok_or_else(|| anyhow::anyhow!("Internal error: oid not found in map."))?;

arguments.insert(
name.clone().into(),
metadata::ReadOnlyColumnInfo {
name: name.clone(),
r#type: metadata::Type::ScalarType(
oids_map
.get(&oid)
.ok_or_else(|| anyhow::anyhow!("Internal error: oid not found in map."))?
.clone(),
),
r#type: to_metadata_type(&kind, &underlying_type_name.to_string()),
description: None,
// we don't have this information, so we assume not nullable.
nullable: metadata::Nullable::NonNullable,
},
);
}
let mut columns = BTreeMap::new();
for (name, (oid, is_nullable)) in columns_to_oids {
for (name, (oid, kind, is_nullable)) in columns_to_oids {
let underlying_type_name = oids_map
.get(&oid)
.ok_or_else(|| anyhow::anyhow!("Internal error: oid not found in map."))?;

columns.insert(
name.clone().into(),
metadata::ReadOnlyColumnInfo {
name: name.clone(),
r#type: metadata::Type::ScalarType(
oids_map
.get(&oid)
.ok_or_else(|| anyhow::anyhow!("Internal error: oid not found in map."))?
.clone(),
),
r#type: to_metadata_type(&kind, &underlying_type_name.to_string()),
description: None,
nullable: if is_nullable {
metadata::Nullable::Nullable
Expand Down
109 changes: 83 additions & 26 deletions crates/configuration/src/version5/native_operations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
//! Infer information about a Native Operation from a Native Operation SQL string.

use ndc_models as models;
use sqlx::postgres::types::Oid;
use sqlx::postgres::PgTypeKind;
use std::collections::{BTreeMap, BTreeSet};
use std::path::Path;

Expand All @@ -22,6 +24,37 @@ pub enum Kind {
Mutation,
}

/// Recursively figure out the underlying type oid for a given type
/// Figure out the underlying type oid for an argument or column
/// Usually we just want the oid, but for arrays we need the oid for the underlying (non-array) type
fn underlying_oid(kind: &PgTypeKind, oid: Option<Oid>) -> Option<Oid> {
match kind {
PgTypeKind::Simple
| PgTypeKind::Pseudo
| PgTypeKind::Domain(_)
| PgTypeKind::Enum(_)
| PgTypeKind::Range(_)
| PgTypeKind::Composite(_) => oid,
PgTypeKind::Array(pg_type_info) => underlying_oid(pg_type_info.kind(), pg_type_info.oid()),
}
}

/// Convert a kind and a resolved underlying type name to a metadata type
fn to_metadata_type(kind: &PgTypeKind, underlying_type_name: &str) -> metadata::Type {
match kind {
PgTypeKind::Simple
| PgTypeKind::Pseudo
| PgTypeKind::Domain(_)
| PgTypeKind::Enum(_)
| PgTypeKind::Range(_) => metadata::Type::ScalarType(underlying_type_name.into()),
PgTypeKind::Composite(_) => metadata::Type::CompositeType(underlying_type_name.into()),
PgTypeKind::Array(pg_type_info) => metadata::Type::ArrayType(Box::new(to_metadata_type(
pg_type_info.kind(),
underlying_type_name,
))),
}
}

/// Take a SQL file containing a Native Operation, check against the database that it is valid,
/// and add it to the configuration if it is.
pub async fn create(
Expand Down Expand Up @@ -64,21 +97,23 @@ pub async fn create(
anyhow::bail!("Internal error: Native operation parameter was not a variable.")
};

let the_oid = result_param
.oid()
// get the oid for the underlying type. If the parameter is an array, get the array item's oid
let the_oid = underlying_oid(result_param.kind(), result_param.oid())
.ok_or(anyhow::anyhow!(
"Internal error: All sqlx TypeInfos should have an oid."
))?
.0;

arguments_to_oids.insert(param_name, i64::from(the_oid));
arguments_to_oids.insert(
param_name,
(i64::from(the_oid), result_param.kind().to_owned()),
);
}

// Fill the columns list.
for (index, column) in result.columns.iter().enumerate() {
let the_oid = column
.type_info()
.oid()
// get the oid for the underlying type. If the parameter is an array, get the array item's oid
let the_oid = underlying_oid(column.type_info().kind(), column.type_info().oid())
.ok_or(anyhow::anyhow!(
"Internal error: All sqlx TypeInfos should have an oid."
))?
Expand All @@ -88,45 +123,60 @@ pub async fn create(
true,
);

columns_to_oids.insert(column.name().to_string(), (i64::from(the_oid), is_nullable));
columns_to_oids.insert(
column.name().to_string(),
(
i64::from(the_oid),
column.type_info().kind().to_owned(),
is_nullable,
),
);
}

let mut oids: BTreeSet<i64> = arguments_to_oids.values().copied().collect();
oids.extend::<BTreeSet<i64>>(columns_to_oids.values().copied().map(|x| x.0).collect());
let mut oids: BTreeSet<i64> = arguments_to_oids
.values()
.map(|(oid, _)| oid)
.copied()
.collect();
oids.extend::<BTreeSet<i64>>(
columns_to_oids
.values()
.map(|(oid, _, _)| oid)
.copied()
.collect(),
);
let oids_vec: Vec<_> = oids.into_iter().collect();
let oids_map =
oids_to_typenames(configuration, connection_string, environment, &oids_vec).await?;

let mut arguments = BTreeMap::new();
for (name, oid) in arguments_to_oids {
for (name, (oid, kind)) in arguments_to_oids {
let underlying_type_name = oids_map
.get(&oid)
.ok_or_else(|| anyhow::anyhow!("Internal error: oid not found in map."))?;

arguments.insert(
name.clone().into(),
metadata::ReadOnlyColumnInfo {
name: name.clone(),
r#type: metadata::Type::ScalarType(
oids_map
.get(&oid)
.ok_or_else(|| anyhow::anyhow!("Internal error: oid not found in map."))?
.clone(),
),
r#type: to_metadata_type(&kind, &underlying_type_name.to_string()),
description: None,
// we don't have this information, so we assume not nullable.
nullable: metadata::Nullable::NonNullable,
},
);
}
let mut columns = BTreeMap::new();
for (name, (oid, is_nullable)) in columns_to_oids {
for (name, (oid, kind, is_nullable)) in columns_to_oids {
let underlying_type_name = oids_map
.get(&oid)
.ok_or_else(|| anyhow::anyhow!("Internal error: oid not found in map."))?;

columns.insert(
name.clone().into(),
metadata::ReadOnlyColumnInfo {
name: name.clone(),
r#type: metadata::Type::ScalarType(
oids_map
.get(&oid)
.ok_or_else(|| anyhow::anyhow!("Internal error: oid not found in map."))?
.clone(),
),
r#type: to_metadata_type(&kind, &underlying_type_name.to_string()),
description: None,
nullable: if is_nullable {
metadata::Nullable::Nullable
Expand Down Expand Up @@ -157,7 +207,7 @@ pub async fn oids_to_typenames(
connection_string: &str,
environment: &impl Environment,
oids: &Vec<i64>,
) -> anyhow::Result<BTreeMap<i64, models::ScalarTypeName>> {
) -> anyhow::Result<BTreeMap<i64, models::TypeName>> {
let connect_options =
crate::get_connect_options(&crate::ConnectionUri::from(connection_string), environment)?;
// Connect to the db.
Expand All @@ -171,7 +221,7 @@ pub async fn oids_to_typenames(
.instrument(info_span!("Run oid lookup query"))
.await?;

let mut oids_map: BTreeMap<i64, models::ScalarTypeName> = BTreeMap::new();
let mut oids_map: BTreeMap<i64, models::TypeName> = BTreeMap::new();

// Reverse lookup the schema.typename and find the ndc type name,
// if we find all we can just add the nq and call it a day.
Expand All @@ -183,7 +233,14 @@ pub async fn oids_to_typenames(
let mut found = false;
for (scalar_type_name, info) in &configuration.metadata.types.scalar.0 {
if info.schema_name == schema_name && info.type_name == type_name {
oids_map.insert(oid, scalar_type_name.clone());
oids_map.insert(oid, scalar_type_name.inner().clone());
found = true;
continue;
}
}
for (composite_type_name, info) in &configuration.metadata.types.composite.0 {
if info.schema_name == schema_name && info.type_name == type_name {
oids_map.insert(oid, composite_type_name.clone());
found = true;
continue;
}
Expand Down
4 changes: 2 additions & 2 deletions ...es/snapshots/databases_tests__postgres__configuration_tests__create_native_operation.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ expression: result
"characters": {
"name": "characters",
"type": {
"scalarType": "characters"
"compositeType": "characters"
Copy link
Contributor

@danieljharvey danieljharvey May 13, 2025

Choose a reason for hiding this comment

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

Do we know why this changed?

Copy link
Contributor

@danieljharvey danieljharvey May 13, 2025

Choose a reason for hiding this comment

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

Is this good?

},
"nullable": "nullable",
"description": null
Expand All @@ -42,7 +42,7 @@ expression: result
"name": {
"name": "name",
"type": {
"scalarType": "chara"
"compositeType": "chara"
},
"nullable": "nullable",
"description": null
Expand Down
Loading