Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 39 additions & 1 deletion datafusion/expr-common/src/interval_arithmetic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1243,6 +1243,20 @@ fn prev_value(value: ScalarValue) -> ScalarValue {
value_transition!(MIN, false, value)
}

/// Returns the previous distinct value of `value`, or `None` if `value` is
/// null, already at the type minimum, or a type that has no predecessor.
pub fn checked_predecessor(value: &ScalarValue) -> Option<ScalarValue> {
if value.is_null() {
return None;
}
let predecessor = prev_value(value.clone());
if predecessor.is_null() || predecessor == *value {
None
} else {
Some(predecessor)
}
}

trait OneTrait: Sized + std::ops::Add + std::ops::Sub {
fn one() -> Self;
}
Expand Down Expand Up @@ -2261,7 +2275,8 @@ impl NullableInterval {
mod tests {
use crate::{
interval_arithmetic::{
Interval, handle_overflow, next_value, prev_value, satisfy_greater,
Interval, checked_predecessor, handle_overflow, next_value, prev_value,
satisfy_greater,
},
operator::Operator,
};
Expand Down Expand Up @@ -2358,6 +2373,29 @@ mod tests {
Ok(())
}

#[test]
fn test_checked_predecessor() {
assert_eq!(
checked_predecessor(&ScalarValue::Int64(Some(10))),
Some(ScalarValue::Int64(Some(9)))
);
assert_eq!(checked_predecessor(&ScalarValue::Int64(None)), None);
assert_eq!(
checked_predecessor(&ScalarValue::Int64(Some(i64::MIN))),
None
);
assert_eq!(
checked_predecessor(&ScalarValue::TimestampNanosecond(Some(i64::MIN), None)),
None
);
// Types without a discrete predecessor return the same value from
// `prev_value`, which `checked_predecessor` treats as absent.
assert_eq!(
checked_predecessor(&ScalarValue::Utf8(Some("a".into()))),
None
);
}

#[test]
fn test_new_interval() -> Result<()> {
use ScalarValue::*;
Expand Down
25 changes: 25 additions & 0 deletions datafusion/physical-expr/src/equivalence/properties/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1319,6 +1319,31 @@ impl EquivalenceProperties {
.unwrap_or_else(|_| ExprProperties::new_unknown())
}

/// Returns true when `expr` is a (possibly non-strict) monotonic function of
/// `range_key` plus literals, such as `date_bin(interval, timestamp)` or
/// `date_trunc(unit, timestamp)`.
///
/// The identity `expr == range_key` returns false so callers can treat "emit
/// the key as-is" separately from "emit a function of the key".
pub(crate) fn is_monotonic_function_of(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rename this.

  • Doing equivalence_properties.is_monotonic_function_of sounds like the properies are a monotonic function.
  • The two expressions aren't necessarily either functions. They are related via the Dependencies relationship.

Maybe rename to check_monotonic_dependency or check_monotonic_transform?

&self,
expr: &Arc<dyn PhysicalExpr>,
range_key: &Arc<dyn PhysicalExpr>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename these to source_expr and transformed_expr? Definately would not call this range_key because this function has nothing to do with range partitioning.

) -> bool {
if expr.eq(range_key) {
return false;
}
let dependencies = Dependencies::new(std::iter::once(PhysicalSortExpr::new(
Arc::clone(range_key),
Default::default(),
)));
matches!(
get_expr_properties(expr, &dependencies, &self.schema)
.map(|properties| properties.sort_properties),
Ok(SortProperties::Ordered(_))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't capture the ordering, so a non monotonic function like f(x) = -x will return true here even though it flips the ordering from ASC to DESC. Even if -x works for range partitioning, this function may be used for some different use case, in which it will incorrectly return true.

Let's just assert that the options in SortProperties::Ordered(options) are the same as sort_expr.options and add a test for -x.

)
}

/// Transforms this `EquivalenceProperties` by mapping columns in the
/// original schema to columns in the new schema by index.
pub fn with_new_schema(mut self, schema: SchemaRef) -> Result<Self> {
Expand Down
Loading