241: Plumb remaining parameters for ST_Buffer#2
Conversation
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)c/sedona-geos/src/register.rs (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
c/sedona-geos/src/register.rs (1)
72-74: Remove duplicate kernel registrations at lines 72-73 and review st_buffer duplication at lines 46-47.Verification confirms st_crosses (line 72) and st_overlaps (line 73) are exact duplicates of earlier entries (lines 53 and 65 respectively) and should be removed. However, st_buffer also appears twice at lines 46-47 with different implementations (st_buffer_impl() and st_buffer_style_impl()). This requires review—if both buffer variants should be exposed under the same key, document the intent; otherwise, one registration should be removed or renamed to avoid the duplicate key conflict.
- ("st_crosses", st_crosses_impl()), - ("st_overlaps", st_overlaps_impl()),
🧹 Nitpick comments (4)
c/sedona-geos/src/st_buffer.rs (4)
36-48: Clarify contract: style string must be scalar; document alias.Please note explicitly that bufferStyleParameters must be a scalar constant (not an array), and list the accepted alias for quadrant segments.
Apply this doc tweak:
/// - ST_Buffer(geometry: Geometry, distance: Double, bufferStyleParameters: String) /// -/// Buffer style parameters format: "key1=value1 key2=value2 ..." +/// Buffer style parameters format: "key1=value1 key2=value2 ..." +/// Notes: +/// - bufferStyleParameters must be a scalar (constant), not an array. +/// - 'quad_segs' and 'quadrant_segments' are accepted aliases.
123-131: Avoid potential panic on distance_iter.next().unwrap().Low-risk but easy hardening: return a clear error instead of panicking if iter lengths ever diverge.
- match (wkb, distance_iter.next().unwrap()) { + let next_dist = distance_iter + .next() + .ok_or_else(|| DataFusionError::Execution( + "Internal error: distance iterator shorter than num_iterations".into() + ))?; + match (wkb, next_dist) {
154-168: Style param currently scalar-only; consider supporting arrays or make the restriction explicit.Runtime rejects array inputs even though return_type accepts any string. Either:
- support Utf8 arrays (row-wise parsing), or
- keep scalar-only but document it prominently and, if possible, validate earlier.
Would you like a follow-up patch to add array support by broadcasting a Utf8 iterator alongside distance?
514-517: Consider validating quadrant_segments >= 1.Allowing negative/zero segments likely degrades buffer quality and may rely on GEOS clamping. Prefer early validation to return a helpful error.
If you adopt this, adjust this test to expect Err for quad_segs <= 0.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
c/sedona-geos/src/register.rs(2 hunks)c/sedona-geos/src/st_buffer.rs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
c/sedona-geos/src/register.rs (1)
c/sedona-geos/src/st_buffer.rs (2)
st_buffer_impl(48-50)st_buffer_style_impl(74-76)
c/sedona-geos/src/st_buffer.rs (5)
rust/sedona-expr/src/scalar_udf.rs (4)
args(230-234)args(244-248)args(250-260)new(126-140)rust/sedona-schema/src/matchers.rs (5)
args(49-56)new(35-37)is_geometry(164-166)is_numeric(179-181)is_string(184-186)c/sedona-geos/src/st_dwithin.rs (3)
invoke_batch(51-72)invoke_scalar(75-80)udf(96-152)rust/sedona-functions/src/st_point.rs (1)
invoke_batch(94-147)c/sedona-geos/src/st_centroid.rs (3)
invoke_batch(47-70)invoke_scalar(73-84)udf(99-123)
🔇 Additional comments (2)
c/sedona-geos/src/st_buffer.rs (1)
70-72: Shared batch path LGTM.Reusing invoke_batch_impl keeps both kernels consistent.
c/sedona-geos/src/register.rs (1)
47-48: Dual-kernel registration LGTM.Arity-based dispatch for "st_buffer" (2-arg vs 3-arg) is appropriate.
| _ => { | ||
| return Err(DataFusionError::Execution(format!( | ||
| "Invalid buffer parameter: {} (accept: 'endcap', 'join', 'mitre_limit', 'miter_limit', 'quad_segs' and 'side')", | ||
| key.to_lowercase() | ||
| ))); | ||
| } | ||
| } |
There was a problem hiding this comment.
Error message omits a supported alias.
Message lists 'quad_segs' but not 'quadrant_segments', which is accepted.
- "Invalid buffer parameter: {} (accept: 'endcap', 'join', 'mitre_limit', 'miter_limit', 'quad_segs' and 'side')",
+ "Invalid buffer parameter: {} (accept: 'endcap', 'join', 'mitre_limit', 'miter_limit', 'quad_segs', 'quadrant_segments' and 'side')",If you adopt this change, update the corresponding test expectation in Lines 491-493:
- "Invalid buffer parameter: unknown_param (accept: 'endcap', 'join', 'mitre_limit', 'miter_limit', 'quad_segs' and 'side')"
+ "Invalid buffer parameter: unknown_param (accept: 'endcap', 'join', 'mitre_limit', 'miter_limit', 'quad_segs', 'quadrant_segments' and 'side')"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _ => { | |
| return Err(DataFusionError::Execution(format!( | |
| "Invalid buffer parameter: {} (accept: 'endcap', 'join', 'mitre_limit', 'miter_limit', 'quad_segs' and 'side')", | |
| key.to_lowercase() | |
| ))); | |
| } | |
| } | |
| _ => { | |
| return Err(DataFusionError::Execution(format!( | |
| "Invalid buffer parameter: {} (accept: 'endcap', 'join', 'mitre_limit', 'miter_limit', 'quad_segs', 'quadrant_segments' and 'side')", | |
| key.to_lowercase() | |
| ))); | |
| } | |
| } |
🤖 Prompt for AI Agents
In c/sedona-geos/src/st_buffer.rs around lines 205 to 211, the error message for
invalid buffer parameters omits the accepted alias 'quadrant_segments'; update
the formatted string to include 'quadrant_segments' alongside 'quad_segs' (and
keep other names unchanged), and also update the corresponding test expectation
on lines 491-493 to reflect the new message text.
There was a problem hiding this comment.
value:useful; category:bug; feedback:The AI reviewer caught an incomplete error message which could mislead a developer what are the supported buffer parameter names.
There was a problem hiding this comment.
@martin-augment Thank you for confirming this catch! I'm glad the review identified this potential source of confusion. Including all accepted parameter aliases in error messages helps developers understand the full API surface without having to dig through the source code.
✏️ Learnings added
Learnt from: martin-augment
PR: martin-augment/sedona-db#2
File: c/sedona-geos/src/st_buffer.rs:205-211
Timestamp: 2025-10-27T06:46:18.139Z
Learning: In Rust code for c/sedona-geos/src/st_buffer.rs, when error messages list accepted parameter names for buffer style parameters, ensure all aliases are included. For example, both 'quad_segs' and 'quadrant_segments' should be mentioned in the error message since both are accepted by the parsing logic.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| fn parse_side(value: &str) -> Result<bool> { | ||
| match value.to_lowercase().as_str() { | ||
| "both" => Ok(false), | ||
| "left" | "right" => Ok(true), | ||
| _ => Err(DataFusionError::Execution(format!( | ||
| "Invalid side: '{}'. Valid options: both, left, right", | ||
| value | ||
| ))), | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
left/right semantics are lost (maps only to single_sided).
parse_side collapses 'left' and 'right' into a boolean passed to BufferParams::single_sided(true), but doesn’t select side. This can mislead users: left/right behave identically.
Options:
- Implement side selection (e.g., parse_side -> enum {Both, Left, Right} and flip the distance sign or use GEOS semantics for right side when single-sided).
- Or reject 'left'/'right' for now with a clear error to avoid incorrect results and document the limitation.
Would you like me to draft a minimal patch for either approach and add tests?
🌐 Web query:
What does GEOS/JTS "single-sided buffer" select by default (left vs right)? Is choosing the opposite side supported (e.g., via distance sign) when using BufferParams::single_sided(true)?
💡 Result:
Short answer: single‑sided buffers pick side by the sign of the distance — positive = left, negative = right (relative to the line vertex order). So setting BufferParams::setSingleSided(true) and using a negative distance selects the opposite side. (locationtech.github.io)
Citations:
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Find BufferParams instantiation and understand how parse_side is used
rg -n "BufferParams" c/sedona-geos/src/st_buffer.rs -B2 -A8Length of output: 2053
🏁 Script executed:
#!/bin/bash
# Get full context around the parse_side function and its callers
rg -n "parse_side\|single_sided" c/sedona-geos/src/st_buffer.rs -B3 -A3Length of output: 0
🏁 Script executed:
#!/bin/bash
# View the full st_buffer.rs file to understand the complete flow
cat c/sedona-geos/src/st_buffer.rsLength of output: 17931
Implement left/right side selection via distance sign negation.
Currently, parse_side() maps both "left" and "right" to true, passed to single_sided(true). Since the distance is never modified, both produce identical buffers and users cannot distinguish between left and right—the semantics are lost.
GEOS/JTS selects sides by distance sign: positive = left, negative = right. To fix this, negate the distance when "right" is specified:
fn parse_side(value: &str) -> Result<SideMode> {
match value.to_lowercase().as_str() {
"both" => Ok(SideMode::Both),
"left" => Ok(SideMode::Left),
"right" => Ok(SideMode::Right),
_ => Err(DataFusionError::Execution(format!(
"Invalid side: '{}'. Valid options: both, left, right",
value
))),
}
}Then in invoke_scalar, apply the distance transformation based on side selection before calling buffer_with_params.
There was a problem hiding this comment.
value:useful; category:bug; feedback:The AI reviewer gives a good reason to improve the API and return an enum with variants indicating the side and let the callers decide how to use it. For example params_builder.single_sided() could use the new enum to decide whether it is single-sided (LEFT or RIGHT) or not (BOTH). Currently parse_side() is a midleading name for this method.
There was a problem hiding this comment.
@martin-augment Thank you for the feedback! I agree completely—returning an enum makes the API much clearer and lets callers make informed decisions about side selection.
Here's a concrete implementation:
#[derive(Debug, Clone, Copy)]
enum BufferSide {
Both,
Left,
Right,
}
fn parse_side(value: &str) -> Result<BufferSide> {
match value.to_lowercase().as_str() {
"both" => Ok(BufferSide::Both),
"left" => Ok(BufferSide::Left),
"right" => Ok(BufferSide::Right),
_ => Err(DataFusionError::Execution(format!(
"Invalid side: '{}'. Valid options: both, left, right",
value
))),
}
}Then in parse_buffer_params, store the side selection:
fn parse_buffer_params(params_str: Option<&str>) -> Result<(BufferParams, BufferSide)> {
let Some(params_str) = params_str else {
return Ok((
BufferParams::builder()
.build()
.map_err(|e| DataFusionError::External(Box::new(e)))?,
BufferSide::Both,
));
};
let mut params_builder = BufferParams::builder();
let mut side = BufferSide::Both;
for param in params_str.split_whitespace() {
let Some((key, value)) = param.split_once('=') else {
return Err(DataFusionError::Execution(format!(
"Missing value for buffer parameter: {}",
param
)));
};
match key.to_lowercase().as_str() {
// ... other cases ...
"side" => {
side = parse_side(value)?;
}
// ... rest ...
}
}
// Configure single_sided based on the enum
match side {
BufferSide::Both => {}
BufferSide::Left | BufferSide::Right => {
params_builder = params_builder.single_sided(true);
}
}
Ok((
params_builder
.build()
.map_err(|e| DataFusionError::External(Box::new(e)))?,
side,
))
}Finally, in invoke_batch_impl, apply the distance sign based on side:
let (params, side) = parse_buffer_params(buffer_style_params.as_deref())?;
// In the executor loop:
let effective_distance = match side {
BufferSide::Both | BufferSide::Left => distance,
BufferSide::Right => -distance,
};
invoke_scalar(&wkb, effective_distance, ¶ms, &mut builder)?;This approach:
- Makes the API self-documenting with the
BufferSideenum - Separates parsing concerns from execution logic
- Correctly implements left/right semantics via distance sign (as GEOS expects)
- Allows future extensions (e.g., other side modes)
Would you like me to prepare the complete patch with updated tests?
🐰✨
/> __ __
( (°º°) 🎨
\__\__/
\|/ Sketching cleaner APIs!
✏️ Learnings added
Learnt from: martin-augment
PR: martin-augment/sedona-db#2
File: c/sedona-geos/src/st_buffer.rs:243-252
Timestamp: 2025-10-27T06:50:54.296Z
Learning: In the ST_Buffer implementation in c/sedona-geos/src/st_buffer.rs, the `parse_side()` function should return an enum (e.g., BufferSide with variants Both, Left, Right) instead of a boolean, to properly distinguish between left and right side buffer operations. The distance sign should be negated for right-side buffers since GEOS uses positive distance for left and negative for right.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| fn parse_side(value: &str) -> Result<bool> { | ||
| match value.to_lowercase().as_str() { | ||
| "both" => Ok(false), | ||
| "left" | "right" => Ok(true), |
There was a problem hiding this comment.
The side parameter doesn’t distinguish between left and right: both map to single_sided(true) without changing direction, so side=right will not be honored and behavior defaults to GEOS’s single-sided default (typically left). This can lead to incorrect buffers when callers request the right side.
🤖 React with 👍 or 👎 to let us know if the comment was useful.
There was a problem hiding this comment.
value:incorrect-but-reasonable; category:bug; feedback:The author's intention is to check whether it is a single-sided buffer (left or right). The method name is confusing and could be improved.
Code Review: ST_Buffer Style ParametersI've reviewed PR #2 which adds style-customizable buffer parameters to the ST_Buffer function. Here's my detailed feedback: ✅ StrengthsCode Quality
Test Coverage
🔍 Issues & Concerns1. Critical: Duplicate Function Registration (c/sedona-geos/src/register.rs:42-43)Both
2. Potential Bug: Side Parameter Implementation (c/sedona-geos/src/st_buffer.rs:243-250)The "left" | "right" => Ok(true),Both map to 3. Logic Issue: Distance Iterator Usage (c/sedona-geos/src/st_buffer.rs:115-122)The distance iterator is created but called with
4. Code Duplication
⚡ Performance Considerations1. Parameter Parsing InefficiencyBuffer style parameters are parsed on every batch invocation (line 117): let params = parse_buffer_params(buffer_style_params.as_deref())?;Recommendation: If the parameter string is a scalar constant, parse it once before the loop, not for every geometry in the batch. 2. String AllocationsMultiple
Recommendation: Use if key.eq_ignore_ascii_case("endcap") { ... }🔒 Security Considerations
📋 Additional Recommendations
SummaryThis is a solid implementation with excellent test coverage. The main concerns are:
Overall quality: Good with minor issues to address before merging. Review generated with assistance from Claude Code |
Here is the code flow:
|
value:incorrect-but-reasonable; category:bug; feedback:The author's intention is to check whether it is a single-sided buffer (left or right). The method name is confusing and could be improved. |
value:useful; category:bug; feedback:The AI reviewer correctly points out that it is not recommended to call |
value:useful; category:bug; feedback:The AI reviewer is correct that the implementation could use an optional ArgMatcher for the third argument (the style) and use only one struct/kernel instead of two. https://github.com/apache/sedona-db/blob/8e6234c7401fc72895752558cc6aeaedd1ff470a/rust/sedona-schema/src/matchers.rs#L202 |
value:annoying; category:bug; feedback:The parsing of the buffer parameters is done just once at https://github.com/apache/sedona-db/pull/241/files#diff-9a014d55d337a1146c2dedbde9e58c84b59f0d30d5058bf6605a390ece09431cR117. There is no loop around it. |
value:useful; category:bug; feedback:The AI agent is correct that using to_lowercase() would cause heap allocations. Using eq_ignore_ascii_case() would be better but it would need many if/else if/else if/else clauses. |
2 participants
241: To review by AI
Summary by CodeRabbit
New Features
Tests