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

PARQUET-1599: Fix to-avro to respect the overwrite option #650

Merged
merged 6 commits into from Apr 11, 2020

Conversation

sekikn
Copy link
Contributor

@sekikn sekikn commented Jun 16, 2019

Make sure you have checked all steps below.

Jira

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

    • testToAvroCommandOverwriteExistentFile
    • testToAvroCommandOverwriteExistentFileWithoutOverwriteOption

Commits

  • My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain Javadoc that explain what it does

@@ -96,9 +97,8 @@ public int run() throws IOException {

Path outPath = qualifiedPath(outputPath);
FileSystem outFS = outPath.getFileSystem(getConf());
if (overwrite && outFS.exists(outPath)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SchemaCommand has the same problem.

Instead of throwing an exception here if the file already exists, how about adding a new method to BaseCommand for no overwrite case, and using that when creating the file in this command (and in SchemaCommand)?

Something like this: a new createWithNoOverwrite, which calls the private create with an extra overwrite=false flag, and it will use fs.create with this parameter value. How does this sound? We can also remove this delete file section in this case, and just call createWithNoOverwrite when overwrite is true, and call create otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review @nandorKollar, that sounds good to me.
I'll update the PR a bit later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nandorKollar Sorry for my long silence, just addressed your comments :)

@sekikn
Copy link
Contributor Author

sekikn commented Sep 23, 2019

Rebased on master and resolve conflicts.

@sekikn
Copy link
Contributor Author

sekikn commented Nov 6, 2019

Rebased on master and resolved conflicts.

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one small comment

for (Record record : reader) {
writer.append(record);
count += 1;
try (OutputStream os = overwrite ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can combine the two try's here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry that I missed your comment @Fokko! Just updated the PR.

@Fokko
Copy link
Contributor

Fokko commented Feb 2, 2020

I've retriggered the failing build, it looked unrelated.

@Fokko Fokko merged commit 49e8627 into apache:master Apr 11, 2020
@Fokko
Copy link
Contributor

Fokko commented Apr 11, 2020

Thanks @sekikn !

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