dongjoon-hyun commented on PR #58412:
URL: https://github.com/apache/spark/pull/58412#issuecomment-5497263304
I reviewed this backport with a multi-agent review (8 finder angles, then a
verification pass), cross-checking against `master` and the existing review
threads. Findings already raised in earlier comments are excluded. All findings
are established from source; no runtime reproduction was attempted.
### Merge order / cross-branch sync
**1. [P2] The last commit pre-includes the still-open master follow-up
#58439** (`DataSourceV2Relation.scala`)
c845783a906 is the same change as #58439 (eager resolution of
`fullyPushedFilterAttributes()` from `runtimeFilterAttrs`). If this merges
first, branch-4.3 becomes stricter than master: a connector whose
`fullyPushedFilterAttributes()` names a column pruned out of the read schema
plans fine on master today but fails every `SELECT` on 4.3 with
`DATA_SOURCE_INVALID_RUNTIME_FILTER_ATTRIBUTE.CANNOT_RESOLVE`. Suggest landing
#58439 first, or noting the intended order here.
**2. [P2] The fixture transform-semantics fixes have no master counterpart**
(`InMemoryBaseTable.scala`, `InMemoryTableWithV2Filter.scala`,
`InMemoryCatalystRuntimeFilterTable.scala`)
The identity-only binding fixes from this PR's review (51ee79a5692 /
070a430c90b) and their regression tests exist only on this branch. master's
`CatalystRuntimeFilteringScan` still binds transformed-source predicates (e.g.
`days()`, `truncate()`) against transformed partition-key slots -- the exact
mis-pruning fixed here -- and only the validation commit is covered by #58439.
Suggest sending the fixture fixes to master as well so future backports of
these files do not conflict.
### Production code (same code exists on master)
**3. [P2] A top-level struct root in `fullyPushedFilterAttributes()` is
unguarded** (`DataSourceV2Relation.scala`, `checkFullyPushedFilterAttrs`)
The check rejects only nested refs (`fieldNames.length > 1`). A scan
partitioned by `s.part` declaring `filterAttributes=[s.part]` and
`fullyPushedFilterAttributes=[s]` lets a scalar-subquery filter on the sibling
`s.other` (references `{s}`) pass the subset check in `DataSourceV2Strategy`,
so `postScanFilters.diff(fullyPushedRuntimeFilters)` removes its `FilterExec`
while the scan can only prune by `s.part` -- rows violating the `s.other`
predicate are returned. The only guard is the new scaladoc sentence ("must not
return the root struct as a substitute..."); consider a code check or at least
a test.
**4. [P3] DPP and row-level paths bypass the new structured error**
(`PartitionPruning.scala`, `RowLevelOperationRuntimeGroupFiltering.scala`,
`PushDownUtils.scala`)
These sites still resolve `filterAttributes()` directly via
`V2ExpressionUtils`, so an invalid attribute surfaces there as raw
`_LEGACY_ERROR_TEMP_1137` / `INVALID_EXTRACT_BASE_FIELD_TYPE` from an optimizer
rule, while a plain `SELECT` over the same scan produces the new
`DATA_SOURCE_INVALID_RUNTIME_FILTER_ATTRIBUTE` error -- inconsistent
diagnostics for the same connector mistake, and the DPP-shaped path is untested.
### Cleanup (same code exists on master; best fixed upstream, then
re-backported)
**5. [P3]** `unalias` in
`RowLevelOperationRuntimeGroupFiltering.buildDynamicPruningCond` re-implements
`AliasHelper.trimAliases`, which is already in scope via `PredicateHelper`.
**6. [P3]** The `readSchema.findNestedField(...).isDefined` filter block is
copy-pasted five times across four in-memory fixture files; a shared helper
next to `identityPartitionReferences` would collapse all call sites.
**7. [P3]** `partitionKeyPath` in `InMemoryBaseTable` matches
`GetStructField` ordinals manually, while this PR's own `fieldPath`
(`RowLevelOperationCatalystRuntimeFilterSuiteBase`) uses the existing
`GetStructFieldObject` extractor for the identical recursion.
**8. [P3]** `SQLConf.get.resolver` is re-fetched per expression-tree node in
`partitionAttrFor` (and per partitioning entry in `partitionAttributes`); the
pre-PR code hoisted it once per `filter()` call.
Generated-by: Claude Fable 5
--
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]
