paleolimbot commented on code in PR #1225:
URL: https://github.com/apache/sedona-db/pull/1225#discussion_r3920719379
##########
rust/sedona-functions/src/st_collect_agg.rs:
##########
@@ -205,14 +208,17 @@ impl Accumulator for CollectionAccumulator {
}
fn state(&mut self) -> Result<Vec<ScalarValue>> {
- let geometry_types_value =
-
serde_json::to_string(&self.unique_geometry_types.iter().collect::<Vec<_>>())
- .map_err(|e| DataFusionError::External(Box::new(e)))?;
+ // Both columns keep the exact pre-bitset wire format: a JSON list of
+ // geometry types, and a JSON list of dimensions wrapped as
+ // (Geometry, dimensions) pairs.
+ let geometry_types_value =
serde_json::to_string(&self.types_and_dims.geometry_types())
Review Comment:
I don't think the wire format needs to be maintained?
##########
rust/sedona-geometry/src/types.rs:
##########
@@ -429,6 +429,28 @@ impl GeometryTypeAndDimensionsSet {
current_bit: 0,
}
}
+
+ /// The distinct geometry types in this set, in bitset iteration order.
+ pub fn geometry_types(&self) -> Vec<GeometryTypeId> {
+ let mut out = Vec::new();
+ for item in self.iter() {
+ if !out.contains(&item.geometry_type()) {
+ out.push(item.geometry_type());
+ }
+ }
+ out
+ }
+
+ /// The distinct dimensions in this set, in bitset iteration order.
+ pub fn dimensions(&self) -> Vec<Dimensions> {
+ let mut out = Vec::new();
+ for item in self.iter() {
+ if !out.contains(&item.dimensions()) {
+ out.push(item.dimensions());
+ }
+ }
+ out
+ }
Review Comment:
These seem like inefficient ways (particular vector contains) to do
these...I think you can probably create bitmasks and & them against the current
value if you truly need these.
##########
rust/sedona-functions/src/st_collect_agg.rs:
##########
@@ -525,4 +545,61 @@ mod test {
"CRS values not equal: ogc:crs84 vs epsg:3857"
);
}
+
+ /// The bitset swap must not change the serialized state wire format:
+ /// column 0 is a JSON list of geometry types and column 1 a JSON list of
+ /// dimensions wrapped as (Geometry, dims) pairs, exactly as the HashSet
+ /// implementation produced, so states merge across versions.
+ #[test]
+ fn state_wire_format_is_unchanged() {
Review Comment:
States merging across versions sounds fishy. Am I missing something about
how this can be run?
##########
rust/sedona-functions/src/st_collect_agg.rs:
##########
@@ -275,10 +281,24 @@ impl Accumulator for CollectionAccumulator {
)
.map_err(|e| DataFusionError::External(Box::new(e)))?
.into_iter()
- .map(|item| item.dimensions());
-
- self.unique_geometry_types.extend(geometry_types);
- self.unique_dimensions.extend(dimensions);
+ .map(|item| item.dimensions())
+ .collect::<Vec<_>>();
+
+ // The state stores the two marginals; only the marginals
+ // are ever consumed, so inserting the cross product
+ // reconstructs them exactly in the pair bitset. (A state
+ // produced by update_batch never has one marginal empty
+ // while the other is not.)
+ for geometry_type in &geometry_types {
+ for dimensions in &dimensions {
+ self.types_and_dims
+ .insert(&GeometryTypeAndDimensions::new(
+ *geometry_type,
+ *dimensions,
+ ))
+ .map_err(|e|
DataFusionError::External(Box::new(e)))?;
+ }
+ }
Review Comment:
How about just storing the 64-bit integer as the state? Otherwise, this
comment needs to be rewritten for humans.
--
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]
