@graingert when you get a chance can you close #1? thanks!

what's needed to merge this PR?

1. use sa.text.
2. add a test

anything else?

needs a test, and needs some review for SQL injection possibilities

I know it's not your code, but I think this PR is a good excuse to clean this function up a bit.

@graingert how do you feel about adding enum34 as a dependency so that we can have something like this:

import enum


class Format(enum.Enum):
    CSV = 0
    JSON = 1


class Compression(enum.Enum):
    GZIP = 0
    LZOP = 1
    # more as needed

Could do something like assert format in ['JSON', 'CSV'] rather than an enum

It doesn't appear that there's a way to use bindparams with the session_token parameter:

In [1]: sa.text(':secret_key:session_token').bindparams(secret_key='a', session_token='b')
ArgumentError: This text() construct doesn't define a bound parameter named 'secret_key'

The format for credentials is:

aws_access_key_id=<access-key-id>;aws_secret_access_key=<secret-access-key>[;token=<temporary-session-token>]

I think the credentials string will need to be formatted in python and bound as one single SQL parameter

fine by me

looks nice, could do with some tests though. Don't worry about uploading files to S3 to the real redshift instance for now. Just some simple compiles with verification of the text produced:

You can use http://docs.sqlalchemy.org/en/latest/faq/sqlexpressions.html#how-do-i-render-sql-expressions-as-strings-possibly-with-bound-parameters-inlined to do the compile including bound params.

i added test_format, test_compression, test_invalid_format and test_invalid_compression. What else do you want me to add?

Or rather, what else do you want me to test?

Ah sorry I didn't spot the tests.

This changes the interfaces (to something I'd prefer), so we'll have to get some consensus with other users of this code.

Another possibility is to rename this implementation as NewCopyCommand (name TBD) and adapt the old CopyCommand interface to call this NewCopyCommand

@jklukas @thisfred @bouk: Thoughts?

I had no idea so many people were developing this library! Awesome.

no.

you can't pass kwargs like that to sa.text

it doesn't work for the same reasons as stated above about escaping

It shouldn't be escaped at all. It should come through as a literal null byte

Is there a place that we can chat? I feel like this is moving slower than it could.

sqla irc?

Yup my contact details are here https://graingert.co.uk/foaf.rdf
On 26 Aug 2015 21:19, "Phillip Cloud" notifications@github.com wrote:

sqla irc?

—
Reply to this email directly or view it on GitHub
#21 (comment)
.

i'm in the sqla irc

I still don't see why setting compiler.dialect._backslash_escapes to False and then resetting it back to the default for the dialect isn't the right solution here. No matter what the approach is (overriding compiler.render_literal_value, or a custom type), it will involve turning off _backslash_escapes and then turning it back to the default.

Can we please get this merged today. This is dragging on far too long.

it will involve turning off _backslash_escapes and then turning it back to the default.

I will not accept anything that breaks the thread-safety of the query compiler. Let's just use .format() here

Also, setting private variables is a no-no

Let's just use .format() here.

For everything, or just for null?

Also, setting private variables is a no-no

I'm aware of that.

I will not accept anything that breaks the thread-safety of the compiler.

Can you give an example of how this breaks thread safety? Is there no public state on the compiler that can be set by a user during compilation?

Just for null, call the parameter "dangerous_null_delimiter=None" and only add the "NULL AS " if it's not None.

Nice example.

call the parameter "dangerous_null_delimiter=None"

is that a joke?

nope

Thanks, but that's unnecessary.

Cool looks like this is ready to go, can you clean up your commits (squash and rebase as appropriate)?

Sure

@graingert Would you like me to squash further?

probably best for one commit here I think?

done

@jklukas @thisfred @bouk: I'm going to merge this. If you've got any objections we can change them in another PR.

@graingert thanks for your patience.

@graingert no, thank you for your patience, feature and improvements to the code!