Split on first period only. Closes #77. #78
Conversation
jseabold
force-pushed
the
fix-schema-split
branch
from
8c84a1f
to
ffe5ca8
Compare
|
Can you create a reflection test for this? |
|
Yeah, I'll give it a shot. |
|
Perhaps it's cleaner to make relname always a delimited identifier? |
jseabold
force-pushed
the
fix-schema-split
branch
from
ffe5ca8
to
12cbe58
Compare
|
I added a test but I can't actually get it to fail without the subsequent two commits. Anything obvious to you? Edit I removed the delimiting the relname so that there aren't quotes on everything in the MetaData registry. |
jseabold
force-pushed
the
fix-schema-split
branch
from
12cbe58
to
a71a3d6
Compare
|
This is semi-related to #74 about things generally needing to be delimited for safety. |
|
Any thoughts? |
| @@ -102,7 +102,7 @@ def _get_relation_key(name, schema): | |||
| def _get_schema_and_relation(key): | |||
| if '.' not in key: | |||
| return (None, key) | |||
| identifiers = SQL_IDENTIFIER_RE.findall(key) | |||
| identifiers = re.split("\.", key, 1) | |||
There was a problem hiding this comment.
If the table name is "this.table", it will always get split up based on this regex, even if no schema name is prepended. So I think this is broken.
There was a problem hiding this comment.
Or if I'm mistaken here, could you provide a test that verifies you can call _get_schema_and_relation with "this.table" and get back (None, '"this.table"')?
There was a problem hiding this comment.
Sorry I missed this comment. Yeah, you're right. This does not handle delimited identifiers well. Let me have a look.
|
@jseabold bump |
|
Are a lot of these tests being skipped? I was trying to figure out why my tests weren't failing when they were expected to and py.test is skipping them. The travis logs seem to indicate that these same tests are being skipped on travis. Any objections to running the tests verbosely on travis? |
|
The tests that are skipped require a real redshift server Are a lot of these tests being skipped? I was trying to figure out why my — |
|
Right. So where do they get run, if ever? Do I need to test them against my own server? |
|
We run it on commits on branches in this repo. I can pull your changes into
|
|
Ok, thanks. That wasn't clear to me. |
jseabold
force-pushed
the
fix-schema-split
branch
from
a71a3d6
to
e02f4fa
Compare
|
Let's see how this looks. |
|
Hmm, I think I've actually lost sight of the original issue with the code comment above. Since the tables are coming from Edit: let me give it one more go. |
Use a schema name explicitly
jseabold
force-pushed
the
fix-schema-split
branch
from
e02f4fa
to
a7b62d2
Compare
|
Ok...I'm remembering the issue again... Why are you doing this? Honest question, I don't know. It seems like the only way you can have a key that's table.name is because you set the schema to None. If you don't do this, you don't have the problem. All of the quotes get stripped when the table information is stored, so we can't reliably use them in the regex and always splitting on the first period should work if you don't remove the schema. |
|
Why does |
| # the delimited identifier dox is silent on whether the following is | ||
| # possible | ||
| (("\"crazy schema\".\"crazy table\""), | ||
| ("\"crazy schema\"", "\"crazy table\"")), |
There was a problem hiding this comment.
These strings would be easier to read by delimiting with single quotes so that the double quotes inside the strings don't need to be escaped ('other_schema."table_name"', etc.)
That's a fair point, and I don't think there's any reason it can't. I'll take a look at what would be needed for that change. |
|
See #97 which pulls in the commits here, but factors out table references to a new |
Don't know if ideal, but fixes my problem described in #77.