cetra3 commented on code in PR #26630:
URL: https://github.com/apache/datafusion/pull/26630#discussion_r4384287330
##########
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:
You're right that both regressions are real; the "within noise" line came
from a bad local run. I've done a sweep with `sql_planner`. Each threshold was
run twice, interleaved with three runs of `main` that agreed within ±0.3%:
| Benchmark | `main` | 8 (this PR) | 64 | 256 |
|---|---|---|---|---|
| `logical_union_8_branches` | 339.1 µs | +2.1% / +1.8% | +2.2% / +2.1% |
−0.2% / −0.3% |
| `logical_correlated_subquery_in` | 109.9 µs | +1.7% / +1.4% | +1.4% /
+1.2% | +0.2% / −0.3% |
| `with_param_values_many_columns` | 228.0 µs | −36.8% / −36.8% | −35.4% /
−35.7% | −16.4% / −14.8% |
| `logical_trivial_join_high_numbered_columns` | 113.2 µs | −6.7% / −6.3% |
−6.9% / −6.5% | −6.7% / −6.6% |
| `physical_many_self_joins` | 4.36 ms | −5.6% / −5.9% | −6.1% / −5.8% |
−5.8% / −5.0% |
| `*_select_all_from_1000` (3 benchmarks) | 4.2–10.6 ms | −67% | −67% | −67%
|
| `optimizer_tpcds_all` | 155.5 ms | −2.8% / −2.7% | −2.7% / −2.3% | −1.8% /
−2.4% |
| Other 6 benchmarks | | within ±2% | within ±2% | within ±2% |
64 behaves the same as 8, since no schema in these benchmarks has between 9
and 64 columns. 256 removes the cost on `union_8` and `correlated_in`, but
`with_param_values_many_columns` plans `SELECT MAX(a0), …, MAX(a199)` over the
same 200-column `t1`, and it loses about 20 points.
So on the same 200-column table, the index costs about 7 µs when 2 columns
are projected and saves about 46 µs when 200 are. Width alone can't tell those
apart. A local microbenchmark agrees: building the index costs about 4 ns per
field, and it pays for itself after 4–8 lookups at every width from 32 to 1000.
I've kept 8, so the remaining cost is bounded by one index build per wide
schema that gets looked up.
`SELECT id` over the 10,240-column file takes 23 ms on both `main` and this
PR.
--
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]
