Re: [PR] refactor: Migrate ScalarSubqueryExpr to self-serialization proto pattern [datafusion]

["Copilot (via GitHub)" <gi...@apache.org>]

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: 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