Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Supports Stddev #348

Merged
merged 6 commits into from
May 7, 2024
Merged
Show file tree
Hide file tree
Changes from 5 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
1 change: 1 addition & 0 deletions core/src/execution/datafusion/expressions/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ pub mod avg_decimal;
pub mod bloom_filter_might_contain;
pub mod covariance;
pub mod stats;
pub mod stddev;
pub mod strings;
pub mod subquery;
pub mod sum_decimal;
Expand Down
181 changes: 181 additions & 0 deletions core/src/execution/datafusion/expressions/stddev.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,181 @@
// Licensed to the Apache Software Foundation (ASF) under one
// or more contributor license agreements. See the NOTICE file
// distributed with this work for additional information
// regarding copyright ownership. The ASF licenses this file
// to you under the Apache License, Version 2.0 (the
// "License"); you may not use this file except in compliance
// with the License. You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.

//! Defines physical expressions that can evaluated at runtime during query execution
Copy link
Member

@viirya viirya May 6, 2024

Choose a reason for hiding this comment

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

Seems copied from somewhere and not related?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed. Thanks


use std::{any::Any, sync::Arc};

use crate::execution::datafusion::expressions::{
stats::StatsType, utils::down_cast_any_ref, variance::VarianceAccumulator,
};
use arrow::{
array::ArrayRef,
datatypes::{DataType, Field},
};
use datafusion::logical_expr::Accumulator;
use datafusion_common::{internal_err, Result, ScalarValue};
use datafusion_physical_expr::{expressions::format_state_name, AggregateExpr, PhysicalExpr};

/// STDDEV and STDDEV_SAMP (standard deviation) aggregate expression
/// The implementation mostly is the same as the DataFusion's implementation. The reason
/// we have our own implementation is that DataFusion has UInt64 for state_field `count`,
/// while Spark has Double for count. Also we have added `null_on_divide_by_zero`
/// to be consistent with Spark's implementation.
#[derive(Debug)]
pub struct Stddev {
name: String,
expr: Arc<dyn PhysicalExpr>,
stats_type: StatsType,
null_on_divide_by_zero: bool,
}

impl Stddev {
/// Create a new STDDEV aggregate function
pub fn new(
expr: Arc<dyn PhysicalExpr>,
name: impl Into<String>,
data_type: DataType,
stats_type: StatsType,
null_on_divide_by_zero: bool,
) -> Self {
// the result of stddev just support FLOAT64 and Decimal data type.
assert!(matches!(data_type, DataType::Float64));
Copy link
Member

Choose a reason for hiding this comment

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

Hmm? So we also need to add DecimalType here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's FLOAT64 only. Removed and Decimal data type

Self {
name: name.into(),
expr,
stats_type,
null_on_divide_by_zero,
}
}
}

impl AggregateExpr for Stddev {
/// Return a reference to Any that can be used for downcasting
fn as_any(&self) -> &dyn Any {
self
}

fn field(&self) -> Result<Field> {
Ok(Field::new(&self.name, DataType::Float64, true))
}

fn create_accumulator(&self) -> Result<Box<dyn Accumulator>> {
Ok(Box::new(StddevAccumulator::try_new(
self.stats_type,
self.null_on_divide_by_zero,
)?))
}

fn create_sliding_accumulator(&self) -> Result<Box<dyn Accumulator>> {
Ok(Box::new(StddevAccumulator::try_new(
self.stats_type,
self.null_on_divide_by_zero,
)?))
}

fn state_fields(&self) -> Result<Vec<Field>> {
Ok(vec![
Field::new(
format_state_name(&self.name, "count"),
DataType::Float64,
true,
),
Field::new(
format_state_name(&self.name, "mean"),
DataType::Float64,
true,
),
Field::new(format_state_name(&self.name, "m2"), DataType::Float64, true),
])
}

fn expressions(&self) -> Vec<Arc<dyn PhysicalExpr>> {
vec![self.expr.clone()]
}

fn name(&self) -> &str {
&self.name
}
}

impl PartialEq<dyn Any> for Stddev {
fn eq(&self, other: &dyn Any) -> bool {
down_cast_any_ref(other)
.downcast_ref::<Self>()
.map(|x| {
self.name == x.name
&& self.expr.eq(&x.expr)
&& self.null_on_divide_by_zero == x.null_on_divide_by_zero
andygrove marked this conversation as resolved.
Show resolved Hide resolved
&& self.stats_type == x.stats_type
})
.unwrap_or(false)
}
}

/// An accumulator to compute the average
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// An accumulator to compute the average
/// An accumulator to compute the standard deviation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed. Thanks

#[derive(Debug)]
pub struct StddevAccumulator {
variance: VarianceAccumulator,
}

impl StddevAccumulator {
/// Creates a new `StddevAccumulator`
pub fn try_new(s_type: StatsType, null_on_divide_by_zero: bool) -> Result<Self> {
Ok(Self {
variance: VarianceAccumulator::try_new(s_type, null_on_divide_by_zero)?,
})
}

pub fn get_m2(&self) -> f64 {
self.variance.get_m2()
}
}

impl Accumulator for StddevAccumulator {
fn state(&mut self) -> Result<Vec<ScalarValue>> {
Ok(vec![
ScalarValue::from(self.variance.get_count()),
ScalarValue::from(self.variance.get_mean()),
ScalarValue::from(self.variance.get_m2()),
])
}

fn update_batch(&mut self, values: &[ArrayRef]) -> Result<()> {
self.variance.update_batch(values)
}

fn retract_batch(&mut self, values: &[ArrayRef]) -> Result<()> {
self.variance.retract_batch(values)
}

fn merge_batch(&mut self, states: &[ArrayRef]) -> Result<()> {
self.variance.merge_batch(states)
}

fn evaluate(&mut self) -> Result<ScalarValue> {
let variance = self.variance.evaluate()?;
match variance {
ScalarValue::Float64(Some(e)) => Ok(ScalarValue::Float64(Some(e.sqrt()))),
ScalarValue::Float64(None) => Ok(ScalarValue::Float64(None)),
_ => internal_err!("Variance should be f64"),
}
}

fn size(&self) -> usize {
std::mem::align_of_val(self) - std::mem::align_of_val(&self.variance) + self.variance.size()
}
}
25 changes: 25 additions & 0 deletions core/src/execution/datafusion/planner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ use crate::{
if_expr::IfExpr,
scalar_funcs::create_comet_physical_fun,
stats::StatsType,
stddev::Stddev,
strings::{Contains, EndsWith, Like, StartsWith, StringSpaceExec, SubstringExec},
subquery::Subquery,
sum_decimal::SumDecimal,
Expand Down Expand Up @@ -1260,6 +1261,30 @@ impl PhysicalPlanner {
))),
}
}
AggExprStruct::Stddev(expr) => {
let child = self.create_expr(expr.child.as_ref().unwrap(), schema.clone())?;
let datatype = to_arrow_datatype(expr.datatype.as_ref().unwrap());
match expr.stats_type {
0 => Ok(Arc::new(Stddev::new(
child,
"stddev",
datatype,
StatsType::Sample,
expr.null_on_divide_by_zero,
))),
1 => Ok(Arc::new(Stddev::new(
child,
"stddev_pop",
datatype,
StatsType::Population,
expr.null_on_divide_by_zero,
))),
stats_type => Err(ExecutionError::GeneralError(format!(
"Unknown StatisticsType {:?} for stddev",
stats_type
))),
}
}
}
}

Expand Down
8 changes: 8 additions & 0 deletions core/src/execution/proto/expr.proto
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ message AggExpr {
CovSample covSample = 12;
CovPopulation covPopulation = 13;
Variance variance = 14;
Stddev stddev = 15;
}
}

Expand Down Expand Up @@ -178,6 +179,13 @@ message Variance {
StatisticsType stats_type = 4;
}

message Stddev {
Expr child = 1;
bool null_on_divide_by_zero = 2;
DataType datatype = 3;
StatisticsType stats_type = 4;
}

message Literal {
oneof value {
bool bool_val = 1;
Expand Down
2 changes: 2 additions & 0 deletions docs/source/user-guide/expressions.md
Original file line number Diff line number Diff line change
Expand Up @@ -107,3 +107,5 @@ The following Spark expressions are currently available:
- CovSample
- VariancePop
- VarianceSamp
- StddevPop
- StddevSamp
42 changes: 41 additions & 1 deletion spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import scala.collection.JavaConverters._

import org.apache.spark.internal.Logging
import org.apache.spark.sql.catalyst.expressions._
import org.apache.spark.sql.catalyst.expressions.aggregate.{AggregateExpression, Average, BitAndAgg, BitOrAgg, BitXorAgg, Count, CovPopulation, CovSample, Final, First, Last, Max, Min, Partial, Sum, VariancePop, VarianceSamp}
import org.apache.spark.sql.catalyst.expressions.aggregate.{AggregateExpression, Average, BitAndAgg, BitOrAgg, BitXorAgg, Count, CovPopulation, CovSample, Final, First, Last, Max, Min, Partial, StddevPop, StddevSamp, Sum, VariancePop, VarianceSamp}
import org.apache.spark.sql.catalyst.expressions.objects.StaticInvoke
import org.apache.spark.sql.catalyst.optimizer.{BuildRight, NormalizeNaNAndZero}
import org.apache.spark.sql.catalyst.plans._
Expand Down Expand Up @@ -504,6 +504,46 @@ object QueryPlanSerde extends Logging with ShimQueryPlanSerde {
withInfo(aggExpr, child)
None
}
case std @ StddevSamp(child, nullOnDivideByZero) =>
val childExpr = exprToProto(child, inputs, binding)
val dataType = serializeDataType(std.dataType)

if (childExpr.isDefined && dataType.isDefined) {
val stdBuilder = ExprOuterClass.Stddev.newBuilder()
stdBuilder.setChild(childExpr.get)
stdBuilder.setNullOnDivideByZero(nullOnDivideByZero)
stdBuilder.setDatatype(dataType.get)
stdBuilder.setStatsTypeValue(0)

Some(
ExprOuterClass.AggExpr
.newBuilder()
.setStddev(stdBuilder)
.build())
} else {
withInfo(aggExpr, child)
None
}
case std @ StddevPop(child, nullOnDivideByZero) =>
val childExpr = exprToProto(child, inputs, binding)
val dataType = serializeDataType(std.dataType)

if (childExpr.isDefined && dataType.isDefined) {
val stdBuilder = ExprOuterClass.Stddev.newBuilder()
stdBuilder.setChild(childExpr.get)
stdBuilder.setNullOnDivideByZero(nullOnDivideByZero)
stdBuilder.setDatatype(dataType.get)
stdBuilder.setStatsTypeValue(1)

Some(
ExprOuterClass.AggExpr
.newBuilder()
.setStddev(stdBuilder)
.build())
} else {
withInfo(aggExpr, child)
None
}
case fn =>
val msg = s"unsupported Spark aggregate function: ${fn.prettyName}"
emitWarning(msg)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ struct<w_warehouse_sk:int,i_item_sk:int,d_moy:int,mean:double,cov:double,w_wareh
1 12259 1 326.5 1.219693210219279 1 12259 2 292.6666666666667 1.2808898286830026
1 12641 1 321.25 1.1286221893301993 1 12641 2 279.25 1.129134558577743
1 13043 1 260.5 1.355894484625015 1 13043 2 295.0 1.056210118409035
1 13157 1 260.5 1.5242630430075292 1 13157 2 413.5 1.0422561797285326
1 13157 1 260.5 1.524263043007529 1 13157 2 413.5 1.0422561797285326
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering what is causing the digit difference...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure what caused the digit difference.
Actually for SortMergeJoin, I got 1.524263043007529, but for BroadCastJoin, I still got 1.5242630430075292. Is it OK if I change the the expected result based on join type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also cc @viirya

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, it would be good to compare floating point numbers based on an epsilon to make sure they are within some tolerance threshold. I assume we are currently just comparing text file output directly? Do we have a way to generate the output into a structured file type such as CSV or JSON?

Copy link
Member

Choose a reason for hiding this comment

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

The difference may be down to order of operations - depending on the order that batches that are being processed from different partitions, for example. I don't think we can expect it to be 100% deterministic in a distributed system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we are currently just comparing text file output directly. We are using Spark's TPCDSQuerySuite. It doesn't seem to be a way to generate the output into a structured file type.

1 13293 1 325.25 1.1599721810918615 1 13293 2 345.75 1.0626233629994524
1 13729 1 486.0 1.0680776434770018 1 13729 2 389.6666666666667 1.3522269473359647
1 14137 1 427.0 1.0418229612154228 1 14137 2 387.5 1.0294855239302605
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
-- !query schema
struct<w_warehouse_sk:int,i_item_sk:int,d_moy:int,mean:double,cov:double,w_warehouse_sk:int,i_item_sk:int,d_moy:int,mean:double,cov:double>
-- !query output
1 13157 1 260.5 1.5242630430075292 1 13157 2 413.5 1.0422561797285326
1 13157 1 260.5 1.524263043007529 1 13157 2 413.5 1.0422561797285326
1 15441 1 295.0 1.597573089984185 1 15441 2 296.0 1.534664092337063
1 16895 1 197.25 1.6216484458744376 1 16895 2 201.33333333333334 1.001838963026182
2 5915 1 231.25 1.8705235468767831 2 5915 2 407.0 1.0885509926353207
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1157,6 +1157,49 @@ class CometAggregateSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}

test("stddev_pop and stddev_samp") {
withSQLConf(CometConf.COMET_EXEC_SHUFFLE_ENABLED.key -> "true") {
Seq(true, false).foreach { cometColumnShuffleEnabled =>
withSQLConf(
CometConf.COMET_COLUMNAR_SHUFFLE_ENABLED.key -> cometColumnShuffleEnabled.toString) {
Seq(true, false).foreach { dictionary =>
withSQLConf("parquet.enable.dictionary" -> dictionary.toString) {
Seq(true, false).foreach { nullOnDivideByZero =>
withSQLConf(
"spark.sql.legacy.statisticalAggregate" -> nullOnDivideByZero.toString) {
val table = "test"
withTable(table) {
sql(s"create table $table(col1 int, col2 int, col3 int, col4 float, " +
"col5 double, col6 int) using parquet")
sql(s"insert into $table values(1, null, null, 1.1, 2.2, 1), " +
"(2, null, null, 3.4, 5.6, 1), (3, null, 4, 7.9, 2.4, 2)")
val expectedNumOfCometAggregates = 2
checkSparkAnswerWithTolAndNumOfAggregates(
"SELECT stddev_samp(col1), stddev_samp(col2), stddev_samp(col3), " +
"stddev_samp(col4), stddev_samp(col5) FROM test",
expectedNumOfCometAggregates)
checkSparkAnswerWithTolAndNumOfAggregates(
"SELECT stddev_pop(col1), stddev_pop(col2), stddev_pop(col3), " +
"stddev_pop(col4), stddev_pop(col5) FROM test",
expectedNumOfCometAggregates)
checkSparkAnswerAndNumOfAggregates(
"SELECT stddev_samp(col1), stddev_samp(col2), stddev_samp(col3), " +
"stddev_samp(col4), stddev_samp(col5) FROM test GROUP BY col6",
expectedNumOfCometAggregates)
checkSparkAnswerWithTolAndNumOfAggregates(
"SELECT stddev_pop(col1), stddev_pop(col2), stddev_pop(col3), " +
"stddev_pop(col4), stddev_pop(col5) FROM test GROUP BY col6",
expectedNumOfCometAggregates)
}
}
}
}
}
}
}
}
}

protected def checkSparkAnswerAndNumOfAggregates(query: String, numAggregates: Int): Unit = {
val df = sql(query)
checkSparkAnswer(df)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,12 @@ class CometTPCDSQuerySuite
"q36",
"q37",
"q38",
"q39a",
"q39b",
// TODO: comment 39a and 39b for now because the expected result for stddev failed:
// expected: 1.5242630430075292, actual: 1.524263043007529.
// Will change the comparison logic to detect floating-point numbers and compare
// with epsilon
// "q39a",
// "q39b",
Copy link
Member

Choose a reason for hiding this comment

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

We should create a ticket for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

opened #392

"q40",
"q41",
"q42",
Expand Down
Loading