You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Adez017 (via GitHub)" <gi...@apache.org> on 2026/04/28 06:14:12 UTC
[PR] fix(proto): correctly serialize FilterExec empty projection [datafusion]
Adez017 opened a new pull request, #21885:
URL: https://github.com/apache/datafusion/pull/21885
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes #21873
## Rationale for this change
FilterExec supports two semantically different projection states:
- None → return all columns (full projection)
- Some(vec![]) → return no columns (empty projection)
However, both cases were being serialized identically as an empty vector in the proto representation. During deserialization, an empty vector was always mapped back to None, meaning an empty projection would silently become a full projection after a serde round-trip.
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
No
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): correctly serialize FilterExec empty projection [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #21885:
URL: https://github.com/apache/datafusion/pull/21885#discussion_r3154642834
##########
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -3681,3 +3681,63 @@ async fn roundtrip_issue_18602_complex_filter_decode_recursion() -> Result<()> {
roundtrip_test_sql_with_context(sql, &ctx).await
}
+
+#[tokio::test]
+async fn test_filter_exec_projection_serde_roundtrip() -> Result<()> {
+ let ctx = SessionContext::new();
+ let codec = DefaultPhysicalExtensionCodec {};
+
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("a", DataType::Int32, false),
+ Field::new("b", DataType::Int32, false),
+ Field::new("c", DataType::Int32, false),
+ ]));
+
+ let input: Arc<dyn ExecutionPlan> = Arc::new(EmptyExec::new(Arc::clone(&schema)));
+
+ let predicate: Arc<dyn PhysicalExpr> = Arc::new(BinaryExpr::new(
+ Arc::new(Column::new("a", 0)),
+ Operator::Gt,
+ Arc::new(Literal::new(ScalarValue::Int32(Some(0)))),
+ ));
+
+ // Case 1: None -> should round-trip as None (return all columns)
+ let filter =
+ FilterExecBuilder::new(Arc::clone(&predicate), Arc::clone(&input)).build()?;
+ let proto = PhysicalPlanNode::try_from_physical_plan(Arc::new(filter) as _, &codec)?;
+ let roundtripped = proto.try_into_physical_plan(ctx.task_ctx().as_ref(), &codec)?;
+ let rt = roundtripped.as_ref().downcast_ref::<FilterExec>().unwrap();
+ assert_eq!(
+ rt.projection().as_deref(),
+ None,
+ "None projection must stay None after roundtrip"
+ );
+
Review Comment:
My comment is suggesting we use `roundtrip_test()` function to streamline these tests. See the code snippet I linked
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): correctly serialize FilterExec empty projection [datafusion]
Posted by "askalt (via GitHub)" <gi...@apache.org>.
askalt commented on PR #21885:
URL: https://github.com/apache/datafusion/pull/21885#issuecomment-4333365555
@Adez017 Thank you for the patch!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): correctly serialize FilterExec empty projection [datafusion]
Posted by "Adez017 (via GitHub)" <gi...@apache.org>.
Adez017 commented on code in PR #21885:
URL: https://github.com/apache/datafusion/pull/21885#discussion_r3154489403
##########
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -3681,3 +3681,63 @@ async fn roundtrip_issue_18602_complex_filter_decode_recursion() -> Result<()> {
roundtrip_test_sql_with_context(sql, &ctx).await
}
+
+#[tokio::test]
+async fn test_filter_exec_projection_serde_roundtrip() -> Result<()> {
+ let ctx = SessionContext::new();
+ let codec = DefaultPhysicalExtensionCodec {};
+
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("a", DataType::Int32, false),
+ Field::new("b", DataType::Int32, false),
+ Field::new("c", DataType::Int32, false),
+ ]));
+
+ let input: Arc<dyn ExecutionPlan> = Arc::new(EmptyExec::new(Arc::clone(&schema)));
+
+ let predicate: Arc<dyn PhysicalExpr> = Arc::new(BinaryExpr::new(
+ Arc::new(Column::new("a", 0)),
+ Operator::Gt,
+ Arc::new(Literal::new(ScalarValue::Int32(Some(0)))),
+ ));
+
+ // Case 1: None -> should round-trip as None (return all columns)
+ let filter =
+ FilterExecBuilder::new(Arc::clone(&predicate), Arc::clone(&input)).build()?;
+ let proto = PhysicalPlanNode::try_from_physical_plan(Arc::new(filter) as _, &codec)?;
+ let roundtripped = proto.try_into_physical_plan(ctx.task_ctx().as_ref(), &codec)?;
+ let rt = roundtripped.as_ref().downcast_ref::<FilterExec>().unwrap();
+ assert_eq!(
+ rt.projection().as_deref(),
+ None,
+ "None projection must stay None after roundtrip"
+ );
+
Review Comment:
absolutely , i just added a different test to use it in a better way .
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): correctly serialize FilterExec empty projection [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #21885:
URL: https://github.com/apache/datafusion/pull/21885#issuecomment-4358015378
Thanks @Adez017 & @askalt
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): correctly serialize FilterExec empty projection [datafusion]
Posted by "Adez017 (via GitHub)" <gi...@apache.org>.
Adez017 commented on code in PR #21885:
URL: https://github.com/apache/datafusion/pull/21885#discussion_r3154618983
##########
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -3681,3 +3681,63 @@ async fn roundtrip_issue_18602_complex_filter_decode_recursion() -> Result<()> {
roundtrip_test_sql_with_context(sql, &ctx).await
}
+
+#[tokio::test]
+async fn test_filter_exec_projection_serde_roundtrip() -> Result<()> {
+ let ctx = SessionContext::new();
+ let codec = DefaultPhysicalExtensionCodec {};
+
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("a", DataType::Int32, false),
+ Field::new("b", DataType::Int32, false),
+ Field::new("c", DataType::Int32, false),
+ ]));
+
+ let input: Arc<dyn ExecutionPlan> = Arc::new(EmptyExec::new(Arc::clone(&schema)));
+
+ let predicate: Arc<dyn PhysicalExpr> = Arc::new(BinaryExpr::new(
+ Arc::new(Column::new("a", 0)),
+ Operator::Gt,
+ Arc::new(Literal::new(ScalarValue::Int32(Some(0)))),
+ ));
+
+ // Case 1: None -> should round-trip as None (return all columns)
+ let filter =
+ FilterExecBuilder::new(Arc::clone(&predicate), Arc::clone(&input)).build()?;
+ let proto = PhysicalPlanNode::try_from_physical_plan(Arc::new(filter) as _, &codec)?;
+ let roundtripped = proto.try_into_physical_plan(ctx.task_ctx().as_ref(), &codec)?;
+ let rt = roundtripped.as_ref().downcast_ref::<FilterExec>().unwrap();
+ assert_eq!(
+ rt.projection().as_deref(),
+ None,
+ "None projection must stay None after roundtrip"
+ );
+
Review Comment:
The issue occurs because when a `FilterExec` has an empty projection, the previous proto serialization didn't explicitly encode the 'empty' state. This caused the physical plan to default back to a full projection or an invalid state upon deserialization. By explicitly handling the projection field even when empty, we ensure that the execution plan remains consistent across the network/serde boundary, which is critical for count-only queries.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): correctly serialize FilterExec empty projection [datafusion]
Posted by "Adez017 (via GitHub)" <gi...@apache.org>.
Adez017 commented on PR #21885:
URL: https://github.com/apache/datafusion/pull/21885#issuecomment-4333112084
@comphead
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): correctly serialize FilterExec empty projection [datafusion]
Posted by "Adez017 (via GitHub)" <gi...@apache.org>.
Adez017 commented on code in PR #21885:
URL: https://github.com/apache/datafusion/pull/21885#discussion_r3154723632
##########
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -3681,3 +3681,63 @@ async fn roundtrip_issue_18602_complex_filter_decode_recursion() -> Result<()> {
roundtrip_test_sql_with_context(sql, &ctx).await
}
+
+#[tokio::test]
+async fn test_filter_exec_projection_serde_roundtrip() -> Result<()> {
+ let ctx = SessionContext::new();
+ let codec = DefaultPhysicalExtensionCodec {};
+
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("a", DataType::Int32, false),
+ Field::new("b", DataType::Int32, false),
+ Field::new("c", DataType::Int32, false),
+ ]));
+
+ let input: Arc<dyn ExecutionPlan> = Arc::new(EmptyExec::new(Arc::clone(&schema)));
+
+ let predicate: Arc<dyn PhysicalExpr> = Arc::new(BinaryExpr::new(
+ Arc::new(Column::new("a", 0)),
+ Operator::Gt,
+ Arc::new(Literal::new(ScalarValue::Int32(Some(0)))),
+ ));
+
+ // Case 1: None -> should round-trip as None (return all columns)
+ let filter =
+ FilterExecBuilder::new(Arc::clone(&predicate), Arc::clone(&input)).build()?;
+ let proto = PhysicalPlanNode::try_from_physical_plan(Arc::new(filter) as _, &codec)?;
+ let roundtripped = proto.try_into_physical_plan(ctx.task_ctx().as_ref(), &codec)?;
+ let rt = roundtripped.as_ref().downcast_ref::<FilterExec>().unwrap();
+ assert_eq!(
+ rt.projection().as_deref(),
+ None,
+ "None projection must stay None after roundtrip"
+ );
+
Review Comment:
> My comment is suggesting we use `roundtrip_test()` function to streamline these tests. See the code snippet I linked
should i remove the additional test i had added ? @Jefffrey
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): correctly serialize FilterExec empty projection [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #21885:
URL: https://github.com/apache/datafusion/pull/21885#discussion_r3154578073
##########
datafusion/proto/src/physical_plan/mod.rs:
##########
@@ -716,18 +716,21 @@ impl protobuf::PhysicalPlanNode {
})?;
let filter_selectivity = filter.default_filter_selectivity.try_into();
- let projection = if !filter.projection.is_empty() {
- Some(
- filter
- .projection
- .iter()
- .map(|i| *i as usize)
- .collect::<Vec<_>>(),
- )
- } else {
+ // Determine if the projection is full to optimize used memory,
+ // storing `None` in this case.
Review Comment:
I feel we should focus more on the fact this logic is trying to preserve a `None` across proto boundaries because of the limitations we face (with how its encoded the same as `Some(vec![])`) instead of explaining it only as a memory efficiency gain
##########
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -3681,3 +3681,63 @@ async fn roundtrip_issue_18602_complex_filter_decode_recursion() -> Result<()> {
roundtrip_test_sql_with_context(sql, &ctx).await
}
+
+#[tokio::test]
+async fn test_filter_exec_projection_serde_roundtrip() -> Result<()> {
+ let ctx = SessionContext::new();
+ let codec = DefaultPhysicalExtensionCodec {};
+
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("a", DataType::Int32, false),
+ Field::new("b", DataType::Int32, false),
+ Field::new("c", DataType::Int32, false),
+ ]));
+
+ let input: Arc<dyn ExecutionPlan> = Arc::new(EmptyExec::new(Arc::clone(&schema)));
+
+ let predicate: Arc<dyn PhysicalExpr> = Arc::new(BinaryExpr::new(
+ Arc::new(Column::new("a", 0)),
+ Operator::Gt,
+ Arc::new(Literal::new(ScalarValue::Int32(Some(0)))),
+ ));
+
+ // Case 1: None -> should round-trip as None (return all columns)
+ let filter =
+ FilterExecBuilder::new(Arc::clone(&predicate), Arc::clone(&input)).build()?;
+ let proto = PhysicalPlanNode::try_from_physical_plan(Arc::new(filter) as _, &codec)?;
+ let roundtripped = proto.try_into_physical_plan(ctx.task_ctx().as_ref(), &codec)?;
+ let rt = roundtripped.as_ref().downcast_ref::<FilterExec>().unwrap();
+ assert_eq!(
+ rt.projection().as_deref(),
+ None,
+ "None projection must stay None after roundtrip"
+ );
+
Review Comment:
What does this mean?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): correctly serialize FilterExec empty projection [datafusion]
Posted by "Adez017 (via GitHub)" <gi...@apache.org>.
Adez017 commented on PR #21885:
URL: https://github.com/apache/datafusion/pull/21885#issuecomment-4341481403
Hi @Jefffrey , i had pushed the changes as per comments , take a look now .
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): correctly serialize FilterExec empty projection [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #21885:
URL: https://github.com/apache/datafusion/pull/21885#discussion_r3154417520
##########
datafusion/proto/src/physical_plan/mod.rs:
##########
@@ -716,18 +716,21 @@ impl protobuf::PhysicalPlanNode {
})?;
let filter_selectivity = filter.default_filter_selectivity.try_into();
- let projection = if !filter.projection.is_empty() {
- Some(
- filter
- .projection
- .iter()
- .map(|i| *i as usize)
- .collect::<Vec<_>>(),
- )
- } else {
+ // Determine if the projection is full to optimize used memory,
+ // storing `None` in this case.
Review Comment:
"optimize used memory" seems the wrong intention here? Isn't it more to try to accurately recreate a `None` state which the proto definition can't encode?
##########
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -3681,3 +3681,63 @@ async fn roundtrip_issue_18602_complex_filter_decode_recursion() -> Result<()> {
roundtrip_test_sql_with_context(sql, &ctx).await
}
+
+#[tokio::test]
+async fn test_filter_exec_projection_serde_roundtrip() -> Result<()> {
+ let ctx = SessionContext::new();
+ let codec = DefaultPhysicalExtensionCodec {};
+
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("a", DataType::Int32, false),
+ Field::new("b", DataType::Int32, false),
+ Field::new("c", DataType::Int32, false),
+ ]));
+
+ let input: Arc<dyn ExecutionPlan> = Arc::new(EmptyExec::new(Arc::clone(&schema)));
+
+ let predicate: Arc<dyn PhysicalExpr> = Arc::new(BinaryExpr::new(
+ Arc::new(Column::new("a", 0)),
+ Operator::Gt,
+ Arc::new(Literal::new(ScalarValue::Int32(Some(0)))),
+ ));
+
+ // Case 1: None -> should round-trip as None (return all columns)
+ let filter =
+ FilterExecBuilder::new(Arc::clone(&predicate), Arc::clone(&input)).build()?;
+ let proto = PhysicalPlanNode::try_from_physical_plan(Arc::new(filter) as _, &codec)?;
+ let roundtripped = proto.try_into_physical_plan(ctx.task_ctx().as_ref(), &codec)?;
+ let rt = roundtripped.as_ref().downcast_ref::<FilterExec>().unwrap();
+ assert_eq!(
+ rt.projection().as_deref(),
+ None,
+ "None projection must stay None after roundtrip"
+ );
+
Review Comment:
I feel like we can use `roundtrip_test` here as others do above, e.g.
https://github.com/apache/datafusion/blob/22bb4e6b752c7a62b677d94a63bcf08b68e8d5ec/datafusion/proto/tests/cases/roundtrip_physical_plan.rs#L3504-L3507
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): correctly serialize FilterExec empty projection [datafusion]
Posted by "askalt (via GitHub)" <gi...@apache.org>.
askalt commented on code in PR #21885:
URL: https://github.com/apache/datafusion/pull/21885#discussion_r3154495296
##########
datafusion/proto/src/physical_plan/mod.rs:
##########
@@ -716,18 +716,21 @@ impl protobuf::PhysicalPlanNode {
})?;
let filter_selectivity = filter.default_filter_selectivity.try_into();
- let projection = if !filter.projection.is_empty() {
- Some(
- filter
- .projection
- .iter()
- .map(|i| *i as usize)
- .collect::<Vec<_>>(),
- )
- } else {
+ // Determine if the projection is full to optimize used memory,
+ // storing `None` in this case.
Review Comment:
We can store `Some(vec![0,1,2...n-1])` to represent a full projection, so I thought to highlight why we focus on `None` usage.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): correctly serialize FilterExec empty projection [datafusion]
Posted by "Adez017 (via GitHub)" <gi...@apache.org>.
Adez017 commented on code in PR #21885:
URL: https://github.com/apache/datafusion/pull/21885#discussion_r3154492736
##########
datafusion/proto/src/physical_plan/mod.rs:
##########
@@ -716,18 +716,21 @@ impl protobuf::PhysicalPlanNode {
})?;
let filter_selectivity = filter.default_filter_selectivity.try_into();
- let projection = if !filter.projection.is_empty() {
- Some(
- filter
- .projection
- .iter()
- .map(|i| *i as usize)
- .collect::<Vec<_>>(),
- )
- } else {
+ // Determine if the projection is full to optimize used memory,
+ // storing `None` in this case.
Review Comment:
Seems valid!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): correctly serialize FilterExec empty projection [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #21885:
URL: https://github.com/apache/datafusion/pull/21885#discussion_r3154759070
##########
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -3681,3 +3681,63 @@ async fn roundtrip_issue_18602_complex_filter_decode_recursion() -> Result<()> {
roundtrip_test_sql_with_context(sql, &ctx).await
}
+
+#[tokio::test]
+async fn test_filter_exec_projection_serde_roundtrip() -> Result<()> {
+ let ctx = SessionContext::new();
+ let codec = DefaultPhysicalExtensionCodec {};
+
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("a", DataType::Int32, false),
+ Field::new("b", DataType::Int32, false),
+ Field::new("c", DataType::Int32, false),
+ ]));
+
+ let input: Arc<dyn ExecutionPlan> = Arc::new(EmptyExec::new(Arc::clone(&schema)));
+
+ let predicate: Arc<dyn PhysicalExpr> = Arc::new(BinaryExpr::new(
+ Arc::new(Column::new("a", 0)),
+ Operator::Gt,
+ Arc::new(Literal::new(ScalarValue::Int32(Some(0)))),
+ ));
+
+ // Case 1: None -> should round-trip as None (return all columns)
+ let filter =
+ FilterExecBuilder::new(Arc::clone(&predicate), Arc::clone(&input)).build()?;
+ let proto = PhysicalPlanNode::try_from_physical_plan(Arc::new(filter) as _, &codec)?;
+ let roundtripped = proto.try_into_physical_plan(ctx.task_ctx().as_ref(), &codec)?;
+ let rt = roundtripped.as_ref().downcast_ref::<FilterExec>().unwrap();
+ assert_eq!(
+ rt.projection().as_deref(),
+ None,
+ "None projection must stay None after roundtrip"
+ );
+
Review Comment:
No I am saying we should refactor these tests to use `roundtrip_test()` instead of manually asserting a specific property of the roundtripped struct
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): correctly serialize FilterExec empty projection [datafusion]
Posted by "Adez017 (via GitHub)" <gi...@apache.org>.
Adez017 commented on code in PR #21885:
URL: https://github.com/apache/datafusion/pull/21885#discussion_r3152446884
##########
datafusion/proto/src/physical_plan/mod.rs:
##########
@@ -716,18 +716,19 @@ impl protobuf::PhysicalPlanNode {
})?;
let filter_selectivity = filter.default_filter_selectivity.try_into();
- let projection = if !filter.projection.is_empty() {
- Some(
- filter
- .projection
- .iter()
- .map(|i| *i as usize)
- .collect::<Vec<_>>(),
- )
- } else {
+ let num_fields = input.schema().fields().len();
Review Comment:
sure @askalt
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): correctly serialize FilterExec empty projection [datafusion]
Posted by "Adez017 (via GitHub)" <gi...@apache.org>.
Adez017 commented on code in PR #21885:
URL: https://github.com/apache/datafusion/pull/21885#discussion_r3154858824
##########
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -3681,3 +3681,63 @@ async fn roundtrip_issue_18602_complex_filter_decode_recursion() -> Result<()> {
roundtrip_test_sql_with_context(sql, &ctx).await
}
+
+#[tokio::test]
+async fn test_filter_exec_projection_serde_roundtrip() -> Result<()> {
+ let ctx = SessionContext::new();
+ let codec = DefaultPhysicalExtensionCodec {};
+
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("a", DataType::Int32, false),
+ Field::new("b", DataType::Int32, false),
+ Field::new("c", DataType::Int32, false),
+ ]));
+
+ let input: Arc<dyn ExecutionPlan> = Arc::new(EmptyExec::new(Arc::clone(&schema)));
+
+ let predicate: Arc<dyn PhysicalExpr> = Arc::new(BinaryExpr::new(
+ Arc::new(Column::new("a", 0)),
+ Operator::Gt,
+ Arc::new(Literal::new(ScalarValue::Int32(Some(0)))),
+ ));
+
+ // Case 1: None -> should round-trip as None (return all columns)
+ let filter =
+ FilterExecBuilder::new(Arc::clone(&predicate), Arc::clone(&input)).build()?;
+ let proto = PhysicalPlanNode::try_from_physical_plan(Arc::new(filter) as _, &codec)?;
+ let roundtripped = proto.try_into_physical_plan(ctx.task_ctx().as_ref(), &codec)?;
+ let rt = roundtripped.as_ref().downcast_ref::<FilterExec>().unwrap();
+ assert_eq!(
+ rt.projection().as_deref(),
+ None,
+ "None projection must stay None after roundtrip"
+ );
+
Review Comment:
> No I am saying we should refactor these tests to use `roundtrip_test()` instead of manually asserting a specific property of the roundtripped struct
sounds good . i'll try producing it
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): correctly serialize FilterExec empty projection [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #21885:
URL: https://github.com/apache/datafusion/pull/21885
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): correctly serialize FilterExec empty projection [datafusion]
Posted by "Adez017 (via GitHub)" <gi...@apache.org>.
Adez017 commented on PR #21885:
URL: https://github.com/apache/datafusion/pull/21885#issuecomment-4352499835
@alamb @comphead any updates on this ?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): correctly serialize FilterExec empty projection [datafusion]
Posted by "Adez017 (via GitHub)" <gi...@apache.org>.
Adez017 commented on code in PR #21885:
URL: https://github.com/apache/datafusion/pull/21885#discussion_r3154618983
##########
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -3681,3 +3681,63 @@ async fn roundtrip_issue_18602_complex_filter_decode_recursion() -> Result<()> {
roundtrip_test_sql_with_context(sql, &ctx).await
}
+
+#[tokio::test]
+async fn test_filter_exec_projection_serde_roundtrip() -> Result<()> {
+ let ctx = SessionContext::new();
+ let codec = DefaultPhysicalExtensionCodec {};
+
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("a", DataType::Int32, false),
+ Field::new("b", DataType::Int32, false),
+ Field::new("c", DataType::Int32, false),
+ ]));
+
+ let input: Arc<dyn ExecutionPlan> = Arc::new(EmptyExec::new(Arc::clone(&schema)));
+
+ let predicate: Arc<dyn PhysicalExpr> = Arc::new(BinaryExpr::new(
+ Arc::new(Column::new("a", 0)),
+ Operator::Gt,
+ Arc::new(Literal::new(ScalarValue::Int32(Some(0)))),
+ ));
+
+ // Case 1: None -> should round-trip as None (return all columns)
+ let filter =
+ FilterExecBuilder::new(Arc::clone(&predicate), Arc::clone(&input)).build()?;
+ let proto = PhysicalPlanNode::try_from_physical_plan(Arc::new(filter) as _, &codec)?;
+ let roundtripped = proto.try_into_physical_plan(ctx.task_ctx().as_ref(), &codec)?;
+ let rt = roundtripped.as_ref().downcast_ref::<FilterExec>().unwrap();
+ assert_eq!(
+ rt.projection().as_deref(),
+ None,
+ "None projection must stay None after roundtrip"
+ );
+
Review Comment:
The issue specifically mention to add test so i did , we can keep or remove totally depend on usecase
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): correctly serialize FilterExec empty projection [datafusion]
Posted by "askalt (via GitHub)" <gi...@apache.org>.
askalt commented on code in PR #21885:
URL: https://github.com/apache/datafusion/pull/21885#discussion_r3152424272
##########
datafusion/proto/src/physical_plan/mod.rs:
##########
@@ -716,18 +716,19 @@ impl protobuf::PhysicalPlanNode {
})?;
let filter_selectivity = filter.default_filter_selectivity.try_into();
- let projection = if !filter.projection.is_empty() {
- Some(
- filter
- .projection
- .iter()
- .map(|i| *i as usize)
- .collect::<Vec<_>>(),
- )
- } else {
+ let num_fields = input.schema().fields().len();
Review Comment:
Let add a comment explaining this code, something like:
```rust
// Determine if the projection is full to optimize used memory,
// storing `None` in this case.
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(proto): correctly serialize FilterExec empty projection [datafusion]
Posted by "Adez017 (via GitHub)" <gi...@apache.org>.
Adez017 commented on PR #21885:
URL: https://github.com/apache/datafusion/pull/21885#issuecomment-4332872241
@askalt , can you look into this ?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org