Skip to content

add support re-encryption in ColumnEncryptor#1015

Open
vectorijk wants to merge 1 commit into
apache:masterfrom
vectorijk:PARQUET-2214
Open

add support re-encryption in ColumnEncryptor#1015
vectorijk wants to merge 1 commit into
apache:masterfrom
vectorijk:PARQUET-2214

Conversation

@vectorijk

@vectorijk vectorijk commented Dec 18, 2022

Copy link
Copy Markdown
Contributor

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:

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

wgtmac
wgtmac reviewed Jan 5, 2023
View reviewed changes
Comment thread parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.stream.Collectors;

@wgtmac wgtmac Jan 5, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why are these imports removed?

Comment thread parquet-hadoop/src/main/java/org/apache/parquet/crypto/ColumnDecryptionProperties.java
return keyBytes;
}

ColumnDecryptionProperties deepClone() {

@wgtmac wgtmac Jan 5, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It seems that the current file has only added a new method, but the formatter accidentally formatted the whole file. Can you please revert that?

Comment thread parquet-hadoop/src/main/java/org/apache/parquet/crypto/FileDecryptionProperties.java
}


/** DecryptionProperties object can be used for reading one file only.

@wgtmac wgtmac Jan 5, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same for this file.

Comment thread parquet-hadoop/src/main/java/org/apache/parquet/hadoop/util/ColumnEncryptor.java
private final Short columnOrdinal;

public DecryptorRunTime(InternalFileDecryptor fileDecryptor, ColumnChunkMetaData chunk) throws IOException {
if (fileDecryptor == null) {

@wgtmac wgtmac Jan 5, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we throw if fileDecryptor is null? When will this branch be hit?

Comment thread parquet-hadoop/src/main/java/org/apache/parquet/hadoop/util/ColumnEncryptor.java
this.conf = conf;
}

public static ParquetMetadata readFooter(String inputFile, ParquetMetadataConverter.MetadataFilter filter,

@wgtmac wgtmac Jan 5, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

filter is not used here

Comment thread parquet-hadoop/src/main/java/org/apache/parquet/hadoop/util/ColumnEncryptor.java

private void processBlocks(TransParquetFileReader reader, ParquetFileWriter writer, ParquetMetadata meta,
MessageType schema, List<String> encryptPaths) throws IOException {
MessageType schema, List<String> encryptPaths, Set<ColumnPath> alreadyEncrColumnPaths,

@wgtmac wgtmac Jan 5, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
MessageType schema, List<String> encryptPaths, Set<ColumnPath> alreadyEncrColumnPaths,
MessageType schema, List<String> encryptPaths, Set<ColumnPath> encryptedPaths,

@wgtmac

wgtmac commented Jan 5, 2023

Copy link
Copy Markdown
Member

BTW, I am working on #1014 to unify several rewriters and most logic of class ColumnEncryptor will be relocated to class ParquetRewriter. Maybe we should cooperate to avoid bulk merge conflict. @vectorijk

cc @shangxinli

@vectorijk

vectorijk commented Jan 17, 2023

Copy link
Copy Markdown
Contributor Author

@wgtmac thanks for the review! I will coordinate with #1014 and address the comments

@shangxinli

shangxinli commented Jan 24, 2023

Copy link
Copy Markdown
Contributor

Yeah, @wgtmac is making the changes to consolidate the rewriters. This change can be on top of that.

@wgtmac

wgtmac commented Jan 30, 2023

Copy link
Copy Markdown
Member

@wgtmac thanks for the review! I will coordinate with #1014 and address the comments

My PR has been merged. Feel free to rebase it and let me review once ready. @vectorijk

@vectorijk
vectorijk marked this pull request as ready for review July 24, 2024 20:17
@github-actions

github-actions Bot commented Jun 27, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has had no activity for at least 2 months. If you are still working on this change or plan to move it forward, please leave a comment or push a new commit so we know to keep it open. Otherwise, this PR will be closed automatically in about one month. Thank you for your contribution to Apache Parquet!

@github-actions github-actions Bot added the stale label Jun 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wgtmac wgtmac wgtmac left review comments

Assignees

No one assigned

Labels

stale

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vectorijk @wgtmac @shangxinli