dongjoon-hyun commented on PR #58412:
URL: https://github.com/apache/spark/pull/58412#issuecomment-5528646817
Follow-up review of c845783a906 against the latest `master` (bcea2b23caa).
Items already covered by the existing threads and my earlier comment are
excluded. All findings are established from source; no runtime reproduction was
attempted.
### Test fixtures (this PR)
**1. [P2] `InMemoryRowLevelOperationTable` did not receive the
identity-transform restriction** (`InMemoryRowLevelOperationTable.scala`,
`filterAttributes`)
After the review fixes, the V1/V2/Catalyst fixtures advertise only
identity-transform sources, but the row-level fixture still returns
`partitioning.flatMap(_.references())` while sharing the same
`CatalystRuntimeFilteringScan` evaluator that binds only identity slots. With
the new `createTable(columns, transforms)` overload, a `bucket(4, dep)` or
`days(...)` row-level table gets a group filter injected and recorded, but
`filter()` returns early on `partAttrs.isEmpty` and prunes nothing, so a future
`checkReplacedPartitions` assertion would rewrite every partition and fail
without pointing at the fixture asymmetry. Suggest using
`identityPartitionReferences` here as well, or documenting why row-level
differs.
**2. [P3] The test name "transformed partition source cannot be declared
fully pushed" implies a Spark-side guard that does not exist**
(`DataSourceV2CatalystRuntimeFilterSuite.scala`)
The test exercises no rejection: the fixture's
`fullyPushedFilterAttributes()` silently drops `part` via
`identityPartitionAttrs`, and `checkFullyPushedFilterAttrs` only rejects nested
references. A real connector partitioned by `days(part)` that lists `part` in
`fullyPushedFilterAttributes()` still passes validation, `DataSourceV2Strategy`
drops the post-scan `FilterExec`, and the scan compares a date against a day
ordinal. Suggest renaming the test to describe the fixture behavior, or adding
a Spark-side guard and a test for it.
**3. [P3] `InMemoryCatalystRuntimeFilterTable.fullyPushedFilterAttributes()`
advertises an unused test hook** (`InMemoryCatalystRuntimeFilterTable.scala`)
It switched from `filterAttributes().filter(...)` to
`identityPartitionAttrs.filter(...)`, and the new comment says a table can now
declare a fully pushed attribute outside `filterAttributes()`, "a combination
the interface forbids". No test sets both `filter-attributes` and
`fully-pushed-filter-attributes`, so the hook is unused and `partitionAttrs` /
`identityPartitionAttrs` are near-duplicates to keep in sync.
`filterAttributes().filter(ref => identityRefs.contains(ref) &&
fullyPushedFilterAttrs.contains(...))` covers the actual need without the extra
helper or the comment.
**4. [P3] `expectedFilterPaths` duplicates `expectedFilterAttrs`**
(`RowLevelOperationCatalystRuntimeFilterSuiteBase.scala`,
`assertCatalystGroupFilter`)
`expectedFilterAttrs` is already asserted equal to
`scan.filterAttributes().map(_.fieldNames.mkString("."))`, so both new callers
pass the same information twice (`Seq("dep.name")` and `Some(Seq(Seq("dep",
"name")))`). `expectedFilterAttrs.map(_.split('.').toSeq)` works for every
existing call site and lets the two nested tests drop the extra argument.
**5. [P3] ~40 unchanged lines re-indented in `InMemoryBatchScan.filter` and
`InMemoryTableWithV2Filter.filter`**
Both bodies were wrapped in `partitioning match { case
Array(IdentityTransform(ref)) => ... case _ => }`, adding a nesting level to
every existing line although the only semantic change is that the single
transform must be an identity. `git blame` on both bodies now points at this
backport instead of the SPARK-56467 history, and later cherry-picks across
branches will conflict on every line. A two-line change to the original guard
keeps the body intact: `if (partitioning.length == 1 &&
identityPartitionReferences.length == 1) { val ref =
identityPartitionReferences.head; ... }`.
### Interface docs (same text exists on master; best fixed upstream)
**6. [P3] Root-attribute eligibility is documented as a contract rather than
current behavior** (`SupportsRuntimeFiltering.java`,
`SupportsRuntimeV2Filtering.java`, `SupportsRuntimeCatalystFiltering.scala`)
Eligibility is `references.subsetOf(AttributeSet)` in `PartitionPruning`,
`DataSourceV2Strategy` and `PushDownUtils`, so a scan advertising
`derives.toStr` also receives predicates on `derives.other`. Beyond the
connector-side burden the new docs describe, this injects a
`DynamicPruningSubquery` for a join on the sibling field, runs (and may
broadcast) the build side, and the source then ignores the pushed filter.
Results stay correct, but no test observes the DPP path (the "sibling" test
covers only scalar subqueries). Since path-aware eligibility is deferred to
SPARK-59095, the Javadoc could say Spark "currently" tracks eligibility by root
attribute rather than stating it as the interface contract.
Generated-by: Claude Fable 5.1
--
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]
