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.
##########
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:
Similar to my comment above, assuming the next batch triggered the spill
file to be written to output, can we check that the spill file length is now 0?
--
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
