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

DISTKEY and SORTKEY should be in quotations #74

Merged
merged 1 commit into from Apr 20, 2016

Conversation

@solackerman
Copy link
Contributor

@solackerman solackerman commented Dec 6, 2015

Redshift column names can contain spaces. A column name like Foo Bar will become a sql statement looking like this

SORTKEY (Foo Bar)

which will fail. This PR quotify's DISTKEY and SORTKEY, so they will appear like this:

SORTKEY ("Foo Bar")
@solackerman solackerman force-pushed the solackerman:master branch 3 times, most recently from 7b1a724 to 16aa366 Dec 6, 2015
@graingert
Copy link
Collaborator

@graingert graingert commented Dec 6, 2015

@solackerman can you create a model + reflection tests with a column name with spaces in?
eg:

class SortKeyDistKeyWithSpaces(Base):
    __tablename__ = 'sort_key_with_spaces'
    col1 = sa.Column('col with spaces', sa.Integer(), primary_key=True)
    __table_args__ = {
        'redshift_diststyle': 'EVEN',
        'redshift_sortkey': 'col with spaces',
        'redshift_distkey': 'col with spaces',
    }
@solackerman solackerman force-pushed the solackerman:master branch from 16aa366 to 799e8d8 Dec 6, 2015
@solackerman
Copy link
Contributor Author

@solackerman solackerman commented Dec 6, 2015

done.

@graingert
Copy link
Collaborator

@graingert graingert commented Jan 22, 2016

@jklukas any thoughts?

@@ -255,7 +255,8 @@ def post_create_table(self, table):
for key in keys]
if interleaved_sortkey:
text += " INTERLEAVED"
text += " SORTKEY ({0})".format(", ".join(keys))
text += " SORTKEY ({0})".format(
", ".join('"' + key + '"' for key in keys))

This comment has been minimized.

@jklukas

jklukas Jan 22, 2016
Collaborator

I think we can have our cake and eat it too. SQLAlchemy's IdentifierPreparer.quote method exists to conditionally quote identifiers. I think we should use that here rather than putting in explicit quotes. It would be safer (there might be some edge cases we aren't considering if column names contain quotes themselves) and it means quotes aren't added to identifiers unless necessary, which would make output more natural.

So I think we could use something like:

sortkey_string = ", ".join(self.preparer.quote(key) for key in keys)
text += " SORTKEY ({0})".format(sortkey_string)
@jklukas
Copy link
Collaborator

@jklukas jklukas commented Jan 22, 2016

Thanks for finding this bug and putting together the PR, @solackerman. Would you agree that conditional quoting is the way to go?

@solackerman
Copy link
Contributor Author

@solackerman solackerman commented Jan 25, 2016

Totally agree, that's much more elegant. I'll update the PR shortly.

@solackerman
Copy link
Contributor Author

@solackerman solackerman commented Jan 25, 2016

done.

@jklukas
Copy link
Collaborator

@jklukas jklukas commented Jan 25, 2016

This looks great! +1

@@ -44,7 +44,7 @@ class ReflectionSortKey(Base):
col2 = sa.Column(sa.Integer())
__table_args__ = (
{'redshift_diststyle': 'EVEN',
'redshift_sortkey': ('col1, col2')}
'redshift_sortkey': ('col1', 'col2')}

This comment has been minimized.

@graingert

graingert Jan 25, 2016
Collaborator

Is there any chance of this being a breaking change?

This comment has been minimized.

@graingert

graingert Jan 25, 2016
Collaborator

Might be worth documenting the fact you can no longer include two columns in one string in the sortkey

This comment has been minimized.

@jklukas

jklukas Jan 25, 2016
Collaborator

I'd agree that this is a breaking change that should be included in the CHANGES.rst

@jklukas
Copy link
Collaborator

@jklukas jklukas commented Jan 25, 2016

@graingert - Do you want to give a final +1 before we merge?

I just remembered that we should probably have a note in the changelog about this as well. We should probably have a CONTRIBUTING.md to write a checklist for PRs.

@graingert
Copy link
Collaborator

@graingert graingert commented Jan 25, 2016

👍

@graingert
Copy link
Collaborator

@graingert graingert commented Apr 19, 2016

@solackerman can you update the CHANGELOG because this is a breaking change.

@solackerman solackerman force-pushed the solackerman:master branch from 7c66fdb to e1cc9c6 Apr 20, 2016
@solackerman
Copy link
Contributor Author

@solackerman solackerman commented Apr 20, 2016

Done.

@jklukas
Copy link
Collaborator

@jklukas jklukas commented Apr 20, 2016

+1

@graingert
Copy link
Collaborator

@graingert graingert commented Apr 20, 2016

@solackerman can you squash these together into one, please.

@solackerman solackerman force-pushed the solackerman:master branch from e1cc9c6 to ffe235a Apr 20, 2016
@solackerman
Copy link
Contributor Author

@solackerman solackerman commented Apr 20, 2016

done.

@graingert graingert merged commit 3fa5f3d into sqlalchemy-redshift:master Apr 20, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@solackerman
Copy link
Contributor Author

@solackerman solackerman commented Apr 20, 2016

will there be a new release coming soon?

@graingert
Copy link
Collaborator

@graingert graingert commented Apr 21, 2016

@solackerman 0.5.0 is out now

@solackerman
Copy link
Contributor Author

@solackerman solackerman commented Apr 21, 2016

Amazing! Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.