-
Notifications
You must be signed in to change notification settings - Fork 1.9k
replace HashTableLookupExpr with lit(true) in proto serialization #19300
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,7 @@ use datafusion_physical_plan::expressions::{ | |
| BinaryExpr, CaseExpr, CastExpr, Column, InListExpr, IsNotNullExpr, IsNullExpr, | ||
| Literal, NegativeExpr, NotExpr, TryCastExpr, UnKnownColumn, | ||
| }; | ||
| use datafusion_physical_plan::joins::HashTableLookupExpr; | ||
| use datafusion_physical_plan::udaf::AggregateFunctionExpr; | ||
| use datafusion_physical_plan::windows::{PlainAggregateWindowExpr, WindowUDFExpr}; | ||
| use datafusion_physical_plan::{Partitioning, PhysicalExpr, WindowExpr}; | ||
|
|
@@ -227,6 +228,30 @@ pub fn serialize_physical_expr( | |
| let value = snapshot_physical_expr(Arc::clone(value))?; | ||
| let expr = value.as_any(); | ||
|
|
||
| // HashTableLookupExpr is used for dynamic filter pushdown in hash joins. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it would be nice if we could move the protobuf serialization logic into the PhysicalExpr trait itself so we don't forget new structures like this However, given this just follows the existing pattern, i think it looks good to me
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I agree |
||
| // It contains an Arc<dyn JoinHashMapType> (the build-side hash table) which | ||
| // cannot be serialized - the hash table is a runtime structure built during | ||
| // execution on the build side. | ||
| // | ||
| // We replace it with lit(true) which is safe because: | ||
| // 1. The filter is a performance optimization, not a correctness requirement | ||
| // 2. lit(true) passes all rows, so no valid rows are incorrectly filtered out | ||
| // 3. The join itself will still produce correct results, just without the | ||
| // benefit of early filtering on the probe side | ||
| // | ||
| // In distributed execution, the remote worker won't have access to the hash | ||
| // table anyway, so the best we can do is skip this optimization. | ||
| if expr.downcast_ref::<HashTableLookupExpr>().is_some() { | ||
| let value = datafusion_proto_common::ScalarValue { | ||
| value: Some(datafusion_proto_common::scalar_value::Value::BoolValue( | ||
| true, | ||
| )), | ||
| }; | ||
| return Ok(protobuf::PhysicalExprNode { | ||
| expr_type: Some(protobuf::physical_expr_node::ExprType::Literal(value)), | ||
| }); | ||
| } | ||
|
|
||
| if let Some(expr) = expr.downcast_ref::<Column>() { | ||
| Ok(protobuf::PhysicalExprNode { | ||
| expr_type: Some(protobuf::physical_expr_node::ExprType::Column( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍