mbutrovich commented on code in PR #1440:
URL: https://github.com/apache/datafusion-comet/pull/1440#discussion_r1972471189
##########
native/core/src/execution/shuffle/shuffle_writer.rs:
##########
@@ -1076,6 +1053,146 @@ mod test {
shuffle_write_test(10000, 100, 200, Some(10 * 1024 * 1024));
}
+ #[test]
+ fn partition_buffer_memory() {
+ let batch = create_batch(900);
+ let runtime_env = create_runtime(128 * 1024);
+ let mut buffer = PartitionBuffer::try_new(
+ batch.schema(),
+ 1024,
+ 0,
+ &runtime_env,
+ CompressionCodec::Lz4Frame,
+ true,
+ )
+ .unwrap();
+ let metrics_set = ExecutionPlanMetricsSet::new();
+ let metrics = ShuffleRepartitionerMetrics::new(&metrics_set, 0);
+ let indices: Vec<usize> = (0..batch.num_rows()).collect();
+
+ assert_eq!(0, buffer.reservation.size());
+ assert!(buffer.spill_file.is_none());
+
+ // append first batch - should fit in memory
+ let status = buffer.append_rows(batch.columns(), &indices, 0,
&metrics);
+ assert_eq!(
+ format!("{status:?}"),
+ format!("{:?}", AppendRowStatus::MemDiff(Ok(106496)))
+ );
+ assert_eq!(900, buffer.num_active_rows);
+ assert_eq!(106496, buffer.reservation.size());
+ assert_eq!(0, buffer.frozen.len());
+ assert!(buffer.spill_file.is_none());
+
+ // append second batch - should trigger flush to frozen bytes
+ let status = buffer.append_rows(batch.columns(), &indices, 0,
&metrics);
+ assert_eq!(
+ format!("{status:?}"),
+ format!("{:?}", AppendRowStatus::MemDiff(Ok(126316)))
+ );
+ assert_eq!(0, buffer.num_active_rows);
+ assert_eq!(9914, buffer.frozen.len());
+ // note that the reservation does not include the frozen bytes
+ assert_eq!(106496, buffer.reservation.size());
+ assert!(buffer.spill_file.is_none());
+
+ // spill
+ buffer.spill(&runtime_env, &metrics).unwrap();
+ assert_eq!(0, buffer.num_active_rows);
+ assert_eq!(0, buffer.frozen.len());
+ assert_eq!(0, buffer.reservation.size());
+ assert!(buffer.spill_file.is_some());
+
+ // append after spill
+ let status = buffer.append_rows(batch.columns(), &indices, 0,
&metrics);
+ assert_eq!(
+ format!("{status:?}"),
+ format!("{:?}", AppendRowStatus::MemDiff(Ok(0)))
+ );
+ assert_eq!(900, buffer.num_active_rows);
Review Comment:
Doesn't seem like we could assert that since copying the spill file back to
the output is not in `PartitionBuffer`, which is all we're exercising in this
test. We might be able to in the test below, though.
--
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
