cast int64 and numeric to string by default #416

soupi · 2024-04-09T14:49:31Z

What

We want to be able to stringify fields of types int8 and numeric in the query response so that we have better interoperability with javascript clients.

This PR changes the default type representation of int8 and numeric types (maintaining the existing type representation if one exists) and stringify numbers.

This option can be change via the configuration by editing the mapping in the metadata.typeRepresentations:

  { ...
    "metadata": {
      ...
      "typeRepresentations": {
        "Phone": "string",
        "CardSuit": {
          "enum": ["hearts", "clubs", "diamonds", "spades"]
        },
        ...
-       "int8": "int64",
+       "int8": "int64AsString",
-       "numeric": "bigDecimal",
+       "numeric": "bigDecimalAsString",
        ...
      }
      ...
    }
  }

How

We introduce new type representations: Int64AsString, BigDecimalAsString.
Make sure we use the existing type representation configuration and override the default if it exists.
In field selection after getting an expression, we cast it if the type representation requires it. (For arrays, we cast to cast_type[])
For composite types we unpack them if the don't specify fields, and cast required fields recursively.

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

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

plcplc · 2024-04-10T10:55:13Z

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

+fn unpack_and_wrap_fields(
+    env: &Env,
+    state: &mut State,
+    current_table: &TableNameAndReference,
+    join_relationship_fields: &mut Vec<relationships::JoinFieldInfo>,
+    column: &str,
+    alias: sql::ast::ColumnAlias,
+    type_info: &CollectionOrCompositeTypeInfo<'_>,
+    nested_field_joins: &mut Vec<JoinNestedFieldInfo>,
+) -> Result<(sql::ast::ColumnAlias, sql::ast::Expression), Error> {


I think that I would be partial to inlining this function in the one place where it is used, since its body has to take part in the somewhat complex join_relationship_fields/nested_field_joins handling.

I actually pulled it out because that function has gotten quite big and found this to be more digestible, but we can talk about this if you really prefer it to be inlined.

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

soupi · 2024-04-11T09:53:56Z

...napshots/databases_tests__aurora__query_tests__basic__select_composite_variable_complex.snap

          "address": {
            "address_line_1": "Somstreet 159",
            "address_line_2": "Second door to the right"
+          },


Previously, when a QueryRequest requested a nested field column without specifying fields, we would let postgres decide on the field ordering. Now we explicitly request fields in such cases, so they are ordered according to the BTreeMap ordering.

soupi · 2024-04-11T09:54:39Z

...tests/src/citus/snapshots/databases_tests__citus__schema_tests__schema_test__get_schema.snap

@@ -2926,7 +2926,7 @@ expression: result
        "only_occurring_here1": {
          "type": {
            "type": "named",
-            "name": "bit"
+            "name": "int8"


I changed our sql definition so I can test stuff. Sorry.

soupi · 2024-04-11T09:56:17Z

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

@@ -3335,7 +3335,7 @@
      "float8": "float64",
      "int2": "int16",
      "int4": "int32",
-      "int8": "json",
+      "int8": "int64",


decided to only set postgres to int64AsString so we can test both behaviours.

soupi · 2024-04-11T09:57:07Z

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

+      "int8": "int64AsString",
+      "numeric": "bigDecimalAsString",


changing the postgres config (only) so we can test this change in both modes.

soupi · 2024-04-11T09:58:01Z

crates/query-engine/translation/tests/snapshots/tests__select_composite_column_simple.snap

-          ) AS "%3_rows"
-      ) AS "%3_rows"
-  ) AS "%2_universe";
+              LEFT OUTER JOIN LATERAL (


We now unpack fields ourselves so we can cast them if needed yey...

soupi · 2024-04-11T10:02:10Z

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

+        // 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,


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

soupi · 2024-04-11T12:27:24Z

scripts/archive-old-ndc-metadata.sh

fix so it will work when the hash already exists as well.

soupi · 2024-04-15T14:14:08Z

...s/snapshots/databases_tests__postgres__query_tests__basic__select_array_composite_field.snap

A test of array of composite types.

SamirTalwar · 2024-04-16T08:39:47Z

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

+            .type_representations
+            .0
+            .get(scalar_type)
+            .cloned()


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.

smart. thanks!

SamirTalwar · 2024-04-16T08:46:22Z

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

+                sql::ast::Expression::ColumnReference(nested_column_reference),
+            ))
+        }
+        // TODO: Arrays of composite types are not handled yet.


I am confused; it looks like they are. Should this say "Arrays of arrays are not handled yet"?

Thanks! stale comment.

SamirTalwar · 2024-04-16T08:47:02Z

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

+        // In these situations the type representation should be the same as
+        // the expression, so we don't cast it.
+        TypeRepresentation::Boolean => None,
+        TypeRepresentation::String => None,
+        TypeRepresentation::Float32 => None,
+        TypeRepresentation::Float64 => None,
+        TypeRepresentation::Int16 => None,
+        TypeRepresentation::Int32 => None,
+        TypeRepresentation::Int64 => None,
+        TypeRepresentation::BigDecimal => None,
+        TypeRepresentation::Timestamp => None,
+        TypeRepresentation::Timestamptz => None,
+        TypeRepresentation::Time => None,
+        TypeRepresentation::Timetz => None,
+        TypeRepresentation::Date => None,
+        TypeRepresentation::UUID => None,
+        TypeRepresentation::Geography => None,
+        TypeRepresentation::Geometry => None,
+        TypeRepresentation::Json => None,
+        TypeRepresentation::Enum(_) => None,


I appreciate you listing these out rather than using a wildcard.

You can use | to join them, though (there's a pedantic warning for this but we haven't enabled it).

cool. Thanks!

cast int64 to string

efd5f64

plcplc reviewed Apr 10, 2024

View reviewed changes

crates/query-engine/translation/src/translation/helpers.rs Outdated Show resolved Hide resolved

plcplc reviewed Apr 10, 2024

View reviewed changes

Gil Mizrahi added 3 commits April 11, 2024 12:45

Refactor, rename type reps, use user defs

f8dc828

comment

3e628cf

comment

d7097a1

soupi commented Apr 11, 2024

View reviewed changes

Gil Mizrahi added 4 commits April 11, 2024 13:02

comments

ca7d2e3

changelog

bedf73d

fix arrays

8f90b96

merge array and column casting

0314d62

soupi marked this pull request as ready for review April 11, 2024 10:27

Gil Mizrahi added 4 commits April 11, 2024 13:30

Merge branch 'main' into gil/int64-string

36ab489

fix merge

55dc327

oops

867a734

archive old data no-op

0df5ca1

soupi commented Apr 11, 2024

View reviewed changes

scripts/archive-old-ndc-metadata.sh Outdated

Copy link

Contributor Author

soupi Apr 11, 2024

Choose a reason for hiding this comment

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

fix so it will work when the hash already exists as well.

soupi changed the title ~~cast int64 to string~~ cast int64 and numeric to string by default Apr 11, 2024

update enums

3d56da8

soupi mentioned this pull request Apr 11, 2024

pg add type representations information hasura/ddn-docs#368

Merged

Gil Mizrahi added 2 commits April 15, 2024 16:15

simplify scalar type

9ef3d24

array of composite type support

7fa9b4f

soupi commented Apr 15, 2024

View reviewed changes

SamirTalwar approved these changes Apr 16, 2024

View reviewed changes

review

8a7a2d7

soupi enabled auto-merge April 16, 2024 08:58

soupi added this pull request to the merge queue Apr 16, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 16, 2024

Merge branch 'main' into gil/int64-string

f1f15ce

soupi enabled auto-merge April 16, 2024 09:04

fix snaps

f3ca583

soupi added this pull request to the merge queue Apr 16, 2024

Merged via the queue into main with commit 819e19d Apr 16, 2024
35 checks passed

soupi deleted the gil/int64-string branch April 16, 2024 09:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cast int64 and numeric to string by default #416

cast int64 and numeric to string by default #416

soupi commented Apr 9, 2024 •

edited

Loading

plcplc Apr 10, 2024

soupi Apr 10, 2024

soupi Apr 11, 2024

soupi Apr 11, 2024

soupi Apr 11, 2024

soupi Apr 11, 2024

soupi Apr 11, 2024

soupi Apr 11, 2024

soupi Apr 11, 2024

soupi Apr 15, 2024

SamirTalwar Apr 16, 2024

soupi Apr 16, 2024

SamirTalwar Apr 16, 2024

soupi Apr 16, 2024

SamirTalwar Apr 16, 2024

soupi Apr 16, 2024

cast int64 and numeric to string by default #416

cast int64 and numeric to string by default #416

Conversation

soupi commented Apr 9, 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

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 Apr 9, 2024 •

edited

Loading