dwsmith1983 commented on code in PR #25656:
URL: https://github.com/apache/datafusion/pull/25656#discussion_r4158766831
##########
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:
Good call. Moved it into `DFSchema::logically_equivalent_names_and_types` in
54919c7c3, so `assert_expected_schema` is back to what main has.
##########
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:
You're right, it isn't an invariant, only an observation that none of the
built-in rules create a subquery. 54919c7c3 makes the rescan always run during
the final pass. A subquery a custom rule adds earlier changes the plan, so
another pass starts and finds it; on the last iteration the rescan after each
changed rule now happens as before, so later rules still descend into it.
`subquery_introduced_in_the_final_pass_is_visited_in_that_pass` covers that
with `max_passes = 1`. Most plans stop changing before they reach the last
pass, so the extra scans rarely run.
##########
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:
Done, the test now sits next to it in `dfschema.rs`, and it also checks that
a separately built but identical schema still compares equal.
--
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]
