SEPURI-SAI-KRISHNA commented on code in PR #57746:
URL: https://github.com/apache/spark/pull/57746#discussion_r3730137971
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/VectorFunctionImplUtils.java:
##########
@@ -90,19 +93,22 @@ public static Float vectorCosineSimilarity(ArrayData left,
ArrayData right, UTF8
if (left.isNullAt(i) || right.isNullAt(i)) {
return null;
}
- float a = left.getFloat(i);
- float b = right.getFloat(i);
+ double a = left.getFloat(i);
+ double b = right.getFloat(i);
dotProduct += a * b;
norm1Sq += a * a;
norm2Sq += b * b;
i++;
}
- float normProduct = (float) Math.sqrt(norm1Sq * norm2Sq);
- if (normProduct < Float.MIN_NORMAL) {
+ // `norm1Sq * norm2Sq` cannot overflow in double precision: both factors
are bounded by
Review Comment:
Good catch, the bound as written was unconditional. Reworded to scope it to
finite elements and to
say what happens otherwise:
```java
// For vectors of finite elements, `norm1Sq * norm2Sq` cannot overflow in
double precision:
// both factors are bounded by MAX_ROUNDED_ARRAY_LENGTH * Float.MAX_VALUE^2,
so their product
// stays well below Double.MAX_VALUE. An element that is already infinite
makes the product
// infinite and the result NaN, exactly as it did before the accumulators
were widened.
```
An infinite element cannot produce a finite dot product either -- it is
multiplied by either a
non-zero element (giving +/-Infinity) or by zero (giving NaN) -- so
`dotProduct / normProduct`
is NaN in every such case, both before and after this change.
I also added golden-file coverage for infinite elements so the behaviour is
pinned rather than just
asserted in a comment:
| query | result |
| --- | --- |
| `vector_cosine_similarity(array(float('inf'), 1.0F), array(1.0F, 1.0F))` |
`NaN` |
| `vector_inner_product(array(float('inf'), 1.0F), array(1.0F, 1.0F))` |
`Infinity` |
| `vector_l2_distance(array(float('inf'), 1.0F), array(0.0F, 0.0F))` |
`Infinity` |
| `vector_norm(array(float('inf'), 1.0F), 2.0F)` | `Infinity` |
| `vector_normalize(array(float('inf'), 1.0F), 2.0F)` | `[NaN,0.0]` |
All five are unchanged from before this PR.
--
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]
