Copilot commented on code in PR #680:
URL: https://github.com/apache/sedona-db/pull/680#discussion_r2882008398
##########
rust/sedona/src/context.rs:
##########
@@ -115,6 +116,17 @@ impl SedonaContext {
opts.crs_provider =
CrsProviderOption::new(Arc::new(sedona_proj::provider::ProjCrsProvider::default()));
+ // Set the spilled batch in-memory size threshold to 5% of the
per-partition memory limit,
+ // with a minimum of 10MB. Batches larger than this threshold will be
broken into smaller batches
+ // before writing to spill files. This is to avoid overshooting memory
limit when reading super
+ // large spilled batches.
+ if let MemoryLimit::Finite(memory_limit) =
runtime_env.memory_pool.memory_limit() {
+ let per_partition_memory_limit =
memory_limit.div_ceil(target_partitions);
+ opts.spatial_join.spilled_batch_in_memory_size_threshold =
per_partition_memory_limit
+ .div_ceil(20)
+ .max(10 * 1024 * 1024);
+ }
Review Comment:
The new auto-configuration logic for
`spilled_batch_in_memory_size_threshold` isn’t covered by tests. Since
`SedonaContextBuilder::build()` and
`SedonaContext::new_local_interactive_with_runtime_env()` are core entry
points, please add a test that builds a `RuntimeEnv` with a finite memory pool
limit and asserts the computed threshold matches the intended formula (5% of
per-partition limit with a 10MB floor).
##########
rust/sedona/src/context.rs:
##########
@@ -115,6 +116,17 @@ impl SedonaContext {
opts.crs_provider =
CrsProviderOption::new(Arc::new(sedona_proj::provider::ProjCrsProvider::default()));
+ // Set the spilled batch in-memory size threshold to 5% of the
per-partition memory limit,
+ // with a minimum of 10MB. Batches larger than this threshold will be
broken into smaller batches
+ // before writing to spill files. This is to avoid overshooting memory
limit when reading super
+ // large spilled batches.
+ if let MemoryLimit::Finite(memory_limit) =
runtime_env.memory_pool.memory_limit() {
+ let per_partition_memory_limit =
memory_limit.div_ceil(target_partitions);
+ opts.spatial_join.spilled_batch_in_memory_size_threshold =
per_partition_memory_limit
+ .div_ceil(20)
+ .max(10 * 1024 * 1024);
Review Comment:
This calculation relies on hard-coded constants (`20` for 5% and `10 * 1024
* 1024` for 10MB). Consider extracting these into named constants (and/or
computing from an explicit percentage) so the intent is easier to audit and
change without re-deriving the math from the literals.
```suggestion
const SPILLED_BATCH_THRESHOLD_PERCENT_DIVISOR: usize = 20; // 5% ==
1 / 20
const MIN_SPILLED_BATCH_IN_MEMORY_THRESHOLD_BYTES: usize = 10 * 1024
* 1024; // 10MB
if let MemoryLimit::Finite(memory_limit) =
runtime_env.memory_pool.memory_limit() {
let per_partition_memory_limit =
memory_limit.div_ceil(target_partitions);
opts.spatial_join.spilled_batch_in_memory_size_threshold =
per_partition_memory_limit
.div_ceil(SPILLED_BATCH_THRESHOLD_PERCENT_DIVISOR)
.max(MIN_SPILLED_BATCH_IN_MEMORY_THRESHOLD_BYTES);
```
--
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]
