Re: [PR] Fixes 3 bugs during serialization and deserialization of physical plans [datafusion]
alamb commented on PR #16858: URL: https://github.com/apache/datafusion/pull/16858#issuecomment-3118107079 Thanks again everyone! -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Fixes 3 bugs during serialization and deserialization of physical plans [datafusion]
alamb merged PR #16858: URL: https://github.com/apache/datafusion/pull/16858 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Fixes 3 bugs during serialization and deserialization of physical plans [datafusion]
NGA-TRAN commented on code in PR #16858:
URL: https://github.com/apache/datafusion/pull/16858#discussion_r2229483527
##
datafusion/core/src/physical_planner.rs:
##
@@ -1358,6 +1358,9 @@ impl DefaultPhysicalPlanner {
physical_name(expr),
))?])),
}
+} else if group_expr.is_empty() {
+// No GROUP BY clause - create empty PhysicalGroupBy
+Ok(PhysicalGroupBy::new(vec![], vec![], vec![]))
Review Comment:
I did, and those were the results I got. It even flagged a semantic error
due to not distinguishing between no group and an empty one. Coding without AI
these days feels almost impossible. 😄
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Fixes 3 bugs during serialization and deserialization of physical plans [datafusion]
alamb commented on code in PR #16858:
URL: https://github.com/apache/datafusion/pull/16858#discussion_r2229471200
##
datafusion/core/src/physical_planner.rs:
##
@@ -1358,6 +1358,9 @@ impl DefaultPhysicalPlanner {
physical_name(expr),
))?])),
}
+} else if group_expr.is_empty() {
+// No GROUP BY clause - create empty PhysicalGroupBy
+Ok(PhysicalGroupBy::new(vec![], vec![], vec![]))
Review Comment:
Maybe something we can ask an AI agent to try in a follow on PR
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Fixes 3 bugs during serialization and deserialization of physical plans [datafusion]
NGA-TRAN commented on code in PR #16858:
URL: https://github.com/apache/datafusion/pull/16858#discussion_r2229020034
##
datafusion/core/src/physical_planner.rs:
##
@@ -1358,6 +1358,9 @@ impl DefaultPhysicalPlanner {
physical_name(expr),
))?])),
}
+} else if group_expr.is_empty() {
+// No GROUP BY clause - create empty PhysicalGroupBy
+Ok(PhysicalGroupBy::new(vec![], vec![], vec![]))
Review Comment:
@alamb : I had a look. This would be very challenging and a lot of changes
needed in physical plan, logical plan, parsing logic,
serialization/deserialization. I do not think we want to make a big change just
for 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Fixes 3 bugs during serialization and deserialization of physical plans [datafusion]
NGA-TRAN commented on code in PR #16858:
URL: https://github.com/apache/datafusion/pull/16858#discussion_r2229020034
##
datafusion/core/src/physical_planner.rs:
##
@@ -1358,6 +1358,9 @@ impl DefaultPhysicalPlanner {
physical_name(expr),
))?])),
}
+} else if group_expr.is_empty() {
+// No GROUP BY clause - create empty PhysicalGroupBy
+Ok(PhysicalGroupBy::new(vec![], vec![], vec![]))
Review Comment:
@alamb : I have a looked. This would be very challenging and a lot of
changes needed in physical plan, logical plan, parsing logic,
serialization/deserialization. I do not think we want to make a big change just
for 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Fixes 3 bugs during serialization and deserialization of physical plans [datafusion]
NGA-TRAN commented on code in PR #16858:
URL: https://github.com/apache/datafusion/pull/16858#discussion_r2228968322
##
datafusion/core/src/physical_planner.rs:
##
@@ -1358,6 +1358,9 @@ impl DefaultPhysicalPlanner {
physical_name(expr),
))?])),
}
+} else if group_expr.is_empty() {
+// No GROUP BY clause - create empty PhysicalGroupBy
+Ok(PhysicalGroupBy::new(vec![], vec![], vec![]))
Review Comment:
Let me try
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Fixes 3 bugs during serialization and deserialization of physical plans [datafusion]
alamb commented on code in PR #16858:
URL: https://github.com/apache/datafusion/pull/16858#discussion_r2228760094
##
datafusion/core/src/physical_planner.rs:
##
@@ -1358,6 +1358,9 @@ impl DefaultPhysicalPlanner {
physical_name(expr),
))?])),
}
+} else if group_expr.is_empty() {
+// No GROUP BY clause - create empty PhysicalGroupBy
+Ok(PhysicalGroupBy::new(vec![], vec![], vec![]))
Review Comment:
I wonder if we should look into changing `group_expr`: Option<&[Expr]>` or
something so the compiler makes it more likely that we handle the difference
between an empty `Vec` vs `None`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Fixes 3 bugs during serialization and deserialization of physical plans [datafusion]
NGA-TRAN commented on PR #16858: URL: https://github.com/apache/datafusion/pull/16858#issuecomment-3113736222 @alamb : When you have a moment, could you take a quick look? Thanks! -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Fixes 3 bugs during serialization and deserialization of physical plans [datafusion]
LiaCastaneda commented on code in PR #16858:
URL: https://github.com/apache/datafusion/pull/16858#discussion_r2226105947
##
datafusion/proto/src/physical_plan/from_proto.rs:
##
@@ -368,7 +368,12 @@ pub fn parse_physical_expr(
e.name.as_str(),
scalar_fun_def,
args,
-Field::new("f", convert_required!(e.return_type)?,
true).into(),
+Field::new(
+&e.return_field_name,
+convert_required!(e.return_type)?,
+true,
Review Comment:
I was refering to the nullability of the return `Field`, that is always
being set to `true` (even before this PR) i was wondering if this could be an
issue later if the output of the scalar function could contain nulls, but is
unrelated to the changes & fixes of this PR, sorry for the confusion 😄
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Fixes 3 bugs during serialization and deserialization of physical plans [datafusion]
NGA-TRAN commented on code in PR #16858:
URL: https://github.com/apache/datafusion/pull/16858#discussion_r2225941942
##
datafusion/proto/src/physical_plan/from_proto.rs:
##
@@ -368,7 +368,12 @@ pub fn parse_physical_expr(
e.name.as_str(),
scalar_fun_def,
args,
-Field::new("f", convert_required!(e.return_type)?,
true).into(),
+Field::new(
+&e.return_field_name,
+convert_required!(e.return_type)?,
+true,
Review Comment:
The change here is to use `e.return_field_name` in deserialization. So
whatever name of the UDF we serialize at line 356 of the file `to_proto.rs in`
this PR, it will be deserialized here; and it will match. Does this answer
your concern?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Fixes 3 bugs during serialization and deserialization of physical plans [datafusion]
LiaCastaneda commented on code in PR #16858:
URL: https://github.com/apache/datafusion/pull/16858#discussion_r2224790385
##
datafusion/proto/src/physical_plan/from_proto.rs:
##
@@ -368,7 +368,12 @@ pub fn parse_physical_expr(
e.name.as_str(),
scalar_fun_def,
args,
-Field::new("f", convert_required!(e.return_type)?,
true).into(),
+Field::new(
+&e.return_field_name,
+convert_required!(e.return_type)?,
+true,
Review Comment:
I wonder if this field could ever be non nullable, for example for udfs
defined by DF users, then the deserialization would result in a mismatch (not
in the scope of this fixes though)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Fixes 3 bugs during serialization and deserialization of physical plans [datafusion]
NGA-TRAN commented on code in PR #16858:
URL: https://github.com/apache/datafusion/pull/16858#discussion_r2223808490
##
datafusion/core/src/physical_planner.rs:
##
@@ -1358,6 +1358,9 @@ impl DefaultPhysicalPlanner {
physical_name(expr),
))?])),
}
+} else if group_expr.is_empty() {
+// No GROUP BY clause - create empty PhysicalGroupBy
+Ok(PhysicalGroupBy::new(vec![], vec![], vec![]))
Review Comment:
One of the places that fixes bug 2 that should return nothing for
non-groupby
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Fixes 3 bugs during serialization and deserialization of physical plans [datafusion]
NGA-TRAN commented on code in PR #16858:
URL: https://github.com/apache/datafusion/pull/16858#discussion_r2223805868
##
datafusion/proto/proto/datafusion.proto:
##
@@ -859,6 +859,7 @@ message PhysicalScalarUdfNode {
optional bytes fun_definition = 3;
datafusion_common.ArrowType return_type = 4;
bool nullable = 5;
+ string return_field_name = 6;
Review Comment:
Fix for bug 3 that does not serialize ScarlarFunction name
##
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##
@@ -1819,10 +1823,7 @@ async fn test_serialize_deserialize_tpch_queries() ->
Result<()> {
Ok(())
}
-// bug: https://github.com/apache/datafusion/issues/16772
-// Only 4 queries pass: q3, q5, q10, q12
-// Ignore the test until the bug is fixed
-#[ignore]
+// Bugs: https://github.com/apache/datafusion/issues/16772
Review Comment:
Turn on all 22 tpc-h queries
##
datafusion/proto/proto/datafusion.proto:
##
@@ -870,6 +871,7 @@ message PhysicalAggregateExprNode {
bool distinct = 3;
bool ignore_nulls = 6;
optional bytes fun_definition = 7;
+ string human_display = 8;
Review Comment:
Fix for bug 1 that does not serialize human_display for aggregate function
##
datafusion/core/src/physical_planner.rs:
##
@@ -1358,6 +1358,9 @@ impl DefaultPhysicalPlanner {
physical_name(expr),
))?])),
}
+} else if group_expr.is_empty() {
+// No GROUP BY clause - create empty PhysicalGroupBy
+Ok(PhysicalGroupBy::new(vec![], vec![], vec![]))
Review Comment:
One of the place that fixes bug 2 that should return nothing for non-groupby
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
