PARQUET-1229: Parquet MR encryption by ggershinsky · Pull Request #776 · apache/parquet-java

ggershinsky · 2020-04-01T07:43:37Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Parquet Jira issues and references them in the PR title. For example, "PARQUET-1234: My Parquet PR"
- https://issues.apache.org/jira/browse/PARQUET-XXX
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Tests

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

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

ggershinsky · 2020-04-01T09:21:40Z

@gszadovszky The encryption pull request is submitted, with changes in "hadoop" and "column" modules. Travis fails on "The job exceeded the maximum log length, and has been terminated". All tests up that point pass successfully (including the tests in hadoop and column modules).

gszadovszky · 2020-04-01T10:09:21Z

FYI: Created PARQUET-1832. I'll try to fix this issue ASAP.

ggershinsky · 2020-04-01T15:32:21Z

Thank you

…rsion. Depends on PR apache#776 for [PARQUET-1229] and on PR apache#12 in parquet-testing for [PARQUET-1807]. JIRA: https://issues.apache.org/jira/browse/PARQUET-1807 Add a test for writing and reading parquet in a number of encryption and decryption configurations. Add interop test that reads files from parquet-testing GitHub repository, that were written by parquet-cpp. This adds parquet-testing repo as a submodule. Run the following to populate the "submodules/parquet-testing/" folder: git submodule update --init --recursive

…rsion Depends on PR apache#776 for [PARQUET-1229] and on PR apache#12 in parquet-testing for [PARQUET-1807]. JIRA: https://issues.apache.org/jira/browse/PARQUET-1807 Add a test for writing and reading parquet in a number of encryption and decryption configurations. Add interop test that reads files from parquet-testing GitHub repository, that were written by parquet-cpp. This adds parquet-testing repo as a submodule. Run the following to populate the "submodules/parquet-testing/" folder: git submodule update --init --recursive

shangxinli · 2020-04-20T14:43:27Z

Is this ready to review? Since there is no comment yet, can you squash it to one single commit to make the review easier?

ggershinsky · 2020-04-21T07:35:48Z

preferably reviewed after the Travis fix is in. @gszadovszky @shangxinli can you apply #777 to the encryption branch.

ggershinsky · 2020-04-21T07:37:18Z

can you squash it to one single commit to make the review easier?

This can be reviewed as a single commit at
https://github.com/apache/parquet-mr/pull/776/files

ggershinsky · 2020-04-22T15:17:47Z

the PR is ready for review

shangxinli · 2020-04-26T23:22:48Z

preferably reviewed after the Travis fix is in. @gszadovszky @shangxinli can you apply #777 to the encryption branch.

It seems I still don't have permission to do so. @gszadovszky, I did 'git cherry-pick' and then git push to encryption branch', but got permission error. Do I have to create a PR for it?

parquet-hadoop/src/main/java/org/apache/parquet/crypto/AesCipher.java

shangxinli · 2020-04-27T00:57:22Z

parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java

  }

-  private void addRowGroup(ParquetMetadata parquetMetadata, List<RowGroup> rowGroups, BlockMetaData block) {
+  private void addRowGroup(ParquetMetadata parquetMetadata, List<RowGroup> rowGroups, BlockMetaData block,


Should we keep original addRowGroup() intact and just add a new one with InternalFileEncryptor to isolate the change's impact? There is not much duplicate code if doing so and we can refactor the existing code with helper functions. In the majority use cases, they are non-encryption cases.

encryption is isolated there, with if (null != fileEncryptor) and if (encryptMetaData) - similar to the if (columnIndexRef != null) and if (offsetIndexRef != null) in the same function.

Not all the changes are isolated. Generally, adding 'if/else' will add diverge the code and add the complexity. One other thing is regression thinking. If fileEncryptor is null, which would be most of the case, then it just executes the existing method without change. It would be less error prone.

Many functions can be run either with or without encryption. Duplicating them will result in hundreds or thousands of duplicate code lines. This will make code maintenance (changes/fixes) a headache. Instead, we isolate encryption with if switches, without duplicating the existing code.

The same goes for other recent new features (column indexes and bloom filters) - they are isolated with an if switch, instead of code duplication. See if (columnIndexRef != null) and if (offsetIndexRef != null) in this addRowGroup function.

It is not a blocking comment and I am fine with it. But generally speaking, adding too much nested if/else diverges the code path and causes the complexity for reading. One way to avoid duplicating is to wrap them up in helper functions. I understand column indexes and bloom filters already did that but if we keep adding features like this, the code will become less and less readable.

shangxinli · 2020-04-27T01:04:48Z

parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java

-    ParquetMetadata parquetMetadata = fromParquetMetadata(fileMetaData);
+
+    if (!encryptedFooter && null != fileDecryptor) {
+      if (!fileMetaData.isSetEncryption_algorithm()) { // Plaintext file


Move comments up

is there a requirement in the code formatting rules in this community to keep comments in separate lines?

No. I see most of them on up line but a few on the same line. It is not a must.

Actually, based on parquet-mr README:

Generally speaking, stick to the Sun Java Code Conventions

Based on the related section both should be fine.

gszadovszky · 2020-04-27T08:31:27Z

preferably reviewed after the Travis fix is in. @gszadovszky @shangxinli can you apply #777 to the encryption branch.

It seems I still don't have permission to do so. @gszadovszky, I did 'git cherry-pick' and then git push to encryption branch', but got permission error. Do I have to create a PR for it?

@shangxinli, I've done a rebase on the branch encryption. Maybe, that's why you were unable to push a commit because after the rebase it would not be a fast forward commit (would need a force-push).
(Because of the rebase the Travis fix is already in.)
If you have any problems with manual push to the repo, let's bring it offline. I'm happy to help.

shangxinli · 2020-05-10T03:17:47Z

@ggershinsky Do you have time to review the code?

…rsion Depends on PR apache#776 for [PARQUET-1229] and on PR apache#12 in parquet-testing for [PARQUET-1807]. JIRA: https://issues.apache.org/jira/browse/PARQUET-1807 Add a test for writing and reading parquet in a number of encryption and decryption configurations. Add interop test that reads files from parquet-testing GitHub repository, that were written by parquet-cpp. This adds parquet-testing repo as a submodule. Run the following to populate the "submodules/parquet-testing/" folder: git submodule update --init --recursive

ggershinsky · 2020-05-12T07:07:01Z

@ggershinsky Do you have time to review the code?

You probably meant @gszadovszky

gszadovszky

I have a couple of comments but did not review all yet.

parquet-column/src/main/java/org/apache/parquet/internal/column/columnindex/OffsetIndex.java

parquet-hadoop/src/main/java/org/apache/parquet/crypto/AesCipher.java

parquet-format-structures/src/main/java/org/apache/parquet/format/BlockCipher.java

parquet-hadoop/src/main/java/org/apache/parquet/crypto/AesGcmDecryptor.java

gszadovszky · 2020-05-19T14:05:11Z

parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java

-    ParquetMetadata parquetMetadata = fromParquetMetadata(fileMetaData);
+
+    if (!encryptedFooter && null != fileDecryptor) {
+      if (!fileMetaData.isSetEncryption_algorithm()) { // Plaintext file


Actually, based on parquet-mr README:

Generally speaking, stick to the Sun Java Code Conventions

Based on the related section both should be fine.

parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java

gszadovszky

Second round. Still not finished. :(

parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java

gszadovszky

Some other comments. This time, I've reached the end. :)

I know, this is not the whole feature yet but would like to ensure that at the end we will have proper test coverity. So, the question is do we have or planning to have tests to verify the working of semi-encrypted files (some columns and the footer are plaintext) with previous readers.

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileWriter.java

parquet-hadoop/src/test/java/org/apache/parquet/hadoop/TestBloomEncryption.java

parquet-hadoop/src/test/java/org/apache/parquet/hadoop/TestEncryption.java

gszadovszky

Now, we are in sync. I will try to keep it this way. ;)

parquet-hadoop/src/main/java/org/apache/parquet/crypto/AesCipher.java

ggershinsky · 2020-05-21T14:52:21Z

I know, this is not the whole feature yet but would like to ensure that at the end we will have proper test coverity. So, the question is do we have or planning to have tests to verify the working of semi-encrypted files (some columns and the footer are plaintext) with previous readers.

We test this manually. We also have an automatic test that runs the current reader code on files that were previously created with other writers (such as parquet-cpp with encryption; these files are kept in parquet-testing repo and fetched from there). But I don't know how to automate running of previous readers on the files written by the current code - ?

…FileWriter.java Co-authored-by: Gabor Szadovszky <gabor@apache.org>

…a/ColumnChunkMetaData.java Co-authored-by: Gabor Szadovszky <gabor@apache.org>

ggershinsky · 2020-05-21T15:10:05Z

Now, we are in sync. I will try to keep it this way. ;)

Appreciated!
Addressing today's comments will keep me busy for a while :), I'll get back next week.

gszadovszky · 2020-05-22T10:21:46Z

I know, this is not the whole feature yet but would like to ensure that at the end we will have proper test coverity. So, the question is do we have or planning to have tests to verify the working of semi-encrypted files (some columns and the footer are plaintext) with previous readers.

We test this manually. We also have an automatic test that runs the current reader code on files that were previously created with other writers (such as parquet-cpp with encryption; these files are kept in parquet-testing repo and fetched from there). But I don't know how to automate running of previous readers on the files written by the current code - ?

Good question. We should have the tooling to provide such tests but not sure if we already have it. I am fine with testing it manually for now just document it in details (e.g. adding to the git comment of the test PR).

ggershinsky · 2020-05-27T12:54:00Z

The current review round is addressed; the checks pass.

…rsion Depends on PR apache#776 for [PARQUET-1229] and on PR apache#12 in parquet-testing for [PARQUET-1807]. JIRA: https://issues.apache.org/jira/browse/PARQUET-1807 Add a test for writing and reading parquet in a number of encryption and decryption configurations. Add interop test that reads files from parquet-testing GitHub repository, that were written by parquet-cpp. This adds parquet-testing repo as a submodule. Run the following to populate the "submodules/parquet-testing/" folder: git submodule update --init --recursive

* PARQUET-1228: Format Structures encryption (#613) * PARQUET-1286: Crypto package (#614) * PARQUET-1818: Fix bloom/encryption collision in format-structures (#771) * PARQUET-1817: Crypto Properties Factory (#769) * PARQUET-1229: Parquet MR encryption (#776) * PARQUET-1807: Encryption: Interop and Function test suite for Java version (#782) * PARQUET-1373: Encryption key tools (#615) Co-authored-by: shangxinli <31421745+shangxinli@users.noreply.github.com> Co-authored-by: Maya Anderson <mayaa@il.ibm.com>

ggershinsky force-pushed the p1229-parquet-mr-encryption branch from 247a2a5 to 45b917f Compare April 1, 2020 15:31

andersonm-1 mentioned this pull request Apr 5, 2020

PARQUET-1807: Encryption: Interop and Function test suite for Java version #780

Closed

andersonm-ibm mentioned this pull request Apr 6, 2020

PARQUET-1807: Encryption: Interop and Function test suite for Java version #782

Merged

ggershinsky mentioned this pull request Apr 6, 2020

PARQUET-1404: [C++] Adding Page level Indexes apache/arrow#6807

Closed

gszadovszky force-pushed the encryption branch from bd876bd to 42827b1 Compare April 22, 2020 09:10

ggershinsky added 9 commits April 22, 2020 13:17

mr encryption - initial push

d2b9370

PFR fix

96d00dc

PFR fix

4eff3ba

format fix

6b2bcff

javadoc annotations

af87bcf

code format clean up

c8aa709

use crypto exception instead of IOexception

297ed1b

remove hidden column exception

40484c6

fix prettyJSON exception

c6b0cde

ggershinsky force-pushed the p1229-parquet-mr-encryption branch from fe7ab01 to c6b0cde Compare April 22, 2020 10:33

ggershinsky added 2 commits April 22, 2020 15:39

bloom encryption test fix

cacea5e

indentation fix

0245db2

remove travis-before_install-encryption (format master fetch)

fdb1d59

shangxinli reviewed Apr 26, 2020

View reviewed changes

parquet-hadoop/src/main/java/org/apache/parquet/crypto/AesCipher.java Show resolved Hide resolved

shangxinli reviewed Apr 27, 2020

View reviewed changes

gszadovszky requested changes May 19, 2020

View reviewed changes

address initial comments

c9761c3

gszadovszky requested changes May 20, 2020

View reviewed changes

gszadovszky requested changes May 21, 2020

View reviewed changes

parquet-hadoop/src/main/java/org/apache/parquet/crypto/AesCipher.java Show resolved Hide resolved

parquet-hadoop/src/main/java/org/apache/parquet/crypto/AesCipher.java Outdated Show resolved Hide resolved

ggershinsky and others added 2 commits May 21, 2020 17:53

Update parquet-hadoop/src/main/java/org/apache/parquet/hadoop/Parquet…

7a8cc10

…FileWriter.java Co-authored-by: Gabor Szadovszky <gabor@apache.org>

Update parquet-hadoop/src/main/java/org/apache/parquet/hadoop/metadat…

dded1bb

…a/ColumnChunkMetaData.java Co-authored-by: Gabor Szadovszky <gabor@apache.org>

address review comments

4a325b7

field scope, rm comment

f3ed0b9

gszadovszky approved these changes May 29, 2020

View reviewed changes

gszadovszky merged commit 06b5372 into apache:encryption May 29, 2020

shangxinli pushed a commit to shangxinli/parquet-mr that referenced this pull request Jul 15, 2020

PARQUET-1229: Parquet MR encryption (apache#776)

237cce2

gszadovszky pushed a commit that referenced this pull request Jul 28, 2020

PARQUET-1229: Parquet MR encryption (#776)

b60f4a4

Conversation

ggershinsky commented Apr 1, 2020

Jira

Tests

Commits

Documentation

Uh oh!

ggershinsky commented Apr 1, 2020

Uh oh!

gszadovszky commented Apr 1, 2020

Uh oh!

ggershinsky commented Apr 1, 2020

Uh oh!

shangxinli commented Apr 20, 2020

Uh oh!

ggershinsky commented Apr 21, 2020

Uh oh!

ggershinsky commented Apr 21, 2020

Uh oh!

ggershinsky commented Apr 22, 2020

Uh oh!

shangxinli commented Apr 26, 2020

Uh oh!

Uh oh!

shangxinli Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gszadovszky commented Apr 27, 2020

Uh oh!

shangxinli commented May 10, 2020

Uh oh!

ggershinsky commented May 12, 2020

Uh oh!

gszadovszky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gszadovszky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gszadovszky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shangxinli Apr 27, 2020 •

edited

Loading