yyanyy commented on PR #58298: URL: https://github.com/apache/spark/pull/58298#issuecomment-5642284265
> Request changes. I found four issues: > > 1. **[P1] Resolver lookup can silently bind the wrong column.** > `CapturedSchemaProjection.scala:248` uses the first resolver match without preferring an exact name. Capture `s`, then externally add a U+017F LONG S column first while retaining `s`. Schema validation treats the names as distinct, but `equalsIgnoreCase` matches both, so rebinding selects the new column and returns its values under the captured expression ID. Lines 58 and 127 have the same issue for metadata and nested fields. Please prefer an exact match and otherwise require a unique resolver match. > 2. **[P2] A second compatible schema refresh makes a derived cache entry unreachable.** > Cache rebuilding persists the already-refreshed normalized plan. Two additions therefore turn the key into `Project(v1) -> Project(v2) -> Relation(v3)`, while an action on the original Dataset refreshes directly to `Project(v1) -> Relation(v3)`. Both field-dropping Projects survive canonicalization, so `sameResult` misses and the cache stops being used. Please preserve the original capture and replace rather than stack generated projections, and extend the [SPARK-54424](https://issues.apache.org/jira/browse/SPARK-54424) cache test through two additions. > 3. **[P2] Added metadata columns still reproduce the `toOutputAttrs` crash.** > Lines 56-69 expose all current data columns but only previously captured metadata. `SupportsPushDownRequiredColumns` permits partial pruning, so a valid source may retain newly added metadata in `Scan.readSchema`. That field is absent from the rebound relation output and `toOutputAttrs` still throws `NoSuchElementException`. The new test fixture explicitly prunes unrequested metadata and masks this case. The rebound relation should expose all available current metadata and let the Project discard unused columns. > 4. **[P2] Nullable struct rebinding defeats nested pruning and filter pushdown.** > The expression generated around line 125, `If(IsNull(...), ..., CreateNamedStruct(GetStructField(KnownNotNull(...))))`, is opaque to the current extraction and schema-pruning rules. Selecting or filtering on one nested field consequently reads and rebuilds the entire current struct, even for a source that fully honors nested pruning; the simple predicate also no longer reaches DSv2 pushdown. Please use an optimizer-friendly null-preserving form or add the corresponding simplification and projection rules, with a recording fully-pruning source test. Thanks for the feedback! I reproduced all four. 1 is fixed; 2 and 4 I would rather leave as follow-ups; 3 I do not think applies. **2. A second compatible refresh makes a derived cache entry unreachable — confirmed, but I would leave it.** `CacheManager.tryRebuildCacheEntry` stores the refreshed plan as the new cache key, so a later refresh of that entry rebinds the relation underneath the projection that is already there and ends up with two stacked projections, while a query rebuilds a single one from its own captured output. The keys stop matching and the entry is no longer reused. Results stay correct. Reaching that takes a cached plan, two external schema changes with a refresh in between, and a Dataset derived from the pre-change one. The entry comes back as soon as it is re-cached, and this area is best-effort by design — `inability to refresh cache shouldn't fail operations` drops the entry outright on any refresh failure. Against that, the fix has to rewrite the plan this rule just built: detect that a projection above a relation is one we generated, then replace it instead of nesting inside it. Getting that detection wrong is a correctness problem, which is a worse risk than the missed cache it buys, so I would rather not put it in this change. What I did add: test `SPARK-54424: successfully refresh cache with compatible schema changes` now goes through a second addition and asserts the results are still correct, so the two-round rebinding path is covered even though the cache is not reused, and the limitation is recorded where the projection is built. Happy to fold the fix in if you disagree. **3. Added metadata columns — out of scope, since nothing here regresses.** This change does not alter that behaviour in either direction: a read schema carrying a metadata column that is not in `relation.output` failed in `toOutputAttrs` before it and fails the same way after, refreshed or not, because those attributes are only ever in the output when the query referenced them. So there is no regression for metadata columns to fix here, and hardening that path would be a separate change in `toOutputAttrs`, for all DSv2 relations. I also do not think a conforming source can produce that read schema. A source that cannot prune returns more than it was asked for, but at most what it would return with no pruning at all, which is `table.schema()`. Metadata columns are not in `table.schema()` — a source emits one only because Spark put it into the required schema. So failing to prune can never add a metadata column; the source would have to invent one nobody asked for. **4. Nullable struct rebinding defeats nested pruning and pushdown — confirmed; also better separately.** Confirmed. Affected: a plan analyzed before a field was added *inside* a struct, array or map, then executed after. With `ADD COLUMN person.age INT FIRST`, a query reading only `person.name` and `person.city` asks for `STRUCT<person: STRUCT<age, name, city>>`, and a nested predicate becomes `if (isnull(person)) false else knownnotnull(person).name = Alice`, which no longer translates — `V2ScanRelationPushDown` runs after the operator-optimization batch and nothing rewrites `GetStructField` over `If`. Not affected: a top-level column addition, which reuses expression IDs and produces a plain attribute project list; and any query analyzed after the change, which prunes normally. So this is confined to an already-analyzed plan that hits the analysis-to-execution window this PR is about, where the same plan previously crashed or read the wrong column. The result is correct, only read wider than necessary. The fix is not small either. The rebuilt struct exists to restore captured field names, order, metadata and nullability, and an optimizer-friendly form has to keep all of that. `UpdateFields` is the promising direction, since `SimplifyExtractValueOps` already unwraps `GetStructField(UpdateFields(...))` back to a plain `GetStructField(attr, ordinal)`, but it covers only the drop-only, metadata-preserving subset — `WithField` builds a `StructField` with no metadata — so captured field metadata, reordering, arrays and maps still need the current form. That is a hybrid `projectToType` plus new pruning tests, which I would rather not fold in here. -- 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]
