viirya commented on code in PR #6761:
URL: https://github.com/apache/datafusion-comet/pull/6761#discussion_r4243786514
##########
native/spark-expr/src/math_funcs/greatest_least.rs:
##########
@@ -51,11 +51,11 @@ impl SparkGreatestLeast {
}
}
- /// Whether arguments of this type need Spark's float ordering.
DataFusion's `greatest` and
- /// `least` already match Spark for every other type, since equal values
are identical there.
+ /// Whether arguments of this type need Spark's ordering, of floats or of
nulls inside nested
+ /// values. DataFusion's `greatest` and `least` already match Spark for
every other type, since
+ /// equal values are identical there.
pub fn handles(data_type: &DataType) -> bool {
- matches!(data_type, DataType::Float32 | DataType::Float64)
- || (data_type.is_nested() && has_float_leaf(data_type))
+ matches!(data_type, DataType::Float32 | DataType::Float64) ||
data_type.is_nested()
Review Comment:
Agreed. fb630b60a reduces all constants of a type without a float leaf to
one, wherever they are, since equal values of such types are identical. Float
types still reduce only adjacent constants, which keeps the earlier of `-0.0`
and `0.0`. Your `constant, column, constant` input now allocates 1,215,900
bytes versus 1,215,772 through DataFusion's functions (2,431,384 at 5f0602251),
with identical output for both `greatest` and `least`.
##########
native/spark-expr/src/math_funcs/greatest_least.rs:
##########
@@ -93,6 +93,30 @@ impl SparkGreatestLeast {
}
}
+ /// Replaces each run of adjacent constant arguments with the one constant
the run picks, so
+ /// that a run is compared once rather than broadcast and compared row by
row for each of its
+ /// constants. The result is the first argument that no later argument
ranks strictly before,
+ /// and picking within a run keeps that, so equal floats such as `-0.0`
and `0.0` still resolve
+ /// to the earlier argument. Constants that are not adjacent stay apart
for the same reason.
+ /// `to_array` turns a constant into a one-row array of the result type.
+ fn merge_constant_runs(
+ &self,
+ args: &[ColumnarValue],
+ to_array: impl Fn(&ColumnarValue) -> Result<ArrayRef>,
+ ) -> Result<Vec<ColumnarValue>> {
+ let mut merged: Vec<ColumnarValue> = Vec::with_capacity(args.len());
+ for arg in args {
+ match (merged.last_mut(), arg) {
+ (Some(previous @ ColumnarValue::Scalar(_)),
ColumnarValue::Scalar(_)) => {
+ let picked = self.pick(&to_array(previous)?,
&to_array(arg)?)?;
+ *previous =
ColumnarValue::Scalar(ScalarValue::try_from_array(&picked, 0)?);
Review Comment:
Done in fb630b60a. The reduction now compares the one-row arrays with
`spark_comparator` and keeps the selected constant itself, so nothing is
copied. Your long-literal input now allocates 201,324 bytes versus 200,940
through DataFusion's functions (2,052,684 at 5f0602251).
`constants_merge_in_spark_order` checks that the kept constant is the original
one.
--
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]
