Repository navigation
Conversation
302f3b8 to
e23797a
Compare
e23797a to
50d1e68
Compare
|
@cloud-fan @peter-toth could you help take a look ? Thanks |
|
thanks @peter-toth, addressed FYI i found an issue in the first approach. The normalization of the nested partition field filters, ie GetStruct(AttributeRef("parent"), "child)) => AttributeRef("parent.child") messed up the post-scan filter if rejected, so restoring them to original form before returning. |
cloud-fan
left a comment
cloud-fan
left a comment
There was a problem hiding this comment.
Review Summary
This PR extends DSV2 partition predicate pushdown (SPARK-55596) to support nested partition columns (e.g., PARTITIONED BY (s.tz)). The approach normalizes GetStructField chains to flat AttributeReferences before the existing partition filter machinery, then denormalizes results back for post-scan filters. The design is clean — normalization is localized to pushPartitionPredicates without changing shared utilities.
One correctness concern with the denormalization map lookup, and two minor text fixes from the rename.
1798ff3 to
4fb6bc8
Compare
cloud-fan
left a comment
cloud-fan
left a comment
There was a problem hiding this comment.
Summary
The normalize-then-restore approach for extending partition predicate pushdown to nested columns is well-designed. The ExprId-based binding in PartitionPredicateImpl is a good improvement over the old name-based binding, and the Seq[String]-based path comparison in pathsMatch correctly handles case sensitivity.
One concern: the flattened dotted names in the partition StructType (e.g., "s.tz" for nested field s → tz) can collide with columns that have literal dots in their names when passed through getPartitionFiltersAndDataFilters, which matches by attr.name. See inline comment on pushPartitionPredicates.
Minor items: missing return type annotation, indentation fix, and a test coverage suggestion for deeply nested (3+ level) partition fields — the recursive flattenStructFieldChain should handle arbitrary depth but an end-to-end test would confirm.
- Add new getPartitionFiltersAndDataFilters overload that takes Seq[AttributeReference] and matches by ExprId via AttributeSet, avoiding collisions when field names contain literal dots. - Add explicit return type to resolveIdentityPartitionField. - Fix indentation in assertReferencedPartitionFieldOrdinals. - Add test for nested partition field with dot in name.
|
Thanks @cloud-fan , i addressed the missed case and added a unit test for nested field with a dot in the field name. |
cloud-fan
left a comment
cloud-fan
left a comment
There was a problem hiding this comment.
Prior reviews established context for the normalize-then-restore design. This round focuses on a binding simplification and minor text fixes.
General:
PartitionPredicate.javaeval()Javadoc (line 82) still says "a subset of referenced columns" — should be "referenced fields" for consistency with thePartitionColumnReference→PartitionFieldReferencerename.
cloud-fan
left a comment
cloud-fan
left a comment
There was a problem hiding this comment.
LGTM, minor comments only.
…c text - Replace manual transform+exprIdToIndex binding with BindReferences.bindReference in PartitionPredicateImpl. - Remove unused structField lazy val from PartitionPredicateField. - Fix Javadoc: "columns" -> "fields" in PartitionPredicate.eval(). - Fix Scaladoc: "Return" -> "Returns", add plural "s" to [[AttributeReference]]s, "name" -> "names".
|
thanks, merging to master! |
3 participants
What changes were proposed in this pull request?
Supported nested columns.
Why are the changes needed?
DSV2 connectors support nested struct fields as partition fields.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Add unit tests to DSV2EnhancedPartitionFilterSuite.
Was this patch authored or co-authored using generative AI tooling?
Yes, cursor with hand refactoring