Add checks in KNNVectorField / KNNVectorQuery to only allow non-null, non-empty and finite vectors

[jbellis]

This PR adds argument checking to constructors of fields and query, so all vector values must be finite.

[jbellis]

(Rebased to main)

[rmuir]

Sorry, I am opposed to this PR. Checking if vectors are finite needs to be done at eg index time (in the xxField class), not one very similarity operation. And widening intermediate calculations to double will kill the performance of vectorized impl, which should stay consistent with it.

These functions are performance sensitive, not the place for this.

[uschindler]

Hi,
there are multiple problems:

• we have no benchmark. The code introduced here will most likely prevent SIMD instructions introduced by Hotspot. This code is very performance sensitive!
• the SIMD implementation in PanamaVectorUtilProvider does not use doubles. It allready differs from its results and this would change the behaviour completely.
• checking for finite vectors should not be done at query runtime and not at indexing time.

I am strongly -1 to apply anything like that!

The big question here: What are "large dimension" vectors? If the cosine gets NaN it must be large. As discussed in other issue #11507 and PR #12306, there is a limit of 1024 dimensions in Lucene - for a good reason! If you changed the constant in the codec and indexed shitillions of dimensions, it is not Lucene's problem. Those functions here are tested to behave fine with up to 1024 dimensions.

BTW, this PR/issue in an argument to not raise the limit in Lucene to more than 1024 or 2048. Each dimension more adds more rounding errors!

There is no need to make the cosine of identical vector return exactly 1 (it can never be exactly one). This is for scoring and sorting documents, the actual value does not matter. This is also why we do not care about the new Panama Vector based implementation returns different values because of different order of execution and rounding.

[benwtrent]

Even with removing the checkFinite for user friendly errors, I would like to see any benchmarking.

I honestly don't know if summing into a double will make things slower or not. But if its slower at all, I would rather not bother with it.

This is a hot path, and we don't want to slow it down at all for exceptional cases.

[benwtrent]

it is indeed the caller's responsibility to ensure the passed vectors have valid values. We cannot do validations, even in exceptional cases.

[rmuir]

I would move the isFinite check to KnnFloatVectorField.setVectorValue:

lucene/lucene/core/src/java/org/apache/lucene/document/KnnFloatVectorField.java

Line 152 in 95e2cfc

*/

I would fix KnnFloatVectorField ctor to actually call setVectorValue, as currently the first value passed in bypasses all checks:

lucene/lucene/core/src/java/org/apache/lucene/document/KnnFloatVectorField.java

Line 104 in 95e2cfc

fieldsData = vector;

This way the check only happens at index-time once, and not on every comparison.

[jbellis]

I'm pretty sure that a single check for isFinite is going to be negligible compared to the cost of doing the computation -- checking the vectors only happens if that single check fails. We "know" that they must be finite b/c of index-time check but since we only do the full check if we lost so much precision during the computation that we get NaN/Infinity back, it seems reasonable to me to double-check.

I'll put together a benchmark to see how much the double math slows things down. Is there a threshold at which we're okay paying a small performance cost for improved precision?

[uschindler]

I'm pretty sure that a single check for isFinite is going to be negligible compared to the cost of doing the computation -- checking the vectors only happens if that single check fails. We "know" that they must be finite b/c of index-time check but since we only do the full check if we lost so much precision during the computation that we get NaN/Infinity back, it seems reasonable to me to double-check.

I'll put together a benchmark to see how much the double math slows things down. Is there a threshold at which we're okay paying a small performance cost for improved precision?

Please do not forget the new vectorized code! The default provider is not the only one!

[jbellis]

Thanks for the reminder. Is working with the default provider a reasonable first step, or is there something about the vectorized code that prevents this entirely?

[rmuir]

Please do not forget the new vectorized code! The default provider is not the only one!

That's the issue. I don't care about the performance of the scalar one. But the semantics must match.

If we widen to double, now we can only process half the elements at once: calling F2D on both sides, then expanding to 64-bit vectors before multiplication/addition. this conversion is expensive and expanding is the only choice (working in "parts" is too slow). To avoid heavy slowdowns, we have to do tricks that will make the code look like the integer versions.

I don't think we should do this: let's keep this stuff simple, it is too hot. But the safety checks can be added elsewhere.

[rmuir]

widening to double is guaranteed to be at least a 2x slowdown for the vector code since only half the elements can be processed in the same time.

[jbellis]

okay, thanks. how about this version then that just does the sanity check?

[uschindler]

Why do we need the sanity check here?

You can do the sanity check directly in VectorUtil, no need to do it in any of both providers. The code in VectorUtil also does the argument checks. Move all code that cannot be directly vectorized to VectorUtil class. As the checks just do isFinite on return value, there's the correct place, check return value of provider.

Maybe do it as assertion only?

[rmuir]

The check belongs in KnnVectorField, not here.

[rmuir]

See comment: #12281 (comment)

[uschindler]

The check belongs in KnnVectorField, not here.

Yes and no. In KnnVectorField it needs to scan through all vector components to check that it is not finite.

The check here that isFinite is only on return value of calculation. If one component is inFinite the result of calculation is infinite.

The code only scans through vector to give a good exception.

[uschindler]

My proposal would be:

• Revert all changes to all providers. The change to double is out of discussion now. We have some rounding errors for large vectors, but this has nothing to do with finite checks. We are a search engine that calculates float scores. The implementations in Panama and the unvectorized default one are different anyways. For a vector search engine it is just important to return a score, if it is precisie does not matter so much, it is only used for sorting
• Add a static method VectorUtil#checkFinite(float[] vector) that throws IllegalArgumentException (and does the missing null check gratis). Theres no need to move it to the providers. The autovectorizer can handle it.
• Call this method from both ctors in KNNVectorField. The ctors are currently missing the checks that are mentioned in the javadocs. E.g., theres no null check, which is a bug.

[uschindler]

Also change the title of this PR to be "add checks in KNNVectorField to only allow non-null, non-empty and finite vectors".

[uschindler]

see my proposal

[jbellis]

Thanks! I've made the proposed changes.

It looks like KNNFloatVectorField.createType is doing the null check for both constructors, so I added checkFinite there.

[rmuir]

This patch is still mixing concerns and calling nonsense checks in similarity functions. Is this a troll?

-1

[uschindler]

If the index no longer has infinite values (checked during indexing), there's really no reason to call isFinite from the similarity functions.

If there's a query vector, it should also be checked, but in the query's ctor:

lucene/lucene/core/src/java/org/apache/lucene/search/KnnFloatVectorQuery.java

Line 73 in 113c4ad

this.target = target;

[uschindler]

Finally I just want to understand that sentence in the description:

Cosine of two equal vectors is exactly 1, but we're losing too much precision on large-dimension vectors and ending up with NaN.

How can that be? The cosine can only get infinite / NaN if any of the arguments are infinite. Do you have an example where it gets NaN with finite vectors (overflow?).

I agree with this PR, if you make the query also check its input and remove the finity checks from the similarity functions.

P.S.: We also do not allow other queries to return scores of NaN, but in the same way like proposed, we check inputs of queries (like invalid or negative boosts). We do not check for invalid float scores on each scored document. This is what you want to do here and that's not accepted.

I would agree to add it as assertion to trigger bugs in our testing of (possibly new) queries. But for that it is enough to do assert Float.isFinite(result), nothing more (no analysis of vector arguments).

[jbellis]

it is enough to do assert Float.isFinite(result), nothing more (no analysis of vector arguments).

I don't understand we wouldn't want to include the analysis. This is a result that we expect "can't happen" so it will be extremely valuable to know which scenario it is:

• A mistake was introduced into validation of parameters
• A new code path happening that didn't do validation properly
• Somehow the float32 math didn't work the way we thought it should

Basically impossible to tell which scenario without knowing the vector arguments, and there is no cost to having that code on the happy path of everything working since it is only called if the assert fails.

[uschindler]

Here is the example vector: https://gist.github.com/jbellis/8a9c42ee8ecdf603021498deddfcc243

This is an example where the result is not expected. But how would throwing an exception help? A query searching for the exact same vector (cosine=1) should return a valid score. So actually this is a different issue.

The original PR did not have that vector - or as it force merged away? The test in your latest PR was just about a 3 dim vector with infinity / NaN.

I suspect, that with double vector math this can't be prevented, too. How does the query fail downstream?

[jbellis]

I force-pushed it away. Here is the original version: https://github.com/jbellis/lucene/tree/hnsw-nan-og

I ordered the commits this way specifically so that it was clear that switching to double math solved the issue with exploding to NaN.

[uschindler]

Hi,
so this looks like 2 different issues:

• incorrect validation of query and indexed field contents.
• some vectors cause NaN when the cosine is calculated. I am not sure how this can happen, because cosine should only be NaN when the argument of the square root is negative or NaN. To me this indeed looks like a rounding error. Maybe we should guard the cosine to not allow negative numbers. This can only happen for special cases, so maybe Math.abs() is fine to work around. Actually the cosine should never be outside [-1 .. 1].

I think instead of throwing an exception (which would be crazy to the end user as a query or the indexing process would suddenly fail with unexpected arguments, although the input vectors are valid), we should make sure that due to rounding errors the argument of the cosine cannot get negative.

Can we fix the cosine problem due to negative sqrt in a separate PR and let this one alone?

[uschindler]

I think I know why it happens, in both providers we calculate the cosine like that:

lucene/lucene/core/src/java/org/apache/lucene/util/VectorUtilDefaultProvider.java

Line 105 in ef35e6e

return (float) (sum / Math.sqrt(norm1 * norm2));

When we calculate the argument for sqrt it may get negative for one of those reasons:

• norm1 or norm2 is negative
• we don't cast both factors to double before multiplication, so it overflows the exponent of the float

The other similarity functions are not affected, but we can possibly add some guard her by casting both floats to double before multiplying them.

[uschindler]

I debugged through it: The vector causes norm1 and norm2, as well as sum to get Infinity. Infinity/Infinity results in NaN.

So it is not caused by sqrt. In general you are right this might happen with any vector if the exponent overflows while summing up the component squares.

I am not sure how to work around that. Maybe it should return 1, if the sqrt gets infinity?

[benwtrent]

This is a solid change. We should not allow infinite floats, and I am surprised that we didn't have null checks already 🤦 .

Thank you for moving the "isFinite" checks and only verifying responses if assertions are enabled.

[uschindler]

I think the related issue found with some vectors creating an NaN cosine (happens when the floats are too large by exponent and the result gets infinity after multiplication) is a separate one. I think we should open an issue.

This vector causes the assert to trigger:

  public void testCosineNaN() {
    final float[] v = new float[] { 1.E31f };
    assertEquals(1f, VectorUtil.cosine(v, v), DELTA);
  }

I figured out that I did not check explicitely for "empty" vectors. I may add a test for that.

[uschindler]

I think we could decide to also disallow such vectors. If the square of one its components gets infinity it should maybe also be rejected. What do you think?

float y= 1e31f * 1e31f;
System.err.println(y); // prints "Infinity"

Maybe we change the infinite check to use square?

[uschindler]

Actually with the current similarity methods we should make sure that for each vector component v, the following is true: Float.isFinite(v * v * vector.length).

This is quite easy to implement in the isFinite check!

In my opinion, this is a good compromise. This will help with all different similarity function, as all of them generally sum up the squares of components (or like that). If somebody indexes or queries a vector that's too large, we should reject it from beginning. Maybe with a better error message than currently.

Otherwise we would really need to go and expand to double because it allows larger exponents. The issue here is not precission of the float (that is fine), the problem is the limitation on the exponent.

[uschindler]

I modified the function like that, but have not yet committed:

  /**
   * Checks if a float vector only has finite components and the square of its components multiplied by vector dimensions is finite.
   *
   * @param v bytes containing a vector
   * @return the vector for call-chaining
   * @throws IllegalArgumentException if any component of vector violates the contract
   */
  public static float[] checkFinite(float[] v) {
    for (int i = 0; i < v.length; i++) {
      float f = v[i] * v[i] * v.length;
      if (!Float.isFinite(f)) {
        throw new IllegalArgumentException("non-finite or too large (with respect to dimensions) value at vector[" + i + "]=" + v[i]);
      }
    }
    return v;
  }

What do you think?

[uschindler]

The above check should auto-vectorize in Hotspot, so the check during indexing/searching should be cheap.

[benwtrent]

@uschindler I think this change (v[i] * v[i] * v.length) is getting complicated. It really makes me think about if we should do any complex infinite checking other than isFinite(float). For example, this check would prevent vectors that may be valid for squareDistance.

If a specific similarity has NaN issues, than a more complicated isFinite check should account for those. But, this then opens the door to how KnnFloatVectorQuery would even know about the similarity used, or how that could be checked at all.

I would prefer us doing the simple float value validation for now and we open an issue to see if we can get a better API for checking similarity infinity checks (e.g. squareDistance vs cosine) and if we should have those at all.

[uschindler]

That would be my plan! Let's open a new issue and discuss that there.

So we should merge this PR for now. Before doing that I will only add the "vector size>0" check in the API, because that may be missing at some places. A zero length vector also causes havoc.

[uschindler]

Hi, I added the dimension check to the constructor which uses a predefined field type. For the query it can't be done in the constructor, as we do not know the field type. The query will fail later, so an explicit check is not needed.

I think this is ready to be merged. @jbellis we should open a separate issue to make sure that we find a solution for vectors where the float overflows (infinity) leading to infinite scores. I strongly disagree to use double math, we should maybe have some documentation or checks like proposed above. We could also enforce it, but that's harder to decide! So it should be separate issue.

It is very easy to reproduce with a single-dimension vector.

[jbellis]

SGTM, thank you for taking the lead on investigating the root cause of the NaNs!

[benwtrent]

Good catch!

[uschindler]

I did not see any slowdowns in last night @mikemccand benchmark caused by the check during indexing and on building the query.