paleolimbot commented on code in PR #642:
URL: https://github.com/apache/sedona-db/pull/642#discussion_r2842904062
##########
python/sedonadb/src/dataframe.rs:
##########
@@ -189,13 +189,52 @@ impl InternalDataFrame {
) -> Result<(), PySedonaError> {
// sort_by needs to be SortExpr. A Vec<String> can unambiguously be
interpreted as
// field names (ascending), but other types of expressions aren't
supported here yet.
+ // We need to special-case geometry columns until we have a logical
optimizer rule or
+ // sorting for user-defined types is supported.
+ let geometry_column_indices =
self.inner.schema().geometry_column_indices()?;
+ let geometry_column_names = geometry_column_indices
+ .iter()
+ .map(|i| self.inner.schema().field(*i).name().as_str())
+ .collect::<HashSet<&str>>();
+
+ #[cfg(feature = "s2geography")]
+ let has_geography = true;
+ #[cfg(not(feature = "s2geography"))]
+ let has_geography = false;
+
let sort_by_expr = sort_by
.into_iter()
.map(|name| {
- let column = Expr::Column(Column::new_unqualified(name));
- SortExpr::new(column, true, false)
+ let column =
Expr::Column(Column::new_unqualified(name.clone()));
+ if geometry_column_names.contains(name.as_str()) {
+ // Create the call sd_order(column). If we're ordering by
geometry but don't have
+ // the required feature for high quality sort output, give
an error. This is mostly
+ // an issue when using maturin develop because geography
is not a default feature.
+ if has_geography {
+ let order_udf_opt: Option<ScalarUDF> = ctx
+ .inner
+ .functions
+ .scalar_udf("sd_order")
+ .map(|udf_opt| udf_opt.clone().into());
Review Comment:
Good point...I updated this to clone an existing Arc rather than clone the
SedonaScalarUDF.
--
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]
