[GitHub] [parquet-mr] ggershinsky commented on a change in pull request #808: Parquet-1396: Cryptodata Interface for Schema Activation of Parquet E…

[GitBox <gi...@apache.org>]

ggershinsky commented on a change in pull request #808:
URL: https://github.com/apache/parquet-mr/pull/808#discussion_r509200667



##########
File path: parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetWriter.java
##########
@@ -279,6 +279,11 @@ public ParquetWriter(Path file, Configuration conf, WriteSupport<T> writeSupport
     WriteSupport.WriteContext writeContext = writeSupport.init(conf);
     MessageType schema = writeContext.getSchema();
 
+    // encryptionProperties could be built from the implementation of EncryptionPropertiesFactory when it is attached.
+    if (encryptionProperties == null) {
+      encryptionProperties = ParquetOutputFormat.createEncryptionProperties(conf, new Path(file.toString()), writeContext);

Review comment:
       This assumes that `file.toString()` returns the full file path. However, the `file` is a `public abstract interface org.apache.parquet.io.OutputFile`, which doesn't have such method, so `toString()` is up to the implementation; no guarantees it will return the path. Also, `new Path(string full_path)` is not aware of the right filesystem (?) Maybe can be handled with an upcast to a known implementing class - preferably one that already has a `Path getPath()` method. 
   But of course, this won't be very general.

##########
File path: parquet-column/src/main/java/org/apache/parquet/schema/Type.java
##########
@@ -362,5 +362,4 @@ void checkContains(Type subType) {
    * @return the converted tree
    */
    abstract <T> T convert(List<GroupType> path, TypeConverter<T> converter);
-
-}
+ }

Review comment:
       probably no need in changing this file




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org