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

Should `types::Text` be an alias for `types::VarChar`? #287

Closed
sgrif opened this Issue Apr 20, 2016 · 1 comment

Comments

Projects
None yet
1 participant
@sgrif
Member

sgrif commented Apr 20, 2016

Right now these are different types, with separate bindings for all related things. As things stand right now, functions can only take arguments of a single type. For example, you can pass an expression of type VarChar to lower, but not one of type Text. We could certainly implement it in such a way that we use a trait to mark valid argument types, but that would end up with ambiguity when you do lower("STRING LITERAL") since that could be an expression of either type, and I'm not sure that type inference will always be able to save us.

All backends seem to treat these as pretty much identical. The only case that I can see where they differ is in things like limits, which we cannot enforce on the Diesel side1. Are there backends which treat them differently, either for valid arguments to functions/operators or for serialization/deserialization? If not, I think we should just make these aliases for one another.

1Ok, we technically could enforce this, and it would be more correct. But this is the same as how technically we shouldn't provide impl ToSql<VarChar, Pg> for String because PG doesn't allow nul bytes so we should only allow CString technically. While we do open up one case for runtime errors, the useability trade off is more than worth it for me.

@sgrif

This comment has been minimized.

Member

sgrif commented Apr 20, 2016

Explored this a bit. Tests mostly passed. Only place I had to make a change outside of Diesel's code was here, as VarChar can no longer be referenced as a struct. The type argument was never actually used though, so it was trivial to eliminate and replace with PhantomData instead.

I also dug into how this might affect us on the supported backends. SQLite we already know for sure won't be affected, as there is on varchar or text type, only the text affinity. For PG, things are kinda similar. We already know that they're treated the same under the hood. In the public API, it's a similar story. The varchar type (oid 1043) basically is never used. All functions or operators work with the text type (oid 25) instead.

[local] sean@sean=# select count(*) from pg_operator where oprleft = 1043;
 count
-------
     0

[local] sean@sean=# select count(*) from pg_operator where oprleft = 25;
 count
-------
    22

[local] sean@sean=# select count(*) from pg_proc where 1043 = any(proallargtypes);
 count
-------
     0

[local] sean@sean=# select count(*) from pg_proc where 25 = any(proallargtypes);
 count
-------
    41

This works because there's a cast defined between the two types.

[local] sean@sean=# select * from pg_cast where castsource = 1043 and casttarget = 25;
 castsource | casttarget | castfunc | castcontext | castmethod
------------+------------+----------+-------------+------------
       1043 |         25 |        0 | i           | b

The wrinkle here is that the same is not true for varchar[] and text[]. This doesn't affect the most common cases, like insert/update. It does however affect all functions and operators. If the type of a column is varchar[] on the db side, doing filter(column.eq(vec!["1"])) would result in ERROR: 42883: operator does not exist: character varying[] = text[].

We could maybe work around that by doing some sort of internal nonsense that only affects arrays. While this might be an annoying gotcha, I think it's relatively low cost to just have the situation be "if you're using a text array, use text[] not varchar[]". The gains elsewhere are worth it.

sgrif added a commit that referenced this issue Apr 20, 2016

Change `VarChar` to be an alias to `Text`
This removes the `VarChar` type entirely, unifying it with text.
This simplifies the interaction with things like SQL functions, and
removes gotchas like `lower` not working with text columns. SQLite will
be unaffected by this change. PG is mostly unaffected. However, it
doesn't treat `varchar[]` arrays as equivalent to `text[]`. INSERT and
UPDATE will continue to work, but other operators/functions will error
out. Since they have identical representations in the database, I'm fine
with introducing the weird ideosynchracy of "use `text[]` instead of
`varchar[]`, as we remove some much larger warts.

There is some more in depth research/reasoning at #287.

One other visible effect here is that `VarChar` can no longer be
referenced in expression positions, as it is not a struct. This can be
seen in the change made to `diesel_tests::schema_dsl`. Since the structs
have no size and no method, there's never any reason to use them in
expression positions, and I've simply replaced that field entirely.

Close #287

sgrif added a commit that referenced this issue Apr 20, 2016

Change `VarChar` to be an alias to `Text`
This removes the `VarChar` type entirely, unifying it with text.
This simplifies the interaction with things like SQL functions, and
removes gotchas like `lower` not working with text columns. SQLite will
be unaffected by this change. PG is mostly unaffected. However, it
doesn't treat `varchar[]` arrays as equivalent to `text[]`. INSERT and
UPDATE will continue to work, but other operators/functions will error
out. Since they have identical representations in the database, I'm fine
with introducing the weird ideosynchracy of "use `text[]` instead of
`varchar[]`, as we remove some much larger warts.

There is some more in depth research/reasoning at #287.

One other visible effect here is that `VarChar` can no longer be
referenced in expression positions, as it is not a struct. This can be
seen in the change made to `diesel_tests::schema_dsl`. Since the structs
have no size and no method, there's never any reason to use them in
expression positions, and I've simply replaced that field entirely.

Close #287

@sgrif sgrif closed this in #288 Apr 20, 2016

sgrif added a commit that referenced this issue Apr 20, 2016

Change `VarChar` to be an alias to `Text` (#288)
This removes the `VarChar` type entirely, unifying it with text.
This simplifies the interaction with things like SQL functions, and
removes gotchas like `lower` not working with text columns. SQLite will
be unaffected by this change. PG is mostly unaffected. However, it
doesn't treat `varchar[]` arrays as equivalent to `text[]`. INSERT and
UPDATE will continue to work, but other operators/functions will error
out. Since they have identical representations in the database, I'm fine
with introducing the weird ideosynchracy of "use `text[]` instead of
`varchar[]`, as we remove some much larger warts.

There is some more in depth research/reasoning at #287.

One other visible effect here is that `VarChar` can no longer be
referenced in expression positions, as it is not a struct. This can be
seen in the change made to `diesel_tests::schema_dsl`. Since the structs
have no size and no method, there's never any reason to use them in
expression positions, and I've simply replaced that field entirely.

Close #287
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment