Skip to content

Commit fa06d92

Browse files
committed
fix: tighten RSS writer visibility and JNI array limits
1 parent 0bc86a2 commit fa06d92

4 files changed

Lines changed: 33 additions & 18 deletions

File tree

‎native/jni-bridge/src/shuffle_partition_pusher.rs‎

Lines changed: 28 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,9 @@ use jni::objects::{Global, JMethodID, JObject, JValue};
2121
use jni::signature::{Primitive, ReturnType};
2222
use jni::Env;
2323

24+
/// OpenJDK's conservative soft array limit; actual JVM limits can vary.
25+
const MAX_JVM_ARRAY_LENGTH: i32 = i32::MAX - 8;
26+
2427
/// Receives a complete encoded shuffle block for one output partition.
2528
///
2629
/// Implementations must remain safe when invoked from native execution
@@ -72,11 +75,13 @@ impl JavaShufflePartitionPusher {
7275
)));
7376
}
7477

75-
i32::try_from(payload_length).map_err(|_| {
76-
DataFusionError::Execution(format!(
77-
"Remote shuffle payload size {payload_length} exceeds the JVM array limit"
78-
))
79-
})
78+
match i32::try_from(payload_length) {
79+
Ok(length) if length <= MAX_JVM_ARRAY_LENGTH => Ok(length),
80+
_ => Err(DataFusionError::Execution(format!(
81+
"Remote shuffle payload size {payload_length} exceeds the JVM array limit of \
82+
{MAX_JVM_ARRAY_LENGTH} bytes"
83+
))),
84+
}
8085
}
8186
}
8287

@@ -117,17 +122,18 @@ impl ShufflePartitionPusher for JavaShufflePartitionPusher {
117122

118123
#[cfg(test)]
119124
mod tests {
120-
use super::JavaShufflePartitionPusher;
125+
use super::{JavaShufflePartitionPusher, MAX_JVM_ARRAY_LENGTH};
121126

122127
#[test]
123-
fn accepts_payload_lengths_representable_by_jvm_arrays() {
128+
fn accepts_payload_lengths_up_to_jvm_soft_array_limit() {
124129
assert_eq!(
125130
JavaShufflePartitionPusher::checked_payload_length(0, 0).unwrap(),
126131
0
127132
);
128133
assert_eq!(
129-
JavaShufflePartitionPusher::checked_payload_length(7, i32::MAX as usize).unwrap(),
130-
i32::MAX
134+
JavaShufflePartitionPusher::checked_payload_length(7, MAX_JVM_ARRAY_LENGTH as usize,)
135+
.unwrap(),
136+
MAX_JVM_ARRAY_LENGTH
131137
);
132138
}
133139

@@ -138,10 +144,18 @@ mod tests {
138144
}
139145

140146
#[test]
141-
fn rejects_payload_lengths_larger_than_a_jvm_array() {
142-
let oversized_length = i32::MAX as usize + 1;
143-
let error =
144-
JavaShufflePartitionPusher::checked_payload_length(0, oversized_length).unwrap_err();
145-
assert!(error.to_string().contains("exceeds the JVM array limit"));
147+
fn rejects_payload_lengths_larger_than_jvm_soft_array_limit() {
148+
for oversized_length in [
149+
MAX_JVM_ARRAY_LENGTH as usize + 1,
150+
i32::MAX as usize,
151+
i32::MAX as usize + 1,
152+
usize::MAX,
153+
] {
154+
let error = JavaShufflePartitionPusher::checked_payload_length(0, oversized_length)
155+
.unwrap_err();
156+
let message = error.to_string();
157+
assert!(message.contains("exceeds the JVM array limit"));
158+
assert!(message.contains(&MAX_JVM_ARRAY_LENGTH.to_string()));
159+
}
146160
}
147161
}

‎native/shuffle/src/lib.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,4 +29,4 @@ pub use comet_partitioning::CometPartitioning;
2929
pub use ipc::read_ipc_compressed;
3030
pub use schema_align::SchemaAlignExec;
3131
pub use shuffle_writer::ShuffleWriterExec;
32-
pub use writers::{CompressionCodec, RssPartitionWriter, ShuffleBlockWriter};
32+
pub use writers::{CompressionCodec, ShuffleBlockWriter};

‎native/shuffle/src/writers/mod.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,13 @@ mod buf_batch_writer;
1919
mod checksum;
2020
mod local;
2121
mod partition_writer;
22+
// Remote shuffle execution will use this writer in a subsequent change.
23+
#[allow(dead_code)]
2224
mod rss;
2325
mod shuffle_block_writer;
2426

2527
pub(crate) use buf_batch_writer::BufBatchWriter;
2628
pub(crate) use checksum::Checksum;
2729
pub(crate) use local::local_partition_writer::LocalPartitionWriter;
2830
pub(crate) use partition_writer::PartitionWriter;
29-
pub use rss::rss_partition_writer::RssPartitionWriter;
3031
pub use shuffle_block_writer::{CompressionCodec, ShuffleBlockWriter};

‎native/shuffle/src/writers/rss/rss_partition_writer.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ use std::sync::Arc;
3636
/// Partition data may arrive in any order until its partition is finalized.
3737
/// Finalization follows the same ascending-partition contract as the existing
3838
/// local shuffle writer.
39-
pub struct RssPartitionWriter {
39+
pub(crate) struct RssPartitionWriter {
4040
block_writer: ShuffleBlockWriter,
4141
pusher: Arc<dyn ShufflePartitionPusher>,
4242
num_partitions: usize,
@@ -53,7 +53,7 @@ impl RssPartitionWriter {
5353
/// `num_partitions` must be nonzero and each partition identifier must fit
5454
/// in a JVM `int`. `max_frame_size` is the maximum complete encoded shuffle
5555
/// block passed to a callback and must also be nonzero.
56-
pub fn try_new(
56+
pub(crate) fn try_new(
5757
block_writer: ShuffleBlockWriter,
5858
pusher: Arc<dyn ShufflePartitionPusher>,
5959
num_partitions: usize,

0 commit comments

Comments
 (0)