sunchao commented on code in PR #5780:
URL: https://github.com/apache/datafusion-comet/pull/5780#discussion_r3962068263


##########
docs/source/user-guide/latest/iceberg-writes.md:
##########
@@ -157,7 +157,7 @@ A write is eligible only when ALL of the following hold:
 | `write.parquet.bloom-filter-enabled.column.<col>`                            
                                                               | unset or 
`false`                                                                         
                                                                                
                                                                                
                                                                                
                                                                                
                                                       |
 | `write.metadata.metrics.*`                                                   
                                                               | any value 
(manifest metrics are re-derived on the JVM with Iceberg's own logic)           
                                                                                
                                                                                
                                                                                
                                                                                
                                                      |
 | `write.spark.fanout.enabled`                                                 
                                                               | any value (the 
native writer implements both clustered and fanout modes)                       
                                                                                
                                                                                
                                                                                
                                                                                
                                                 |
-| `write.target-file-size-bytes`                                               
                                                               | any value 
(file rolling cadence differs; see accepted divergences)                        
                                                                                
                                                                                
                                                                                
                                                                                
                                                      |
+| `write.target-file-size-bytes`                                               
                                                               | any value (the 
roll point can differ by less than 1000 rows; see accepted divergences)         
                                                                                
                                                                                
                                                                                
                                                                                
                                                 |

Review Comment:
   ### Correctness
   
   [P2] Remove the claimed 1,000-row bound on JVM/native roll-point drift
   
   The new pacer puts size checks on a 1,000-row grid. It does not bound the 
difference between the two writers' roll points. The locked Rust writer uses 
[flushed bytes plus 
`in_progress_size()`](https://github.com/apache/iceberg-rust/blob/665c64e48e8d33797ecb1a421f327edd9b024879/crates/iceberg/src/writer/file_writer/parquet_writer.rs#L708),
 while iceberg-java uses [file position plus buffered 
size](https://github.com/apache/iceberg/blob/6976e020b894f6a6777704df2b8c4458cb291ae9/parquet/src/main/java/org/apache/iceberg/parquet/ParquetWriter.java#L163).
 They also retain different `>` and `>=` threshold comparisons. These are 
independent estimates, and nothing here limits their threshold crossings to 
adjacent grid points. This table promises **less than 1,000 rows** for any 
target, while the paragraph below also describes a full 1,000-row difference. 
Please document the shared sampling grid without a numeric bound on 
cross-writer row counts, and remove the matching one-step-only claim f
 rom the PR rationale. Otherwise users may rely on file-layout parity that this 
change does not establish.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to