Copilot commented on code in PR #687:
URL: https://github.com/apache/sedona-db/pull/687#discussion_r2885252720
##########
sedona-cli/src/main.rs:
##########
@@ -66,15 +66,14 @@ struct Args {
#[clap(
short = 'm',
long,
- help = "The memory pool limitation (e.g. '10g'), default to None (no
limit)",
- value_parser(extract_memory_pool_size)
+ help = "The memory pool limitation (e.g. '10g'), default to 75% of
physical memory. Use 'unlimited' to disable"
)]
- memory_limit: Option<usize>,
+ memory_limit: Option<String>,
Review Comment:
The CLI no longer validates/parses `--memory-limit` at argument parsing time
(the `value_parser` was removed), so invalid values will now fail later with a
generic runtime error instead of Clap showing a proper usage error. Consider
restoring Clap-side parsing by using a custom `value_parser` (or a small
`FromStr` wrapper type) that accepts either a size string or the special
`unlimited` value.
##########
rust/sedona/src/context_builder.rs:
##########
@@ -33,26 +33,53 @@ use crate::{
size_parser,
};
+/// The fraction of total physical memory to use as the default memory limit.
+const DEFAULT_MEMORY_FRACTION: f64 = 0.75;
+
+/// Compute the default memory limit as 75% of total physical memory.
+fn default_memory_limit() -> usize {
+ let mut sys = sysinfo::System::new();
+ sys.refresh_memory();
+ let total = sys.total_memory() as f64;
+ (total * DEFAULT_MEMORY_FRACTION) as usize
Review Comment:
`sysinfo::System::total_memory()` is not guaranteed to be in bytes (it has
historically been reported in KiB). Since `memory_limit` is documented/used as
bytes (and passed directly to DataFusion memory pools), please ensure the units
match (e.g., convert KiB→bytes) and consider adding an inline comment asserting
the unit to prevent future regressions.
```suggestion
// `System::total_memory` has historically returned KiB; convert to
bytes to match
// the `memory_limit` contract and DataFusion's expectation of byte
units.
let total_bytes = (sys.total_memory() as f64) * 1024.0;
(total_bytes * DEFAULT_MEMORY_FRACTION) as usize
```
--
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]
