Copilot commented on code in PR #23130:
URL: https://github.com/apache/datafusion/pull/23130#discussion_r3551973735
##########
datafusion/physical-expr/src/scalar_subquery.rs:
##########
@@ -59,19 +59,6 @@ impl ScalarSubqueryExpr {
}
}
- pub fn data_type(&self) -> &DataType {
- &self.data_type
- }
-
- pub fn nullable(&self) -> bool {
- self.nullable
- }
-
- /// Returns the index of this subquery in the shared results container.
- pub fn index(&self) -> SubqueryIndex {
- self.index
- }
-
pub fn results(&self) -> &ScalarSubqueryResults {
&self.results
}
Review Comment:
`ScalarSubqueryExpr` previously exposed `data_type()`, `nullable()`, and
`index()` as public accessors. Removing them is a breaking change to the
`datafusion-physical-expr` public API and seems to contradict the PR
description's "No user-facing changes" claim. Consider keeping these accessors
(possibly deprecated / doc(hidden)) for compatibility, or otherwise explicitly
acknowledging the API break.
##########
datafusion/physical-expr/src/scalar_subquery.rs:
##########
@@ -139,6 +126,64 @@ impl PhysicalExpr for ScalarSubqueryExpr {
fn fmt_sql(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "(scalar subquery)")
}
+
+ #[cfg(feature = "proto")]
+ fn try_to_proto(
+ &self,
+ _ctx:
&datafusion_physical_expr_common::physical_expr::proto_encode::PhysicalExprEncodeCtx<'_>,
+ ) -> Result<Option<datafusion_proto_models::protobuf::PhysicalExprNode>> {
+ use datafusion_proto_models::protobuf;
+ Ok(Some(protobuf::PhysicalExprNode {
+ expr_id: None,
+ expr_type:
Some(protobuf::physical_expr_node::ExprType::ScalarSubquery(
+ protobuf::PhysicalScalarSubqueryExprNode {
+ data_type: Some((&self.data_type).try_into()?),
+ nullable: self.nullable,
+ index: self.index.as_usize() as u32,
+ },
+ )),
+ }))
Review Comment:
`self.index.as_usize() as u32` will silently truncate if the subquery index
exceeds `u32::MAX`, producing a lossy/incorrect wire value. Since the protobuf
field is `u32`, it’s safer to use a checked conversion and return an error on
overflow.
--
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]
