alamb commented on code in PR #16351:
URL: https://github.com/apache/datafusion/pull/16351#discussion_r2164809665


##########
datafusion/core/src/dataframe/parquet.rs:
##########
@@ -246,4 +246,72 @@ mod tests {
 
         Ok(())
     }
+
+    #[tokio::test]
+    async fn roundtrip_parquet_with_encryption() -> Result<()> {

Review Comment:
   I wonder why this isn't in core/tests as well 🤔   (I see you are just 
following the existing pattern, I just noticed this while reviewing this PR) 



##########
datafusion-examples/examples/parquet_encrypted.rs:
##########
@@ -0,0 +1,118 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use datafusion::common::DataFusionError;

Review Comment:
   I ran this example and it works great
   
   ```
   
   
   
===============================================================================
   Encrypted Parquet DataFrame:
   Schema:
   
+------------+--------------------+----------+--------------------+--------------------+--------------------+--------------------+--------------------+-------------------+-----------------+------------+---------------------+
   | describe   | id                 | bool_col | tinyint_col        | 
smallint_col       | int_col            | bigint_col         | float_col        
  | double_col        | date_string_col | string_col | timestamp_col       |
   
+------------+--------------------+----------+--------------------+--------------------+--------------------+--------------------+--------------------+-------------------+-----------------+------------+---------------------+
   | count      | 8.0                | 8        | 8.0                | 8.0      
          | 8.0                | 8.0                | 8.0                | 8.0  
             | 8               | 8          | 8                   |
   | null_count | 0.0                | 0        | 0.0                | 0.0      
          | 0.0                | 0.0                | 0.0                | 0.0  
             | 0               | 0          | 0                   |
   | mean       | 3.5                | null     | 0.5                | 0.5      
          | 0.5                | 5.0                | 0.550000011920929  | 5.05 
             | null            | null       | null                |
   | std        | 2.4494897427831783 | null     | 0.5345224838248488 | 
0.5345224838248488 | 0.5345224838248488 | 5.3452248382484875 | 
0.5879747449513427 | 5.398677086630973 | null            | null       | null    
            |
   | min        | 0.0                | null     | 0.0                | 0.0      
          | 0.0                | 0.0                | 0.0                | 0.0  
             | 01/01/09        | 0          | 2009-01-01T00:00:00 |
   | max        | 7.0                | null     | 1.0                | 1.0      
          | 1.0                | 10.0               | 1.100000023841858  | 10.1 
             | 04/01/09        | 1          | 2009-04-01T00:01:00 |
   | median     | 3.0                | null     | 0.0                | 0.0      
          | 0.0                | 5.0                | 0.550000011920929  | 5.05 
             | null            | null       | null                |
   
+------------+--------------------+----------+--------------------+--------------------+--------------------+--------------------+--------------------+-------------------+-----------------+------------+---------------------+
   
   Selected rows and columns:
   +----+----------+---------------------+
   | id | bool_col | timestamp_col       |
   +----+----------+---------------------+
   | 6  | true     | 2009-04-01T00:00:00 |
   | 7  | false    | 2009-04-01T00:01:00 |
   +----+----------+---------------------+
   ```



##########
docs/source/user-guide/configs.md:
##########
@@ -81,6 +81,8 @@ Environment variables are read during `SessionConfig` 
initialisation so they mus
 | datafusion.execution.parquet.allow_single_file_parallelism              | 
true                      | (writing) Controls whether DataFusion will attempt 
to speed up writing parquet files by serializing them in parallel. Each column 
in each row group in each output file are serialized in parallel leveraging a 
maximum possible core count of n_files*n_row_groups*n_columns.                  
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                        |
 | datafusion.execution.parquet.maximum_parallel_row_group_writers         | 1  
                       | (writing) By default parallel parquet writer is tuned 
for minimum memory usage in a streaming execution plan. You may see a 
performance benefit when writing large parquet files by increasing 
maximum_parallel_row_group_writers and 
maximum_buffered_record_batches_per_stream if your system has idle cores and 
can tolerate additional memory usage. Boosting these values is likely 
worthwhile when writing out already in-memory data, such as from a cached data 
frame.                                                                          
                                                                                
                                                                                
                                                                                
                                                                                
                                |
 | datafusion.execution.parquet.maximum_buffered_record_batches_per_stream | 2  
                       | (writing) By default parallel parquet writer is tuned 
for minimum memory usage in a streaming execution plan. You may see a 
performance benefit when writing large parquet files by increasing 
maximum_parallel_row_group_writers and 
maximum_buffered_record_batches_per_stream if your system has idle cores and 
can tolerate additional memory usage. Boosting these values is likely 
worthwhile when writing out already in-memory data, such as from a cached data 
frame.                                                                          
                                                                                
                                                                                
                                                                                
                                                                                
                                |
+| datafusion.execution.parquet.file_decryption_properties                 | 
NULL                      | Optional file decryption properties                 
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                    |

Review Comment:
   It would be nice to document the format of these properties  -- for example 
mention they are hex encoded keys of whatever type, or perhaps add a link to 
the appropriate documentation 
   
   



##########
docs/source/user-guide/configs.md:
##########
@@ -81,6 +81,8 @@ Environment variables are read during `SessionConfig` 
initialisation so they mus
 | datafusion.execution.parquet.allow_single_file_parallelism              | 
true                      | (writing) Controls whether DataFusion will attempt 
to speed up writing parquet files by serializing them in parallel. Each column 
in each row group in each output file are serialized in parallel leveraging a 
maximum possible core count of n_files*n_row_groups*n_columns.                  
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                        |
 | datafusion.execution.parquet.maximum_parallel_row_group_writers         | 1  
                       | (writing) By default parallel parquet writer is tuned 
for minimum memory usage in a streaming execution plan. You may see a 
performance benefit when writing large parquet files by increasing 
maximum_parallel_row_group_writers and 
maximum_buffered_record_batches_per_stream if your system has idle cores and 
can tolerate additional memory usage. Boosting these values is likely 
worthwhile when writing out already in-memory data, such as from a cached data 
frame.                                                                          
                                                                                
                                                                                
                                                                                
                                                                                
                                |
 | datafusion.execution.parquet.maximum_buffered_record_batches_per_stream | 2  
                       | (writing) By default parallel parquet writer is tuned 
for minimum memory usage in a streaming execution plan. You may see a 
performance benefit when writing large parquet files by increasing 
maximum_parallel_row_group_writers and 
maximum_buffered_record_batches_per_stream if your system has idle cores and 
can tolerate additional memory usage. Boosting these values is likely 
worthwhile when writing out already in-memory data, such as from a cached data 
frame.                                                                          
                                                                                
                                                                                
                                                                                
                                                                                
                                |
+| datafusion.execution.parquet.file_decryption_properties                 | 
NULL                      | Optional file decryption properties                 
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                                                                
                                    |

Review Comment:
   For example, how would you configure parquet encryption from the 
`datafusion-cli` (
   
   ```sql
   set datafusion.execution.parquet.file_decryption_properties = ???
   ```
   
   To be clear, I don't think we need to have a super easy to configure system 
at first, but I do think it is important to document and point people in the 
right direction if they get here



-- 
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.

To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Reply via email to