gene-bordegaray commented on code in PR #24698:
URL: https://github.com/apache/datafusion/pull/24698#discussion_r3922887000
##########
datafusion/datasource/src/source.rs:
##########
@@ -705,3 +718,90 @@ where
Self::new(Arc::new(source))
}
}
+
+#[cfg(test)]
+mod tests {
Review Comment:
I think this is largely covered inderectly on other tests 👍
##########
datafusion/physical-plan/src/test.rs:
##########
@@ -363,6 +367,51 @@ impl TestMemoryExec {
Ok(self)
}
+ /// Attach a composite key whose values occur in one contiguous range in
+ /// each output stream. See
+ ///
[`ExecutionPlanProperties::group_contiguous_exprs`](crate::ExecutionPlanProperties::group_contiguous_exprs)
+ /// for the correctness contract.
+ pub fn try_with_group_contiguous_exprs(
Review Comment:
nice helper 👍
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1526,6 +1546,79 @@ mod tests {
BinaryExpr, Column, DynamicFilterPhysicalExpr, Literal, binary, col,
lit,
};
+ #[test]
+ fn group_contiguous_projection_is_all_or_nothing() -> Result<()> {
Review Comment:
nit: could we split this into smaller tests that cover the invariants
1. projection works with exact match (order flipped around) and additional
cols
2. projection rejected if not all cols are in it (maybe an additional col in
there)
##########
datafusion/ffi/src/expr/expr_properties.rs:
##########
@@ -113,3 +116,14 @@ impl From<&FFI_SortOptions> for SortOptions {
}
}
}
+
+#[cfg(test)]
Review Comment:
dont thinking we should add new test module just for this guys 👍
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
