Skip to content

Conversation

@soupi
Copy link
Contributor

@soupi soupi commented Mar 29, 2024

What

ndc-spec 0.1.1 introduce an optional type representation hint. In this PR we provide type representation for enum values.

How

  1. We add a new TypeRepresentations field to our metadata, similar to aggregate functions and comparison operators
  2. We already query for the enum types and variants, now we also return those from sql to rust and into the metadata.
  3. For each scalar type, we try to look for their name in type reps, and return it if we find it.

@soupi soupi requested a review from plcplc March 29, 2024 15:25
Copy link
Contributor

@plcplc plcplc left a comment

Choose a reason for hiding this comment

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

Looks good to me~

.entry("int4".to_string())
.or_insert(models::ScalarType {
representation: None,
representation: Some(models::TypeRepresentation::Integer),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really something of an edge case. I wonder if we actually need to register int4, as in, I wonder if there actually is a functional difference between "scalarTypes":{"int4": {"aggregateFunctions": {}, "comparisonOperators": {}} and "scalarTypes": {}.

At any rate, it is a curious detail that this edge case gives a different result than the common case where int4 is defined, but we don't yet assign type representations to anything but enums.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we definitely need it. I think I added it because a test failed. Try and see :)

The comments about this are not visible in this diff by default, adding it here:

    // If int4 doesn't exist anywhere else in the schema, we need to add it here. However, a user
    // can't filter or aggregate based on the affected rows of a procedure, so we don't need to add
    // any aggregate functions or comparison operators. However, if int4 exists elsewhere in the
    // schema and has already been added, it will also already contain these functions and
    // operators.

@soupi soupi enabled auto-merge April 1, 2024 09:57
@soupi soupi added this pull request to the merge queue Apr 1, 2024
Merged via the queue into main with commit 5ec39c0 Apr 1, 2024
@soupi soupi deleted the gil/enum-type-reps branch April 1, 2024 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants