Add support for reflection of collation in types on PostgreSQL #12510
Conversation
af277b9 to
baf48b5
Compare
|
sorry for not yet getting a look at this. will try to come to it |
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision baf48b5 of this pull request into gerrit so we can run tests and reviews and stuff
|
New Gerrit review created for change baf48b5: https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5993 |
There was a problem hiding this comment.
Federico Caselli (CaselIT) wrote:
code review left on gerrit
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5993
| @@ -3801,6 +3802,19 @@ def _columns_query(self, schema, has_filter_names, scope, kind): | |||
| == pg_catalog.pg_attribute.c.attnum, | |||
| ), | |||
| ) | |||
| .join( | |||
There was a problem hiding this comment.
Federico Caselli (CaselIT) wrote:
Note to self: this could take advantage of the change in the style of the one added here https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5899
will try to check if it makes sense
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5993
There was a problem hiding this comment.
Added an extra commit to implement this idea (thus removing the JOINs); and rebased.
There was a problem hiding this comment.
Except I did it wrong; sorry. I will get back to this in a couple of weeks.
There was a problem hiding this comment.
no problem, note that I make take a loot at this before (since it blocks your other prs, right?), in that I'll write about it here.
thanks for the effort in any case
There was a problem hiding this comment.
I think I fixed it; apparently array_agg(<OIDs>) requires a cast in pg8000.
No worries about other PRs, I'll rework them later.
There was a problem hiding this comment.
Yes, I had to do the same here c3fb776
|
Fixed test failure for empty table case. |
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 62d03e7 of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset 62d03e7 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5993 |
|
Rebased to get a working flake8 setup (5c61cc6). |
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision c6ba75f of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset c6ba75f added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5993 |
The 'collation' returned by PGDialect._columns_query() is None if it matches the default collation for the type. On the other hand, if the column collation matches the one of the database but is explicitly set at column creation, the value is reflected. Related to sqlalchemy#6511. Closes: sqlalchemy#12510 Pull-request: sqlalchemy#12510 Pull-request-sha: c6ba75f Change-Id: I269a194d526a0689a4b38f10456d28539c73cffb
There was a problem hiding this comment.
Denis Laxalde (dlax) wrote:
code review left on gerrit
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5993
- lib/sqlalchemy/dialects/postgresql/base.py (line 3805): Done
2725b36 to
4735f0e
Compare
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 4735f0e of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset 4735f0e added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5993 |
The 'collation' returned by PGDialect._columns_query() is None if it matches the default collation for the type. On the other hand, if the column collation matches the one of the database but is explicitly set at column creation, the value is reflected. Related to sqlalchemy#6511.
|
@CaselIT, would you mind pulling the latest version in gerrit to get CI jobs running? Thanks. |
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 896aa73 of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset 896aa73 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5993 |
|
sure, sorry if this is taking a bit longer to merge. |
Following a similar approach to 703a323. The CAST(..., BIGINT) for array_agg()'s elements is needed for pg8000.
|
New version to fix an issue spotted by alembic tests. |
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 4c2ba73 of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset 4c2ba73 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5993 |
The 'collation' returned is
Noneif it matches the default collation for the type. On the other hand, if the column collation matches the one of the database but is explicitly set at column creation, the value is reflected.Related to #6511.