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: add support for array_remove expression #1179

Merged
merged 15 commits into from
Jan 12, 2025
Merged
Show file tree
Hide file tree
Changes from 6 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
31 changes: 31 additions & 0 deletions native/core/src/execution/planner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ use datafusion::{
};
use datafusion_comet_spark_expr::{create_comet_physical_fun, create_negate_expr};
use datafusion_functions_nested::concat::ArrayAppend;
use datafusion_functions_nested::remove::array_remove_all_udf;
use datafusion_physical_expr::aggregate::{AggregateExprBuilder, AggregateFunctionExpr};

use crate::execution::shuffle::CompressionCodec;
Expand Down Expand Up @@ -735,6 +736,36 @@ impl PhysicalPlanner {
));
Ok(array_has_expr)
}
ExprStruct::ArrayRemove(expr) => {
let src_array_expr =
self.create_expr(expr.left.as_ref().unwrap(), Arc::clone(&input_schema))?;
let key_expr =
self.create_expr(expr.right.as_ref().unwrap(), Arc::clone(&input_schema))?;
let args = vec![Arc::clone(&src_array_expr), Arc::clone(&key_expr)];
let return_type = src_array_expr.data_type(&input_schema)?;

let datafusion_array_remove = array_remove_all_udf();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This can be inlined


let array_remove_expr: Arc<dyn PhysicalExpr> = Arc::new(ScalarFunctionExpr::new(
"array_remove",
datafusion_array_remove,
args,
return_type,
));
let is_null_expr: Arc<dyn PhysicalExpr> = Arc::new(IsNullExpr::new(key_expr));

let null_literal_expr: Arc<dyn PhysicalExpr> =
Arc::new(Literal::new(ScalarValue::Null));

let case_expr = CaseExpr::try_new(
None,
vec![(is_null_expr, null_literal_expr)],
Some(array_remove_expr),
)?;

Ok(Arc::new(case_expr))
// Ok(array_remove_expr)
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove this commented out code?

Suggested change
// Ok(array_remove_expr)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed ! 😅

}
expr => Err(ExecutionError::GeneralError(format!(
"Not implemented: {:?}",
expr
Expand Down
1 change: 1 addition & 0 deletions native/proto/src/proto/expr.proto
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ message Expr {
BinaryExpr array_append = 58;
ArrayInsert array_insert = 59;
BinaryExpr array_contains = 60;
BinaryExpr array_remove = 61;
}
}

Expand Down
12 changes: 11 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 @@ -2266,6 +2266,12 @@ object QueryPlanSerde extends Logging with ShimQueryPlanSerde with CometExprShim
withInfo(expr, "unsupported arguments for GetArrayStructFields", child)
None
}
case expr if expr.prettyName == "array_remove" =>
createBinaryExpr(
expr.children(0),
expr.children(1),
inputs,
(builder, binaryExpr) => builder.setArrayRemove(binaryExpr))
case expr if expr.prettyName == "array_contains" =>
createBinaryExpr(
expr.children(0),
Expand Down Expand Up @@ -2859,7 +2865,11 @@ object QueryPlanSerde extends Logging with ShimQueryPlanSerde with CometExprShim
case RightOuter => JoinType.RightOuter
case FullOuter => JoinType.FullOuter
case LeftSemi => JoinType.LeftSemi
case LeftAnti => JoinType.LeftAnti
// TODO: DF SMJ with join condition fails TPCH q21
case LeftAnti if condition.isEmpty => JoinType.LeftAnti
case LeftAnti =>
withInfo(join, "LeftAnti SMJ join with condition is not supported")
return None
case _ =>
// Spark doesn't support other join types
withInfo(op, s"Unsupported join type ${join.joinType}")
Expand Down
12 changes: 12 additions & 0 deletions spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2529,4 +2529,16 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
spark.sql("SELECT array_contains((CASE WHEN _2 =_3 THEN array(_4) END), _4) FROM t1"));
}
}
test("array_remove") {
Copy link
Member

Choose a reason for hiding this comment

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

nit: there should be a blank line between tests:

Suggested change
}
test("array_remove") {
}
test("array_remove") {

Seq(true, false).foreach { dictionaryEnabled =>
withTempDir { dir =>
val path = new Path(dir.toURI.toString, "test.parquet")
makeParquetFileAllTypes(path, dictionaryEnabled, 10000)
spark.read.parquet(path.toString).createOrReplaceTempView("t1");
Copy link
Member

Choose a reason for hiding this comment

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

nit: no need for semicolon:

Suggested change
spark.read.parquet(path.toString).createOrReplaceTempView("t1");
spark.read.parquet(path.toString).createOrReplaceTempView("t1")

checkSparkAnswerAndOperator(sql("SELECT array_remove(array(_2, _3,_4), _2) from t1"))
checkSparkAnswerAndOperator(sql("SELECT array_remove(array(_2, _3,_4), 3) from t1"))
checkSparkAnswerAndOperator(sql("SELECT array_remove(array(_2, _3,_4), 10) from t1"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add test case where key is null ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also can we tests where the array is null, array is empty, and array contains nulls.

}
}
}
}