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.

Sign up for GitHub

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

Jump to bottom

Support Serde for ScalarUDF in Physical Expressions #9436

Merged
merged 14 commits into from
Mar 19, 2024
Merged

Support Serde for ScalarUDF in Physical Expressions #9436

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9ab42e2
initial try
yyy1000 Mar 3, 2024
8d7c866
revert
yyy1000 Mar 3, 2024
ee9c36f
Merge branch 'main' into serde-phsyical
yyy1000 Mar 12, 2024
6ecfb11
Merge branch 'serde-phsyical' of https://github.com/yyy1000/arrow-dat…
yyy1000 Mar 12, 2024
6b154e0
stage commit
yyy1000 Mar 12, 2024
ecde242
Merge branch 'main' into serde-phsyical
yyy1000 Mar 13, 2024
b33656a
use ScalarFunctionDefinition to rewrite PhysicalExpr proto
yyy1000 Mar 13, 2024
1738529
cargo fmt
yyy1000 Mar 13, 2024
93d9750
feat : add test
yyy1000 Mar 13, 2024
376905b
fix bug
yyy1000 Mar 13, 2024
d8d1eb2
fix wrong delete code when resolve conflict
yyy1000 Mar 13, 2024
623e56c
Update datafusion/proto/src/physical_plan/to_proto.rs
yyy1000 Mar 19, 2024
1dfce4a
Update datafusion/proto/tests/cases/roundtrip_physical_plan.rs
yyy1000 Mar 19, 2024
767b53d
address the comment
yyy1000 Mar 19, 2024
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
9 changes: 5 additions & 4 deletions datafusion/physical-expr/src/scalar_function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,12 @@ use arrow::record_batch::RecordBatch;
use datafusion_common::Result;
use datafusion_expr::{
expr_vec_fmt, BuiltinScalarFunction, ColumnarValue, FuncMonotonicity,
ScalarFunctionImplementation,
ScalarFunctionDefinition, ScalarFunctionImplementation,
};

/// Physical expression of a scalar function
pub struct ScalarFunctionExpr {
fun: ScalarFunctionImplementation,
fun: ScalarFunctionDefinition,
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 think the fun here can be a ScalarFunctionDefinition to get the ScalarUDF from ScalarFunctionExpr?
Also I searched the code base and found ScalarFunctionImplementation is only used for:

  1. BuiltinScalarFunction
  2. method like create_udf which I think will be deprecated in the future given the new ScalarUDFImpl way.
    So I think ScalarFunctionImplementation will also be deleted in a day. 🤔 WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, this should be ScalarFunctionDefinition here. I don't think create_udf will be deprecated but we have ScalarUdfLegacyWrapper to wrap ScalarFunctionImplementation and create a ScalarUDFImpl out of it. So the new ScalarFunctionDefinition would include anything that is represented as a ScalarFunctionImplementation

name: String,
args: Vec<Arc<dyn PhysicalExpr>>,
return_type: DataType,
Expand Down Expand Up @@ -79,7 +79,7 @@ impl ScalarFunctionExpr {
/// Create a new Scalar function
pub fn new(
name: &str,
fun: ScalarFunctionImplementation,
fun: ScalarFunctionDefinition,
args: Vec<Arc<dyn PhysicalExpr>>,
return_type: DataType,
monotonicity: Option<FuncMonotonicity>,
Expand All @@ -97,7 +97,8 @@ impl ScalarFunctionExpr {

/// Get the scalar function implementation
pub fn fun(&self) -> &ScalarFunctionImplementation {
&self.fun
//
todo!()
}

/// The name for this expression
Expand Down
Loading
Loading