szehon-ho commented on code in PR #58002:
URL: https://github.com/apache/spark/pull/58002#discussion_r3807138010
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/functions/BoundFunction.java:
##########
@@ -100,4 +135,49 @@ default String canonicalName() {
// bugs if not replaced before release.
return UUID.randomUUID().toString();
}
+
+ /**
+ * Implementations SHOULD override {@link Object#equals(Object)} and {@link
Object#hashCode()}.
+ * <p>
+ * Spark binds a function afresh every time it converts a partitioning or
ordering reported by a
+ * source, so two reports of the same transform -- from two scans of the
same table, say -- hold
+ * two bound instances. Spark compares the partition transform expressions
built from them with
+ * ordinary expression equality, which reaches this method. With the
inherited identity comparison
+ * the two are never equal, and Spark cannot deduplicate the expressions,
reuse a scan that
+ * reports them, or recognize two subplans as the same. That costs
optimizations only: results
+ * are unaffected either way.
+ * <p>
+ * Compare whatever state affects behaviour. {@link #canonicalName()} on its
own is not
+ * necessarily enough: it answers a narrower question and is allowed to be
coarse, so two
+ * functions can share one and still differ in {@link #inputTypes()}, {@link
#resultType()},
+ * {@link #isResultNullable()}, {@link #isDeterministic()}, or a {@link
ReducibleFunction}'s
+ * reducers. Comparing the name IS sufficient for an implementation whose
names already encode
+ * everything that distinguishes its functions -- one name per argument
type, say. Only the
+ * implementation knows which of its state matters, which is why Spark
cannot derive this
+ * comparison itself. Note that a coarse name does not have to carry the
whole identity: Spark
+ * compares the transform's arguments separately.
+ * <p>
+ * This is the FINER of the two comparisons, so two functions that compare
equal must also return
+ * the same {@link #canonicalName()}; an implementation overriding this
method should override
+ * that one too, rather than leave it at its default.
+ * <p>
+ * Two functions that can produce DIFFERENT values must NOT compare equal.
Being too coarse is the
+ * direction that costs more than optimizations, and it is not confined to
partition transforms: a
+ * {@link ScalarFunction} or an {@link AggregateFunction} is held by the
expression Spark builds
+ * for an ordinary call to it, so the same {@code equals} decides whether
two such calls are the
+ * same expression, and Spark may then evaluate one where the query asked
for the other.
+ * <p>
+ * Whatever is compared must be stable across {@code bind} calls, and {@code
hashCode} must agree
+ * with {@code equals}, since Spark puts these expressions in hash-based
collections.
+ */
+ @Override
+ boolean equals(Object other);
Review Comment:
its definitely correct but its a bit long and difficult to parse, if a
connector is just trying to implement a function. (from the point of view of
someone not so experienced in Spark)
Can this work:
```
/**
* Implementations SHOULD override {@link Object#equals(Object)} and
* {@link Object#hashCode()}.
* <p>
* Without a stable {@code equals}/{@code hashCode}, Spark can miss
* optimizations such as:
* <ul>
* <li>keeping a union's keyed partitioning</li>
* <li>retaining a reported ordering that matches the partitioning</li>
* <li>reusing identical bucketed scans</li>
* <li>recognizing two identical subplans</li>
* </ul>
* Missed matches cost performance only, never correctness.
* <p>
* Compare whatever state affects behaviour, and keep it stable across
* {@code bind} calls. {@link #canonicalName()} alone is not always enough.
* Two functions that can produce different values must not compare equal.
* {@code hashCode} must agree with {@code equals}.
* <p>
* If this method is overridden, {@link #canonicalName()} should be
overridden
* as well so equal functions share the same name.
*/
@Override
boolean equals(Object other);
/**
* Must agree with {@link #equals(Object)}. See that method.
*/
@Override
int hashCode();
```
--
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]
