Fix equals and hashCode methods for KNNQuery and KNNQueryBuilder

[bugmakerrrrrr]

Description

The equals and hashCode implementations of KNNQuery and KNNQueryBuilder are missing some fields (indexName and filterQuery for KNNQuery, filter and ignoreUnmapped for KNNQueryBuilder) and are not using the correct array comparison/hash method.

The error in the relevant unit test is as follows:

java.lang.AssertionError: expected: org.opensearch.knn.index.query.KNNQueryBuilder<{
  "knn" : {
    "myvector" : {
      "vector" : [
        1.0,
        2.0,
        3.0,
        4.0
      ],
      "k" : 1,
      "filter" : {
        "term" : {
          "field" : {
            "value" : "value",
            "boost" : 1.0
          }
        }
      },
      "boost" : 1.0
    }
  }
}> but was: org.opensearch.knn.index.query.KNNQueryBuilder<{
  "knn" : {
    "myvector" : {
      "vector" : [
        1.0,
        2.0,
        3.0,
        4.0
      ],
      "k" : 1,
      "filter" : {
        "term" : {
          "field" : {
            "value" : "value",
            "boost" : 1.0
          }
        }
      },
      "boost" : 1.0
    }
  }
}>
	at __randomizedtesting.SeedInfo.seed([9EA5DFBEA12B6A22:F50BAC6E9E6B27AA]:0)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:120)
	at org.junit.Assert.assertEquals(Assert.java:146)
	at org.opensearch.knn.index.query.KNNQueryBuilderTests.testFromXContent_WithFilter(KNNQueryBuilderTests.java:128)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:64)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:564)
	at com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1750)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:938)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$9.evaluate(RandomizedRunner.java:974)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$10.evaluate(RandomizedRunner.java:988)
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
	at org.junit.rules.RunRules.evaluate(RunRules.java:20)
	at org.apache.lucene.tests.util.TestRuleSetupTeardownChained$1.evaluate(TestRuleSetupTeardownChained.java:48)
	at org.apache.lucene.tests.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:43)
	at org.apache.lucene.tests.util.TestRuleThreadAndTestName$1.evaluate(TestRuleThreadAndTestName.java:45)
	at org.apache.lucene.tests.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:60)
	at org.apache.lucene.tests.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:44)
	at org.junit.rules.RunRules.evaluate(RunRules.java:20)
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
	at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:368)
	at com.carrotsearch.randomizedtesting.ThreadLeakControl.forkTimeoutingTask(ThreadLeakControl.java:817)
	at com.carrotsearch.randomizedtesting.ThreadLeakControl$3.evaluate(ThreadLeakControl.java:468)
	at com.carrotsearch.randomizedtesting.RandomizedRunner.runSingleTest(RandomizedRunner.java:947)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$5.evaluate(RandomizedRunner.java:832)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$6.evaluate(RandomizedRunner.java:883)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$7.evaluate(RandomizedRunner.java:894)
	at org.apache.lucene.tests.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:43)
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
	at org.apache.lucene.tests.util.TestRuleStoreClassName$1.evaluate(TestRuleStoreClassName.java:38)
	at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40)
	at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40)
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
	at org.apache.lucene.tests.util.TestRuleAssertionsRequired$1.evaluate(TestRuleAssertionsRequired.java:53)
	at org.apache.lucene.tests.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:43)
	at org.apache.lucene.tests.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:44)
	at org.apache.lucene.tests.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:60)
	at org.apache.lucene.tests.util.TestRuleIgnoreTestSuites$1.evaluate(TestRuleIgnoreTestSuites.java:47)
	at org.junit.rules.RunRules.evaluate(RunRules.java:20)
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
	at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:368)
	at java.base/java.lang.Thread.run(Thread.java:832)

Issues Resolved

[List any issues this PR will resolve]

Check List

• [~] New functionality includes testing.
  • All tests pass
• [~] New functionality has been documented.
  • [~] New functionality has javadoc added
• Commits are signed as per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[jmazanec15]

Thanks @bugmakerrrrrr for the fix. Overall, it looks good to me. Could you add the fix to the changelog for 2.x? https://github.com/opensearch-project/k-NN/blob/main/CHANGELOG.md#bug-fixes-1

Also, did you run into a bug for this when running OpenSearch?

[codecov]

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (fcbfef1) 85.03% compared to head (cc9efc1) 84.88%.

Files	Patch %	Lines
...rg/opensearch/knn/index/query/KNNQueryBuilder.java	0.00%	1 Missing and 5 partials ⚠️
.../java/org/opensearch/knn/index/query/KNNQuery.java	16.66%	5 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1397      +/-   ##
============================================
- Coverage     85.03%   84.88%   -0.16%     
+ Complexity     1275     1274       -1     
============================================
  Files           167      167              
  Lines          5179     5186       +7     
  Branches        483      491       +8     
============================================
- Hits           4404     4402       -2     
- Misses          571      575       +4     
- Partials        204      209       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[martin-gaievski]

@bugmakerrrrrr thank you for your contribution.

I do have couple of asks:

• can you please list in PR description what exact fields were added to equals and hashCode comparing to existing implementation
• please confirm that the unit tests KNNQueryBuilderTests.java would fail without your additions to KNNQuery and KNNQueryBuilder

[bugmakerrrrrr]

Also, did you run into a bug for this when running OpenSearch?

No, I just find this bug when I read the code.

[martin-gaievski]

You don't need to specify prefix [Bug] , we use github labels attached to PR and categories in CHANGELOG file for this. I've corrected PR description, can you please make corresponding change to CHANGELOG

[jmazanec15]

LGTM