james-willis commented on code in PR #1225:
URL: https://github.com/apache/sedona-db/pull/1225#discussion_r3929236585
##########
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:
Yeah I was overly cautious trying to maintain the wire format. I can't think
of a reason that it'd be neccessary.
Serializing to use the u32 simplifies all the code here.
--
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]
