Skip to content
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

Expand use of QualifiedName to types, composites, enums #263

Merged
merged 1 commit into from
Jul 23, 2023

Conversation

shane-circuithub
Copy link
Contributor

@shane-circuithub shane-circuithub commented Jul 15, 2023

Types in PostgreSQL can also be qualified with a schema. However, it's not sufficient to just change the type of TypeInformation's typeName to QualifiedName, because a type isn't just a name. Postgres types can also be parameterised by modifiers (e.g., numeric(7, 2)) and array types of arbitrary depth (e.g., int4[][]).

To accomodate this, a new type is introduced, TypeName. Like QualifiedName, it has an IsString instance, so the common case (schema set to Nothing, no modifiers, scalar type) will continue working as before.

@shane-circuithub shane-circuithub changed the title Type name Expand use of QualifiedName to types, composites, enums Jul 15, 2023
Types in PostgreSQL can also be qualified with a schema. However, it's not sufficient to just change the type of `TypeInformation`'s `typeName` to `QualifiedName`, because a type isn't *just* a name. Postgres types can also be parameterised by modifiers (e.g., `numeric(7, 2)`) and array types of arbitrary depth (e.g., `int4[][]`).

To accomodate this, a new type is introduced, `TypeName`. Like `QualifiedName`, it has an `IsString` instance, so the common case (`schema` set to `Nothing`, no modifiers, scalar type) will continue working as before.
Copy link
Contributor

@ocharles ocharles left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines +100 to +105
, typeName =
TypeName
{ name = "bpchar"
, modifiers = ["1"]
, arrayDepth = 0
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to not just stay with char?

Copy link
Contributor Author

@shane-circuithub shane-circuithub Jul 22, 2023

Choose a reason for hiding this comment

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

Because the underlying type is actually called bpchar, not char. Now that we quote things properly, the SQL we produce for lit '👍' will be '👍'::"char". However, in PostgreSQL, char and "char" are different types, with the former meaning bpchar latter meaning a single ASCII character, so if we don't change this to bpchar then lit '👍' == lit '👎'.

Comment on lines +19 to +21
- `TypeInformation`'s `typeName` parameter from `String` to `TypeName`.
- `DBEnum`'s `enumTypeName` method from `String` to `QualifiedName`.
- `DBComposite`'s `compositeTypeName` method from `String` to `QualifiedName`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming this is true:

Suggested change
- `TypeInformation`'s `typeName` parameter from `String` to `TypeName`.
- `DBEnum`'s `enumTypeName` method from `String` to `QualifiedName`.
- `DBComposite`'s `compositeTypeName` method from `String` to `QualifiedName`.
- `TypeInformation`'s `typeName` parameter from `String` to `TypeName`.
- `DBEnum`'s `enumTypeName` method from `String` to `QualifiedName`.
- `DBComposite`'s `compositeTypeName` method from `String` to `QualifiedName`.
- Both `TypeName` and `QualifiedName` have `IsString` instances which have the same behavior as the original `String` types they now replace.

Copy link
Contributor Author

@shane-circuithub shane-circuithub Jul 22, 2023

Choose a reason for hiding this comment

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

Eh, kind of. If your string represents an unqualified name with no uppercase or special characters (or modifiers or array nonsense in the case of TypeName), then yes. Otherwise, no. Someone could previously have set typeName to a String like "\"mySchema\".\"myType\"(4)[][]" — we make no attempt to parse that into a TypeName.

@shane-circuithub shane-circuithub merged commit c06bd5f into master Jul 23, 2023
2 checks passed
@shane-circuithub shane-circuithub deleted the TypeName branch July 23, 2023 16:12
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