Re: [PR] fix: gc string view arrays in RepartitionExec [datafusion]

["kosiew (via GitHub)" <gi...@apache.org>]

kosiew commented on code in PR #20500:
URL: https://github.com/apache/datafusion/pull/20500#discussion_r2944875606


##########
datafusion/physical-plan/src/repartition/mod.rs:
##########
@@ -2602,6 +2642,31 @@ mod tests {
 
         Ok(())
     }
+
+    #[tokio::test]
+    async fn hash_repartition_string_view_compaction() -> Result<()> {

Review Comment:
   This test does not actually exercise the regression it is meant to cover.
   It only checks that `repartition` returns all rows. That would also pass before the `gc()` change.
   
   As a result, we still do not have a test that would catch the over-counting bug if this logic regresses.
   
   Please add an assertion that observes the compaction or accounting behavior directly. For example:
   
   * Check that the total `get_array_memory_size()` across the repartitioned outputs stays close to the original batch, instead of scaling with the number of output partitions.
   * Test spill behavior under a tight memory limit (e.g., spilled bytes).
   * Verify `StringViewArray` buffer ownership after repartition, so outputs no longer all retain the original shared payload buffer.
   



##########
datafusion/physical-plan/src/repartition/mod.rs:
##########
@@ -611,11 +611,24 @@ impl BatchPartitioner {
                             let columns =
                                 take_arrays(batch.columns(), &indices_array, None)?;
 
+                            let new_columns = columns
+                                .into_iter()
+                                .map(|col| {
+                                    if let Some(sv) =
+                                        col.as_any().downcast_ref::<StringViewArray>()
+                                    {
+                                        Arc::new(sv.gc())

Review Comment:
   The new `StringViewArray::gc()` pass in repartition is very similar to the existing `organize_stringview_arrays` logic in `datafusion/physical-plan/src/sorts/sort.rs`. 
   
   A small shared helper would keep the workaround in one place and reduce the chance that sort/repartition diverge when Arrow view handling changes again.



-- 
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-unsubscribe@datafusion.apache.org

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


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