Skip to content

Conversation

@sekikn
Copy link
Contributor

@sekikn sekikn commented Jun 17, 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:

    • testToAvroCommandWithGzipCompression
    • testToAvroCommandWithSnappyCompression
    • testToAvroCommandWithZstdCompression
    • testToAvroCommandWithInvalidCompression

In addition, I confirmed an output file generated by to-avro was properly compressed by zstd as follows:

$ java -cp 'target/dependency/*:target/*' org.apache.parquet.cli.Main to-avro sample.parquet -o sample.avro --compression-codec zstd
$ java -jar ~/avro-tools-1.9.0.jar getmeta sample.avro 

(snip)

avro.codec	zstandard

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

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.

One minor comment, apart from that: LGTM

<opencsv.version>2.3</opencsv.version>
<jcommander.version>1.35</jcommander.version>
<commons-codec.version>1.10</commons-codec.version>
<zstd-jni.version>1.3.8-6</zstd-jni.version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe set this to 1.4.0-1 right away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment @Fokko! Avro 1.9.0 seems to depend on zstd-jni 1.3.8-6, so I reconciled them. Should I update it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, maybe leave it as is for now to avoid conflicts

Copy link
Contributor

Choose a reason for hiding this comment

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

@sekikn
Copy link
Contributor Author

sekikn commented Sep 19, 2019

Rebased on master and resolved conflicts.

@Fokko Fokko merged commit e9d8716 into apache:master Sep 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants