IgnatiusPang commented on code in PR #25746:
URL: https://github.com/apache/datafusion/pull/25746#discussion_r4172117047
##########
datafusion/functions/src/math/log.rs:
##########
@@ -813,22 +840,50 @@ mod tests {
assert_eq!(results, expected);
// Test with different `nulls_first`
- let base_order = ExprProperties::new_unknown().with_order(
- SortProperties::Ordered(SortOptions {
+ let base_order = ExprProperties::new_unknown()
+ .with_range(gt_one_range.clone())
+ .with_order(SortProperties::Ordered(SortOptions {
descending: true,
nulls_first: true,
- }),
- );
- let num_order = ExprProperties::new_unknown().with_order(
- SortProperties::Ordered(SortOptions {
+ }));
+ let num_order = ExprProperties::new_unknown()
+ .with_range(gt_one_range.clone())
+ .with_order(SortProperties::Ordered(SortOptions {
descending: false,
nulls_first: false,
- }),
- );
+ }));
assert_eq!(
log.output_ordering(&[base_order, num_order]).unwrap(),
SortProperties::Unordered
);
+
+ // Test base in (0, 1), e.g. base = 0.5:
Review Comment:
Agreed, the slt tests are the real regression coverage. I kept these because
they check output_ordering directly, without depending on what the planner
knows about bounds. The second one also covers a constant base with unknown
bounds (the "can't prove base > 1" path), which the slt tests with literal
bases don't reach. Happy to drop them, or just the [0.2, 0.8] one, if you'd
rather keep the unit tests lean.
--
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]
