alamb commented on code in PR #17189:
URL: https://github.com/apache/datafusion/pull/17189#discussion_r2277004846


##########
datafusion/expr/src/logical_plan/extension.rs:
##########
@@ -57,7 +57,7 @@ pub trait UserDefinedLogicalNode: fmt::Debug + Send + Sync {
     fn schema(&self) -> &DFSchemaRef;
 
     /// Perform check of invariants for the extension node.
-    fn check_invariants(&self, check: InvariantLevel, plan: &LogicalPlan) -> 
Result<()>;
+    fn check_invariants(&self, check: InvariantLevel) -> Result<()>;

Review Comment:
   I think technically this is an API change. Given this is a public API I 
worry that this change might break downstream users and there is no other way 
to get at the plan



##########
datafusion/pruning/src/pruning_predicate.rs:
##########
@@ -978,6 +978,7 @@ impl<'a> PruningExpressionBuilder<'a> {
                 }
             };
 
+        // TODO pass in SchemaRef so we don't need to clone the schema

Review Comment:
   I bet this would be a good first issue type ticket



##########
datafusion/common/src/dfschema.rs:
##########
@@ -899,6 +899,7 @@ impl TryFrom<SchemaRef> for DFSchema {
             field_qualifiers: vec![None; field_count],
             functional_dependencies: FunctionalDependencies::empty(),
         };
+        dfschema.check_names()?;

Review Comment:
   Context is that checking names here makes this consistent with the other 
(fallible) ways to create a DFSchema from a Schema



##########
datafusion-examples/examples/pruning.rs:
##########
@@ -187,10 +187,10 @@ impl PruningStatistics for MyCatalog {
 }
 
 fn create_pruning_predicate(expr: Expr, schema: &SchemaRef) -> 
PruningPredicate {
-    let df_schema = DFSchema::try_from(schema.as_ref().clone()).unwrap();
+    let df_schema = DFSchema::try_from(Arc::clone(schema)).unwrap();

Review Comment:
   this saves a deep `clone` I think 👍 



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Reply via email to