Skip to content

cast int64 and numeric to string by default #416

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
Apr 16, 2024
2 changes: 2 additions & 0 deletions changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

- Support ndc-spec v0.1.2 and change the type representation of types accordingly.
([#408](https://github.com/hasura/ndc-postgres/pull/408))
- `int8` and `numeric` columns will now emit a string json representation by default.
([#416](https://github.com/hasura/ndc-postgres/pull/416))

### Fixed

Expand Down
7 changes: 6 additions & 1 deletion crates/configuration/src/version3/metadata/database.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,10 +232,15 @@ pub enum TypeRepresentation {
Int16,
/// int4
Int32,
/// int8
/// int8 as integer
Int64,
/// int8 as string
Int64AsString,
/// numeric
BigDecimal,
/// numeric as string
BigDecimalAsString,

/// timestamp
Timestamp,
/// timestamp with timezone
Expand Down
38 changes: 30 additions & 8 deletions crates/configuration/src/version3/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,9 @@ pub async fn introspect(
.introspection_options
.introspect_prefix_function_comparison_operators,
)
.bind(serde_json::to_value(base_type_representations().0)?);
.bind(serde_json::to_value(
base_type_representations(args.metadata.type_representations).0,
)?);

let row = connection
.fetch_one(query)
Expand Down Expand Up @@ -196,8 +198,13 @@ pub async fn introspect(
})
}

fn base_type_representations() -> database::TypeRepresentations {
database::TypeRepresentations(
/// Merge the type representations currenting defined in the user's configuration with
/// our base defaults. User configuration takes precedence.
fn base_type_representations(
database::TypeRepresentations(existing_type_representations): database::TypeRepresentations,
) -> database::TypeRepresentations {
// Start with the default type representations
let mut type_representations: database::TypeRepresentations = database::TypeRepresentations(
[
// Bit strings:
// https://www.postgresql.org/docs/current/datatype-bit.html
Expand Down Expand Up @@ -247,11 +254,11 @@ fn base_type_representations() -> database::TypeRepresentations {
// This is not what we do now and is a breaking change.
// This will need to be changed in the future. In the meantime, we report
// The type representation to be json.
database::TypeRepresentation::Json,
database::TypeRepresentation::Int64AsString,
),
(
database::ScalarType("numeric".to_string()),
database::TypeRepresentation::BigDecimal,
database::TypeRepresentation::BigDecimalAsString,
),
(
database::ScalarType("text".to_string()),
Expand Down Expand Up @@ -283,7 +290,14 @@ fn base_type_representations() -> database::TypeRepresentations {
),
]
.into(),
)
);
// If the user already has existing type representations defined,
// override the default ones using `insert`.
// We do this to not change the behaviour for the user on update.
for (typ, type_rep) in existing_type_representations {
type_representations.0.insert(typ, type_rep);
}
type_representations
}

/// Collect all the composite types that can occur in the metadata.
Expand Down Expand Up @@ -765,9 +779,15 @@ fn convert_type_representation(
metadata::TypeRepresentation::Int64 => {
query_engine_metadata::metadata::TypeRepresentation::Int64
}
metadata::TypeRepresentation::Int64AsString => {
query_engine_metadata::metadata::TypeRepresentation::Int64AsString
}
metadata::TypeRepresentation::BigDecimal => {
query_engine_metadata::metadata::TypeRepresentation::BigDecimal
}
metadata::TypeRepresentation::BigDecimalAsString => {
query_engine_metadata::metadata::TypeRepresentation::BigDecimalAsString
}
metadata::TypeRepresentation::Timestamp => {
query_engine_metadata::metadata::TypeRepresentation::Timestamp
}
Expand All @@ -792,11 +812,13 @@ fn convert_type_representation(
metadata::TypeRepresentation::Geometry => {
query_engine_metadata::metadata::TypeRepresentation::Geometry
}
// This is deprecated in ndc-spec
metadata::TypeRepresentation::Number => {
query_engine_metadata::metadata::TypeRepresentation::Number
query_engine_metadata::metadata::TypeRepresentation::Json
}
// This is deprecated in ndc-spec
metadata::TypeRepresentation::Integer => {
query_engine_metadata::metadata::TypeRepresentation::Integer
query_engine_metadata::metadata::TypeRepresentation::Json
}
metadata::TypeRepresentation::Json => {
query_engine_metadata::metadata::TypeRepresentation::Json
Expand Down
13 changes: 8 additions & 5 deletions crates/connectors/ndc-postgres/src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -622,8 +622,14 @@ fn map_type_representation(
metadata::TypeRepresentation::Float64 => models::TypeRepresentation::Float64,
metadata::TypeRepresentation::Int16 => models::TypeRepresentation::Int16,
metadata::TypeRepresentation::Int32 => models::TypeRepresentation::Int32,
metadata::TypeRepresentation::Int64 => models::TypeRepresentation::Int64,
metadata::TypeRepresentation::BigDecimal => models::TypeRepresentation::BigDecimal,
// Int64 returns a number.
metadata::TypeRepresentation::Int64 => models::TypeRepresentation::JSON,
// Int64AsString returns a string.
metadata::TypeRepresentation::Int64AsString => models::TypeRepresentation::Int64,
// BigDecimal returns a number.
metadata::TypeRepresentation::BigDecimal => models::TypeRepresentation::JSON,
// BigDecimalAsString returns a string.
metadata::TypeRepresentation::BigDecimalAsString => models::TypeRepresentation::BigDecimal,
Comment on lines +625 to +632
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the mapping between our internal type representations and ndc-spec type representations.

metadata::TypeRepresentation::Timestamp => models::TypeRepresentation::Timestamp,
metadata::TypeRepresentation::Timestamptz => models::TypeRepresentation::TimestampTZ,
metadata::TypeRepresentation::Time => models::TypeRepresentation::String,
Expand All @@ -636,8 +642,5 @@ fn map_type_representation(
metadata::TypeRepresentation::Enum(variants) => models::TypeRepresentation::Enum {
one_of: variants.clone(),
},
// Stop gap until we remove these. Done so we won't break compatability.
metadata::TypeRepresentation::Integer => models::TypeRepresentation::JSON,
metadata::TypeRepresentation::Number => models::TypeRepresentation::JSON,
}
}
21 changes: 16 additions & 5 deletions crates/query-engine/metadata/src/metadata/database.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,17 @@ pub enum Type {
ArrayType(Box<Type>),
}

impl Type {
/// If a type is a scalar type, fetch it. If not, return None.
pub fn scalar_type(&self) -> Option<&ScalarType> {
match self {
Type::ScalarType(scalar_type) => Some(scalar_type),
Type::CompositeType(_) => None,
Type::ArrayType(_) => None,
}
}
}

/// Information about a composite type. These are very similar to tables, but with the crucial
/// difference that composite types do not support constraints (such as NOT NULL).
#[derive(Debug, Clone, PartialEq, Eq)]
Expand Down Expand Up @@ -199,10 +210,14 @@ pub enum TypeRepresentation {
Int16,
/// int4
Int32,
/// int8
/// int8 as integer
Int64,
/// int8 as string
Int64AsString,
/// numeric
BigDecimal,
/// numeric as string
BigDecimalAsString,
/// timestamp
Timestamp,
/// timestamp with timezone
Expand All @@ -219,10 +234,6 @@ pub enum TypeRepresentation {
Geography,
/// geometry
Geometry,
/// Any JSON number
Number,
/// Any JSON number, with no decimal part
Integer,
/// An arbitrary json.
Json,
/// One of the specified string values
Expand Down
43 changes: 40 additions & 3 deletions crates/query-engine/translation/src/translation/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ pub struct State {
}

#[derive(Debug)]
/// Used for generating a unique name for intermediate tables.
pub struct TableAliasIndex(pub u64);

#[derive(Debug)]
Expand Down Expand Up @@ -89,7 +90,7 @@ pub enum CollectionInfo<'env> {
}

#[derive(Debug)]
/// Metadata information about a specific collection.
/// Metadata information about a specific collection or composite type.
pub enum CompositeTypeInfo<'env> {
CollectionInfo(CollectionInfo<'env>),
CompositeTypeInfo {
Expand All @@ -114,8 +115,7 @@ impl<'request> Env<'request> {
}
}

/// Lookup a collection's information in the metadata.

/// Lookup collection or composite type.
pub fn lookup_composite_type(
&self,
type_name: &'request str,
Expand All @@ -138,6 +138,7 @@ impl<'request> Env<'request> {
}
}

/// Lookup a collection's information in the metadata.
pub fn lookup_collection(
&self,
collection_name: &'request str,
Expand Down Expand Up @@ -202,6 +203,18 @@ impl<'request> Env<'request> {
})
}

/// Lookup type representation of a type.
pub fn lookup_type_representation(
&self,
scalar_type: &metadata::ScalarType,
) -> Option<metadata::TypeRepresentation> {
self.metadata
.type_representations
.0
.get(scalar_type)
.cloned()
Copy link
Contributor

Choose a reason for hiding this comment

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

Stupid nitpick: we only use this as a reference so we could probably return one and not need to clone it.

This is stupid because cloning it will be cheap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

smart. thanks!

}

/// Try to get the variables table reference. This will fail if no variables were passed
/// as part of the query request.
pub fn get_variables_table(&self) -> Result<sql::ast::TableReference, Error> {
Expand Down Expand Up @@ -262,6 +275,30 @@ impl CompositeTypeInfo<'_> {
)),
}
}

/// Fetch all the field names (external, internal) of a composite type.
pub fn fields(&self) -> Vec<(&String, &String)> {
match self {
CompositeTypeInfo::CompositeTypeInfo { name: _, info } => info
.fields
.iter()
.map(|(name, field)| (name, &field.name))
.collect::<Vec<_>>(),

CompositeTypeInfo::CollectionInfo(collection_info) => match collection_info {
CollectionInfo::Table { name: _, info } => info
.columns
.iter()
.map(|(name, column)| (name, &column.name))
.collect::<Vec<_>>(),
CollectionInfo::NativeQuery { name: _, info } => info
.columns
.iter()
.map(|(name, column)| (name, &column.name))
.collect::<Vec<_>>(),
},
}
}
}

impl Default for State {
Expand Down
Loading