Copilot commented on code in PR #24389:
URL: https://github.com/apache/datafusion/pull/24389#discussion_r3787591193
##########
datafusion/physical-plan/src/union.rs:
##########
@@ -1627,6 +1643,31 @@ mod tests {
Ok(())
}
+ #[test]
+ fn test_union_schema_fast_path_content_equal() -> Result<()> {
+ // Inputs whose schemas are pointer-distinct but structurally equal
must
+ // take the content-equality (`==`) fast path and still produce a
schema
+ // equal to the shared one, matching the slow-path merge exactly.
+ let schema = create_test_schema()?;
+ let distinct: SchemaRef = Arc::new((*schema).clone());
+ // Guard the branch under test: these must NOT be the same allocation,
so
+ // the fast path is reached via `==` rather than `Arc::ptr_eq`.
+ assert!(!Arc::ptr_eq(&schema, &distinct));
+
+ let memory_exec1 =
+ Arc::new(TestMemoryExec::try_new(&[], Arc::clone(&schema), None)?);
+ let memory_exec2 =
+ Arc::new(TestMemoryExec::try_new(&[], Arc::clone(&distinct),
None)?);
+ let memory_exec3 =
+ Arc::new(TestMemoryExec::try_new(&[], Arc::clone(&distinct),
None)?);
+
+ let union_plan =
+ UnionExec::try_new(vec![memory_exec1, memory_exec2,
memory_exec3])?;
+
+ assert_eq!(union_plan.schema(), schema);
+ Ok(())
Review Comment:
The new test exercises pointer-distinct-but-equal schemas, but it doesn’t
actually assert that the content-equality fast path was taken; the slow-path
merge would also produce an equal schema and this test would still pass. To
make this test guard the intended optimization, assert `Arc::ptr_eq` between
the union schema and the first input’s schema (or otherwise verify the
early-return behavior).
##########
datafusion/physical-plan/benches/union_schema.rs:
##########
@@ -0,0 +1,157 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Benchmark for `UnionExec` construction cost as a function of child count.
+//!
+//! Scenarios (run against a flat and a nested/struct schema):
+//! - `shared_arc`: every child returns the same `Arc<Schema>`
+//! - `content_equal`: pointer-distinct but identical schemas per child
+//! - `last_differs`: identical except the last child's deepest field (worst
+//! case for any equality fast path: the scan is wasted, then the full
+//! merge runs)
+
Review Comment:
The benchmark module docs list only three scenarios and describe
`last_differs` as changing the last child’s “deepest field”, but the benchmark
also includes `names_differ` and the divergence is implemented as extra
metadata on the last top-level field. Updating the docs will keep the benchmark
intent clear for future readers.
--
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]
