comphead commented on code in PR #26630:
URL: https://github.com/apache/datafusion/pull/26630#discussion_r4361929186
##########
datafusion/common/src/dfschema.rs:
##########
@@ -118,6 +120,13 @@ pub struct DFSchema {
field_qualifiers: Vec<Option<TableReference>>,
/// Stores functional dependencies in the schema.
functional_dependencies: FunctionalDependencies,
+ /// Lazily built lookup from field name to positions in `inner`, only used
+ /// for schemas with more than [`LINEAR_NAME_SCAN_MAX`] fields.
+ ///
+ /// Derived from the field names alone, so it is shared by clones and by
+ /// schemas that only change qualifiers, and must be reset whenever `inner`
+ /// changes. It takes no part in equality, hashing or `Debug` output.
+ name_index: OnceLock<Arc<NameIndex>>,
Review Comment:
The `OnceLock` makes `DFSchema` interior mutable for
`clippy::mutable_key_type`. A std-only crate confirms the lint propagates
through `Arc<LogicalPlan>`, so a downstream `HashSet<Expr>` or
`HashMap<LogicalPlan, _>` that lints with `-D warnings` can start failing. The
`clippy.toml` entry only covers this workspace. That is a visible change for a
PR that lists no API changes.
`check_names` already runs a `BTreeSet` pass over every field on each
construction. Building the index eagerly (for example an
`Option<Arc<NameIndex>>` set in the constructors above the threshold) may
therefore cost less than that pass. That is an expectation, not a measurement.
Could you bench an eager variant with `sql_planner`? If it is close, it drops
the lint change and the lazy-init state. It interacts with the threshold
comment below, since eager builds also hit schemas that are never probed.
##########
datafusion/common/src/dfschema.rs:
##########
@@ -846,6 +894,7 @@ impl DFSchema {
field_qualifiers: vec![None; self.inner.fields.len()],
inner: self.inner,
functional_dependencies: self.functional_dependencies,
+ name_index: self.name_index,
Review Comment:
Sharing the index here, in `replace_qualifier` and in
`with_field_specific_qualified_schema` only helps when the source schema was
probed before the rewrite. Outside tests these have a few callers (substrait
read, the unparser rewrite, a physical planner join-schema helper) and none in
the analyzer or optimizer rules the PR targets. Unless a profile shows one of
them hot, I would drop these moves and keep the index tied to `Clone` and
`merge` only. That shrinks the "index follows `inner`" invariant to two places
and removes the `Arc::ptr_eq` assertions on the rewrite paths from the test.
##########
datafusion/common/src/dfschema.rs:
##########
@@ -1766,6 +1930,135 @@ mod tests {
Ok(())
}
+ /// Builds a schema of `fields` padded with uniquely named fields to
`width`.
+ /// Bypasses `check_names`, since `merge` can produce the same name both
+ /// qualified and unqualified.
+ fn name_lookup_schema(
+ fields: &[(Option<TableReference>, &str)],
+ width: usize,
+ ) -> DFSchema {
+ let mut qualifiers = vec![];
+ let mut arrow_fields = vec![];
+ for (qualifier, name) in fields {
+ qualifiers.push(qualifier.clone());
+ arrow_fields.push(Field::new(*name, DataType::Int32, true));
+ }
+ for i in fields.len()..width {
+ qualifiers.push(Some(TableReference::bare("pad")));
+ arrow_fields.push(Field::new(format!("pad_{i}"), DataType::Int32,
true));
+ }
+ DFSchema {
+ inner: Arc::new(Schema::new(arrow_fields)),
+ field_qualifiers: qualifiers,
+ functional_dependencies: FunctionalDependencies::empty(),
+ name_index: OnceLock::new(),
+ }
+ }
+
+ /// The name lookups as they were before the name index, kept as a
reference.
+ fn scan_index_of_column_by_name(
+ schema: &DFSchema,
+ qualifier: Option<&TableReference>,
+ name: &str,
+ ) -> Option<usize> {
+ schema.iter().position(|(q, f)| match (qualifier, q) {
+ (Some(q), Some(field_q)) => q.resolved_eq(field_q) && f.name() ==
name,
+ (Some(_), None) => false,
+ (None, _) => f.name() == name,
+ })
+ }
+
+ #[test]
+ fn name_index_matches_scan() {
Review Comment:
The PR body says every converted lookup is compared with the scan, but
`fields_with_unqualified_name` and `columns_with_unqualified_name` are not
asserted here. Nothing checks the hand-written `PartialEq` and `Debug` either,
and those keep plan snapshots stable. A cheap addition to
`name_index_follows_schema_changes`: compare a schema whose index is built with
an identical schema that was never probed, using `assert_eq!` on the values and
on their `{:?}` strings.
##########
datafusion/common/src/dfschema.rs:
##########
@@ -1159,6 +1210,116 @@ impl Hash for DFSchema {
}
}
+// `name_index` is derived state, so it is left out of equality and `Debug`.
+impl PartialEq for DFSchema {
+ fn eq(&self, other: &Self) -> bool {
+ self.inner == other.inner
+ && self.field_qualifiers == other.field_qualifiers
+ && self.functional_dependencies == other.functional_dependencies
+ }
+}
+
+impl Eq for DFSchema {}
+
+impl std::fmt::Debug for DFSchema {
+ fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+ f.debug_struct("DFSchema")
+ .field("inner", &self.inner)
+ .field("field_qualifiers", &self.field_qualifiers)
+ .field("functional_dependencies", &self.functional_dependencies)
+ .finish()
+ }
+}
+
+/// Schemas with up to this many fields resolve names by comparing against
every
+/// field; wider ones use a lazily built [`NameIndex`]. Below it, the scan is
+/// cheaper than building the index (the same trade-off as
+/// `LINEAR_FIELD_SCAN_MAX` in `datafusion-datasource-parquet`).
+const LINEAR_NAME_SCAN_MAX: usize = 8;
Review Comment:
8 looks low. The benchmark run on this PR shows `logical_union_8_branches`
at 625.7 ±2.0 µs on the base and 642.7 ±1.7 µs here (+2.7%), and
`logical_correlated_subquery_in` at 208.9 vs 212.2 µs (+1.6%). Both queries use
`t1`/`t2` with 200 columns and project two of them, so each scan schema pays an
O(W) index build for two lookups that a scan likely answers more cheaply (not
measured). The PR body lists these as within noise, but the intervals do not
overlap. `SELECT a, b FROM wide_table` is the common shape, so this is the case
to protect.
The linked issue suggested a break-even near 64 fields. Could you add a
width sweep (for example 9, 16, 32, 64, 128, 200) with 2 lookups and with W
lookups, and choose the threshold from that? The `SELECT id` case on the
10,240-column file (23 ms on `main`) is also missing from the benchmark table.
--
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]
