[
https://issues.apache.org/jira/browse/SPARK-58769?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Peter Toth updated SPARK-58769:
-------------------------------
Description:
Spark compares partition transform expressions with ordinary expression
equality, which reaches {{BoundFunction.equals}}. It binds a function afresh
every time it converts a partitioning or ordering reported by a source, so two
reports of one transform - from two scans of the same table, or from the
partitioning and the ordering of a single scan - hold two bound instances. A
function that does not implement {{equals}}/{{hashCode}} therefore yields
expressions that never compare equal across binds, so Spark cannot deduplicate
them, reuse a scan reporting them, keep a union's partitioning, or recognize
two subplans as the same. That costs optimizations only, never correctness.
{{canonicalName}} is a documented obligation; {{equals}} was never mentioned.
Spark cannot derive the identity itself either, because {{BoundFunction}} is an
open interface and a single class can capture behaviour-affecting state at bind
time (a width, a precision, a timezone, a snapshot id), so there is no list of
state to compare. Only the implementation knows which of its state matters.
This documents the contract on {{BoundFunction}}: implementations SHOULD
override {{equals}}/{{hashCode}}, comparing whatever state affects behaviour;
{{canonicalName}} alone is not necessarily enough; whatever is compared must be
stable across bind calls; {{hashCode}} must agree. It also states the
relationship between the two comparisons - {{equals}} is the finer one, so two
functions that compare equal must return the same {{canonicalName}}, while the
same name does not make them equal - and scopes {{canonicalName}}'s own javadoc
to the question it really answers.
With that written down, SPARK-58549's {{PlanMerger}} workaround (comparing
reported transforms by {{isSameFunction}}, i.e. by canonical name with
arguments ignored) is removed, so plan identity has one notion instead of two:
every other place that compares a reported key-grouped partitioning or ordering
already treats it as an ordinary expression ({{UnionExec.comparePartitioning}},
{{GroupPartitionsExec.outputOrdering}}, {{BatchScanExec.equals}},
{{DataSourceV2ScanRelation}}'s canonical form). A connector without {{equals}}
loses that one merge and keeps missing the others exactly as before;
SPARK-58549 is unreleased, so no released behaviour changes.
> Document the BoundFunction equality contract and drop the name-based
> transform comparison
> -----------------------------------------------------------------------------------------
>
> Key: SPARK-58769
> URL: https://issues.apache.org/jira/browse/SPARK-58769
> Project: Spark
> Issue Type: Improvement
> Components: SQL
> Affects Versions: 5.0.0
> Reporter: Peter Toth
> Priority: Major
> Labels: pull-request-available
>
> Spark compares partition transform expressions with ordinary expression
> equality, which reaches {{BoundFunction.equals}}. It binds a function afresh
> every time it converts a partitioning or ordering reported by a source, so
> two reports of one transform - from two scans of the same table, or from the
> partitioning and the ordering of a single scan - hold two bound instances. A
> function that does not implement {{equals}}/{{hashCode}} therefore yields
> expressions that never compare equal across binds, so Spark cannot
> deduplicate them, reuse a scan reporting them, keep a union's partitioning,
> or recognize two subplans as the same. That costs optimizations only, never
> correctness.
> {{canonicalName}} is a documented obligation; {{equals}} was never mentioned.
> Spark cannot derive the identity itself either, because {{BoundFunction}} is
> an open interface and a single class can capture behaviour-affecting state at
> bind time (a width, a precision, a timezone, a snapshot id), so there is no
> list of state to compare. Only the implementation knows which of its state
> matters.
> This documents the contract on {{BoundFunction}}: implementations SHOULD
> override {{equals}}/{{hashCode}}, comparing whatever state affects behaviour;
> {{canonicalName}} alone is not necessarily enough; whatever is compared must
> be stable across bind calls; {{hashCode}} must agree. It also states the
> relationship between the two comparisons - {{equals}} is the finer one, so
> two functions that compare equal must return the same {{canonicalName}},
> while the same name does not make them equal - and scopes {{canonicalName}}'s
> own javadoc to the question it really answers.
> With that written down, SPARK-58549's {{PlanMerger}} workaround (comparing
> reported transforms by {{isSameFunction}}, i.e. by canonical name with
> arguments ignored) is removed, so plan identity has one notion instead of
> two: every other place that compares a reported key-grouped partitioning or
> ordering already treats it as an ordinary expression
> ({{UnionExec.comparePartitioning}}, {{GroupPartitionsExec.outputOrdering}},
> {{BatchScanExec.equals}}, {{DataSourceV2ScanRelation}}'s canonical form). A
> connector without {{equals}} loses that one merge and keeps missing the
> others exactly as before; SPARK-58549 is unreleased, so no released behaviour
> changes.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
