szehon-ho commented on code in PR #58002:
URL: https://github.com/apache/spark/pull/58002#discussion_r3807143300
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/functions/BoundFunction.java:
##########
@@ -90,6 +90,41 @@ default boolean isDeterministic() {
* functions in other catalogs. For example, many catalogs may define a
"bucket" function with a
* different implementation. Adding context, like
"com.mycompany.bucket(string)", is recommended
* to avoid unintentional collisions.
+ * <p>
+ * Two functions that partition data DIFFERENTLY must NOT return the same
name. They are
+ * indistinguishable to Spark, which can then treat unrelated data as
co-partitioned and produce
+ * a join with wrong results, so include whatever tells them apart --
argument types, for
+ * instance -- in the name. This is the only requirement here that guards
results rather than
+ * performance.
+ * <p>
+ * An overriding implementation should keep the name stable across calls,
and across separate
+ * {@code bind} calls for the same function: Spark binds a function afresh
every time it converts
+ * a partitioning or ordering reported by a source, so when the two
instances are not the same
+ * object it has only this name to relate them by when deciding whether two
sides of a join are
+ * co-partitioned, and so whether a storage-partitioned join can avoid a
shuffle. An unstable name
+ * is not unsafe -- it just makes overriding pointless, since it relates
nothing to anything.
+ * <p>
+ * This name answers only that question -- whether two transforms are the
same PARTITION FUNCTION,
+ * ignoring their arguments, which a join needs because it compares {@code
bucket(4, left.id)}
+ * against {@code bucket(4, right.id)}. It is deliberately not a complete
identity for the bound
+ * function: it says nothing about {@link #inputTypes()}, {@link
#resultType()},
+ * {@link #isResultNullable()}, {@link #isDeterministic()}, a {@link
ReducibleFunction}'s
+ * reducers, or any other state the implementation carries. For the separate
question of whether
+ * two partition transform expressions are the same expression, see {@link
#equals(Object)}.
+ * <p>
+ * The two questions are related in one direction: this name is the COARSER
of the two, so two
+ * functions that compare equal must return the same canonical name, while
the same name does not
+ * make them equal. An implementation that overrides {@link #equals(Object)}
should therefore
+ * override this method as well -- keeping the default while claiming two
instances are equal says
+ * they are the same expression but not the same partition function, which
costs it exactly the
+ * co-partitioning the name exists to enable.
+ * <p>
+ * Leaving the default in place is allowed: it opts the function out of
being recognized as
+ * equivalent to anything, which costs optimizations and nothing else. Two
instances are never
+ * equivalent, so Spark forgoes co-partitioning between them; and because
the default is not even
+ * stable across two calls on ONE instance, such a function is not the same
partition function as
+ * itself, so a partitioning it takes part in can cost a shuffle that a
stable name would have
+ * avoided. Overriding it is the way out of all of that.
Review Comment:
can it be shorter like:
```
* <p>
* Two functions that partition data differently must not return the same
name;
* Spark may otherwise treat them as co-partitioned.
* An override should return a stable name across {@code bind} calls.
* Equal functions must share the same canonical name; the reverse is not
true.
* For whether two transform expressions are the same expression, see
* {@link #equals(Object)}.
```
--
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]
