Postgres: make sure table from the public schema doesn't get merged with table from other schemas #4224
Conversation
…ith a table from another schema.
|
|
||
|
|
||
| def build_schema(query_result, schema): | ||
| # By default we omit the public schema name from the table name. But there are |
There was a problem hiding this comment.
Can you convert this to a heredoc or simply link to this PR's description?
| # (while this feels unlikely, this actually happened) | ||
| # In this case if we omit the schema name for the public table, we will have | ||
| # a conflict. | ||
| table_names = set(map(lambda r: full_table_name(r['table_schema'], r['table_name']), query_result['rows'])) |
There was a problem hiding this comment.
IIRC, as part of the Python 3 PR, there's a massive conversion of maps to comprehensions. While we can argue on which is nicer (hey, I'm a map guy!), I do feel should consider a consolidation of styles.
There was a problem hiding this comment.
@NicolasLM is this something 2to3 does on its own?
Until python-3 hits master branch and as we merge new code, I would like to reduce the amount of manual changes you need to do. So if there are anything like this that you need to manually update, please let us know so we are mindful of it.
There was a problem hiding this comment.
Yes, 2to3 replaces maps with list comprehensions (but only when the first argument given to map is a lambda). This is because in Python 3 map returns a lazy iterator while in Python 2.7 it returns a list, so 2to3 tries to preserve the return type. A similar thing happens with dict.keys() or dict.values().
In this specific example it does not matter since everything is wrapped in a set just after, so it will work in both Python 2.7 and Python 3.
I don't think you need to bother too much about this until the python-3 branch is merged as these incompatibilities are usually caught early on in tests after a rebase.
| if row['table_schema'] != 'public': | ||
| table_name = full_table_name(row['table_schema'], row['table_name']) | ||
| else: | ||
| if row['table_name'] in table_names: | ||
| table_name = full_table_name(row['table_schema'], row['table_name']) | ||
| else: | ||
| table_name = row['table_name'] |
There was a problem hiding this comment.
Since this is an edge case, maybe it'll be best if it has less distraction?
| if row['table_schema'] != 'public': | |
| table_name = full_table_name(row['table_schema'], row['table_name']) | |
| else: | |
| if row['table_name'] in table_names: | |
| table_name = full_table_name(row['table_schema'], row['table_name']) | |
| else: | |
| table_name = row['table_name'] | |
| table_name = row['table_name'] | |
| if row['table_schema'] != 'public' or table_name in table_names: | |
| table_name = full_table_name(row['table_schema'], row['table_name']) |
There was a problem hiding this comment.
I remember having some issue with doing it this way, but looking at it now it seems legit. 🤷♂️
What type of PR is this? (check all applicable)
Description
This pull request addresses an edge case in Postgres' schema handling:
By default we omit the public schema name from the table name. But there are edge cases, where this might cause conflicts. For example:
(while this feels unlikely, this actually happened)
In this case if we omit the schema name for the public table, we will have a conflict. This pull reuqest makes sure that if such collision exists, we will name the public schema table as
public."main.users"(quotes are needed because of the dot).This is probably relevant to other data sources, but for now limiting the change to the Postgres data source to make the change smaller and simpler.