make delimiter optional

[jfyu]

This PR aims to close #39 .

Changes

• allowed delim or delimiter options in unload_and_copy and load_and_copy to be None, in which case we will ignore that parameter
• adjusted tests to that case
• if PARQUET in the options, we remove the default copy options because they don't work. I cannot however test other file types (mostly because i have no time to write these files)...

open comments

At the moment, if you do unload_options = ['PARQUET'] and forget to unset delim from the default, you will get an error. This is intentional, I think we don't need to hold the user's hands too much, they can examine the error message and figure out that they need to adjust the parameter of delim. However it's possible to set it default to None so the reverse happens (if someone was using a different delimiter they can run into an error and figure out that they need to change that).

Tests

I tested on our own dataset, and it works fine (I hid the output on the locopy because it contains AWS creds)

[CLAassistant]

All committers have signed the CLA.

[fdosani]

@jfyu This is awesome. Thanks so much for the contribution! Just some comments and suggestions, happy to discuss further.

[fdosani]

Any chance we can add some integration tests too? I actually don't have access to a redshift cluster atm. Are you able to run those test on your instance?

[jfyu]

@fdosani Hey thanks so much for the comments! I still haven't written a unit test yet for I think unload. Things just picked up from work so I'll keep this in draft for a bit and will hopefully come back to it soon.

RE: integration test -- currently running my "in-prod" integration test 😂 , I can do it but I can't create S3 buckets so most likely will....screenshot a integration test pass but not modify the code directly

[jfyu]

I have a question @fdosani I actually unified delim and delimiter (it was strange to me that they are called differently in unload and load), but I am not sure if we want this to break backward compatibility.

Also I think I addressed everything except tests! will look into it

[fdosani]

I have a question @fdosani I actually unified delim and delimiter (it was strange to me that they are called differently in unload and load), but I am not sure if we want this to break backward compatibility.

Also I think I addressed everything except tests! will look into it

I think that’s fine. I’m ok with aligning things and we can just a major bump and call that out

[fdosani]

@jfyu Is this ready for review? Just thought I'd check in.

[jfyu]

@fdosani yes! Sorry. I was in the middle of writing a message then got pulled away to work lol. I was going to say, it looks like it's fairly complicated for me to change the integration test or even run it because I don't think I can create s3 bucket and am fairly worried about touching other people's stuff in the same bucket. You might have to trust me on the integration 😅

I just re-ran with the existing code and they worked.

[jfyu]

(sorry doing a merge because I"m trying to use that branch and need higher versions of boto3)

[fdosani]

Sorry for the delay here. I’m on vacation at the moment, I’ll be back in next week and will do a full review and merge this in. Sorry again  Sent from Proton Mail for iOS On Tue, Jul 26, 2022 at 9:19 AM, Jennifer (Jing Fei) Yu ***@***.***> wrote: (sorry doing a merge because I"m trying to use that branch and need higher versions of boto3) —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[fdosani]

LGTM