alamb commented on code in PR #25656:
URL: https://github.com/apache/datafusion/pull/25656#discussion_r4158107095
##########
datafusion/expr/src/logical_plan/invariants.rs:
##########
@@ -503,4 +511,38 @@ mod test {
check_inner_plan(&plan).unwrap();
}
+
+ #[test]
+ fn assert_expected_schema_accepts_same_arc_and_rejects_renamed_schema() {
+ use arrow::datatypes::{DataType, Field, Schema};
+
+ let arrow_schema = Schema::new(vec![Field::new("a", DataType::Int32,
false)]);
+ let schema: DFSchemaRef =
+ Arc::new(DFSchema::try_from_qualified_schema("t",
&arrow_schema).unwrap());
+ let plan =
LogicalPlan::EmptyRelation(crate::logical_plan::EmptyRelation {
+ produce_one_row: false,
+ schema: Arc::clone(&schema),
+ });
+
+ // The fast path: the plan's schema is the exact same Arc as `schema`.
+ assert_expected_schema(&schema, &plan).unwrap();
+
+ // A schema with a different field name (same type) must still be
+ // rejected once the fast path can't apply.
+ let renamed_arrow_schema =
Review Comment:
I recommend we move these tests to logically_equivalent_names_and_types as
well
##########
datafusion/expr/src/logical_plan/invariants.rs:
##########
@@ -112,6 +114,12 @@ fn assert_valid_semantic_plan(plan: &LogicalPlan) ->
Result<()> {
/// Returns an error if the plan does not have the expected schema.
/// Ignores metadata and nullability.
pub fn assert_expected_schema(schema: &DFSchemaRef, plan: &LogicalPlan) ->
Result<()> {
+ // A plan whose schema is the exact same Arc as `schema` is trivially
+ // compatible with it, so skip the field-by-field comparison below.
+ if Arc::ptr_eq(plan.schema(), schema) {
Review Comment:
How about we move this check into `logically_equivalent_names_and_types` ?
It would then benefit more sites perhaps
##########
datafusion/optimizer/src/optimizer.rs:
##########
@@ -708,7 +708,26 @@ impl Optimizer {
new_plan = data;
observer(&new_plan, rule.as_ref());
if transformed {
- has_subqueries = plan_has_subqueries(&new_plan);
+ // Only rescan for subqueries when this pass
+ // already saw one: none of the built-in rules
+ // construct a subquery expression from scratch,
Review Comment:
Is this an invariant? I am not quite sure about this argument
What if the subqueries got introduced on the last iteration of the optimizer
(when there won't be another pass 🤔 )
--
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]
