-
Notifications
You must be signed in to change notification settings - Fork 5
ndc-spec 0.1.2 #408
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
ndc-spec 0.1.2 #408
Changes from all commits
fd338e7
6928aed
a698642
670a92d
3603732
5fb9d0a
8db2336
ee860fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -222,66 +222,59 @@ fn base_type_representations() -> database::TypeRepresentations { | |
| ), | ||
| ( | ||
| database::ScalarType("date".to_string()), | ||
| database::TypeRepresentation::String, | ||
| database::TypeRepresentation::Date, | ||
| ), | ||
| ( | ||
| database::ScalarType("float4".to_string()), | ||
| database::TypeRepresentation::Number, | ||
| database::TypeRepresentation::Float32, | ||
| ), | ||
| // Note that we default wide numerical types to Number/Integer because any column of such type | ||
| // that PostgreSQL outputs as json will still be using numbers, and there is nothing we can | ||
| // feasibly do about this that doesn't involve very careful book-keeping on types and extra | ||
| // deserialization/serialization passes over query results. | ||
| // | ||
| // Hinting any such type as String would thus only affect input. It is therefore up to users | ||
| // themselves to opt in to such assymetrical behavior if they can benefit from that. But | ||
| // nothing saves anyone from having to use a big-numbers aware json parser if they are ever | ||
| // going to consume the results of queries that use such types. | ||
| // | ||
| // See for instance https://neon.tech/blog/parsing-json-from-postgres-in-js. | ||
| ( | ||
| database::ScalarType("float8".to_string()), | ||
| database::TypeRepresentation::Number, | ||
| database::TypeRepresentation::Float64, | ||
| ), | ||
| ( | ||
| database::ScalarType("int2".to_string()), | ||
| database::TypeRepresentation::Integer, | ||
| database::TypeRepresentation::Int16, | ||
| ), | ||
| ( | ||
| database::ScalarType("int4".to_string()), | ||
| database::TypeRepresentation::Integer, | ||
| database::TypeRepresentation::Int32, | ||
| ), | ||
| ( | ||
| database::ScalarType("int8".to_string()), | ||
| database::TypeRepresentation::Integer, | ||
| // ndc-spec defines that Int64 has the json representation of a string. | ||
| // 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, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @danieljharvey here |
||
| ), | ||
| ( | ||
| database::ScalarType("numeric".to_string()), | ||
| database::TypeRepresentation::Number, | ||
| database::TypeRepresentation::BigDecimal, | ||
| ), | ||
| ( | ||
| database::ScalarType("text".to_string()), | ||
| database::TypeRepresentation::String, | ||
| ), | ||
| ( | ||
| database::ScalarType("time".to_string()), | ||
| database::TypeRepresentation::String, | ||
| database::TypeRepresentation::Time, | ||
| ), | ||
| ( | ||
| database::ScalarType("timestamp".to_string()), | ||
| database::TypeRepresentation::String, | ||
| database::TypeRepresentation::Timestamp, | ||
| ), | ||
| ( | ||
| database::ScalarType("timestamptz".to_string()), | ||
| database::TypeRepresentation::String, | ||
| database::TypeRepresentation::Timestamptz, | ||
| ), | ||
| ( | ||
| database::ScalarType("timetz".to_string()), | ||
| database::TypeRepresentation::String, | ||
| database::TypeRepresentation::Timetz, | ||
| ), | ||
| ( | ||
| database::ScalarType("uuid".to_string()), | ||
| database::TypeRepresentation::String, | ||
| database::TypeRepresentation::UUID, | ||
| ), | ||
| ( | ||
| database::ScalarType("varchar".to_string()), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,23 +29,11 @@ pub async fn get_schema( | |
| .into_iter() | ||
| .map(|scalar_type| { | ||
| let result = models::ScalarType { | ||
| representation: metadata.type_representations.0.get(&scalar_type).map( | ||
| |type_representation| match type_representation { | ||
| metadata::TypeRepresentation::Boolean => { | ||
| models::TypeRepresentation::Boolean | ||
| } | ||
| metadata::TypeRepresentation::Integer => { | ||
| models::TypeRepresentation::Integer | ||
| } | ||
| metadata::TypeRepresentation::Number => models::TypeRepresentation::Number, | ||
| metadata::TypeRepresentation::String => models::TypeRepresentation::String, | ||
| metadata::TypeRepresentation::Enum(variants) => { | ||
| models::TypeRepresentation::Enum { | ||
| one_of: variants.clone(), | ||
| } | ||
| } | ||
| }, | ||
| ), | ||
| representation: metadata | ||
| .type_representations | ||
| .0 | ||
| .get(&scalar_type) | ||
| .map(map_type_representation), | ||
| aggregate_functions: metadata | ||
| .aggregate_functions | ||
| .0 | ||
|
|
@@ -584,7 +572,7 @@ fn make_procedure_type( | |
| scalar_types | ||
| .entry("int4".to_string()) | ||
| .or_insert(models::ScalarType { | ||
| representation: Some(models::TypeRepresentation::Integer), | ||
| representation: Some(models::TypeRepresentation::Int32), | ||
| aggregate_functions: BTreeMap::new(), | ||
| comparison_operators: BTreeMap::new(), | ||
| }); | ||
|
|
@@ -626,3 +614,34 @@ fn make_procedure_type( | |
| }, | ||
| } | ||
| } | ||
|
|
||
| /// Map our local type representation to ndc-spec type representation. | ||
| fn map_type_representation( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure we should be changing the outputted type representations like this without the user providing us an opt-in config flag to do so. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This part is just translating our own TypeRepresentation type to ndc-spec TypeRepresentation type. Could you please elaborate on what do you mean? I'm not sure I understand. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was under the impression that our types should return the same There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing the type representations themselves to what they are supposed to be is not a breaking change, and the previous values (Integer, Number) are deprecated. What's breaking is if we decide to say that bigints have the representation Int64, because:
What we do here is to map each type (we know) to its right type representation, except bigints, which we map to json, and this is done in the version3/mod.rs code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What will a version of engine that does not know about these types do when it receives them? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an ndc-schema concept that can be used by cli/lsp to generate engine objects, so I don't think this will be used directly in the engine. The engine already uses v0.1.2 of ndc-spec though: https://github.com/hasura/v3-engine/pull/443/files#diff-5e05c14aa4760c15d75bc66970581abe17a82eef4770a0cd9861bd40ce65934bR27 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh OK, makes sense. This all seems sensible then, thank you for indulging my questions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gladly :) |
||
| type_representation: &metadata::TypeRepresentation, | ||
| ) -> models::TypeRepresentation { | ||
| match type_representation { | ||
| metadata::TypeRepresentation::Boolean => models::TypeRepresentation::Boolean, | ||
| metadata::TypeRepresentation::String => models::TypeRepresentation::String, | ||
| metadata::TypeRepresentation::Float32 => models::TypeRepresentation::Float32, | ||
| 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, | ||
| metadata::TypeRepresentation::Timestamp => models::TypeRepresentation::Timestamp, | ||
| metadata::TypeRepresentation::Timestamptz => models::TypeRepresentation::TimestampTZ, | ||
| metadata::TypeRepresentation::Time => models::TypeRepresentation::String, | ||
| metadata::TypeRepresentation::Timetz => models::TypeRepresentation::String, | ||
| metadata::TypeRepresentation::Date => models::TypeRepresentation::Date, | ||
| metadata::TypeRepresentation::Geometry => models::TypeRepresentation::Geometry, | ||
| metadata::TypeRepresentation::Geography => models::TypeRepresentation::Geography, | ||
| metadata::TypeRepresentation::UUID => models::TypeRepresentation::UUID, | ||
| metadata::TypeRepresentation::Json => models::TypeRepresentation::JSON, | ||
| 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, | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to use the full SHA here because Cargo thinks two dependencies pointing to the same revision but using a different prefix length are actually different dependencies. This causes serious issues when using ndc-postgres as a dependency as it brings in ndc-sdk with the short ref, so everything else needs to use the exact same prefix length.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know. Will fix.