fix: dynamic template vector array is overridden by automatic dense_vector mapping#143733
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
|
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
carlosdelest
left a comment
There was a problem hiding this comment.
Thanks for submitting this PR so quickly!
I have a comment on the test, as it passes without your change. Can you modify it a bit so it fails when your fix is not present?
| body: | ||
| my_object: | ||
| my_float_field: [ | ||
| 159.1, 289.56, -128.7424, 145.9871, -164.0003, 86.4034, -89.6929, 257.9717, 131.6075, 67.9233, -144.8255, 223.8446, 77.3228, -210.1163, -139.4783, 12.6499, 15.4491, 108.3465, -189.3947, 178.2045, -187.5925, 184.5089, 77.3022, -202.7439, -13.4959, 115.9719, -139.4332, 196.7845, 104.7573, -156.7746, 166.9878, 68.3936, 159.8473, -141.4446, 21.1947, 186.5908, -209.6895, 68.6169, 44.1255, 147.4659, 56.5079, -179.7997, -85.1651, 11.4847, 124.1662, 96.2246, -178.6705, 85.5925, 205.3616, -16.4704, 172.4947, -115.2535, -58.1722, 94.4836, 34.6458, -70.1011, -58.8047, 149.9562, -37.8998, 196.9805, -169.3555, -163.9432, 188.5611, 214.8378, 29.3182, -24.8724, 152.9382, -109.4345, -123.6716, -8.2441, 64.5902, 27.8083, 40.8185, -94.3161, 58.1463, -138.7432, 24.6805, -88.7222, -11.2018, 206.6434, 201.9024, 87.3079, -3.2883, -60.2484, -109.5789, 105.5766, -116.6709, -17.7073, -71.5093, -75.2937, -176.8691, -146.4967, 53.7586, 199.5294, 55.9754, -48.7399, 82.2051, 135.2921, 22.4408, -116.4008, -33.7538, 29.7207, 6.3692, -97.5768, -12.7982, -200.9331, -62.2743, 81.0843, 136.2247, 150.2565, 139.6838, 155.2657, -25.7447, 198.5955, 18.8099, 46.9014, -60.2672, 136.4801, 171.8966, 172.5842, 13.9123, 75.8386, -64.2444, -48.1964, 135.9685, 7.4927, -40.6424, -76.8922 |
There was a problem hiding this comment.
I'm afraid this test passes without the updated logic.
You may need to create a specific dynamic template that matches ".dense_vector" (with the dot after the wildcard in ".dense_vector") like in your original issue.
There was a problem hiding this comment.
Thanks for the review. I have setup the environment and run the test on my local. It fails as expected without the fix. And passes with the fix. Could you have a look again 🙏
🔴 red
Test failing without the patch on server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java, saying got: dense_vector, expected: float
short output:
$ ./gradlew :rest-api-spec:yamlRestTest \
--tests "org.elasticsearch.test.rest.ClientYamlTestSuiteIT" \
-Dtests.method="test {yaml=search.vectors/60_dense_vector_dynamic_mapping/*}"
...
ClientYamlTestSuiteIT > test > test {yaml=search.vectors/60_dense_vector_dynamic_mapping/We respect values set in dynamic_templates for floats and dense vectors} FAILED
java.lang.AssertionError: java.lang.AssertionError:
Expected: "float"
but: was "dense_vector"
...full output:
./gradlew :rest-api-spec:yamlRestTest \
--tests "org.elasticsearch.test.rest.ClientYamlTestSuiteIT" \
-Dtests.method="test {yaml=search.vectors/60_dense_vector_dynamic_mapping/*}"
=======================================
Elasticsearch Build Hamster says Hello!
Gradle Version : 9.4.0
OS Info : Mac OS X 26.3 (aarch64)
JDK Version : 21.0.7+6-LTS (Eclipse Temurin)
JAVA_HOME : /Users/some-user-name/.gradle/jdks/eclipse_adoptium-21-aarch64-os_x.2/jdk-21.0.7+6/Contents/Home
Random Testing Seed : D1C269F1868D356A
In FIPS 140 mode : false
=======================================
> Task :server:compileJava
> Task :rest-api-spec:yamlRestTest
WARNING: Using incubator modules: jdk.incubator.vector
REPRODUCE WITH: ./gradlew ":rest-api-spec:yamlRestTest" --tests "org.elasticsearch.test.rest.ClientYamlTestSuiteIT" -Dtests.method="test {yaml=search.vectors/60_dense_vector_dynamic_mapping/We respe
ct values set in dynamic_templates for floats and dense vectors}" -Dtests.seed=D1C269F1868D356A -Dtests.locale=ksf-Latn-CM -Dtests.timezone=Europe/Zagreb -Druntime.java=25
ClientYamlTestSuiteIT > test > test {yaml=search.vectors/60_dense_vector_dynamic_mapping/We respect values set in dynamic_templates for floats and dense vectors} FAILED
java.lang.AssertionError: java.lang.AssertionError:
Expected: "float"
but: was "dense_vector"
at __randomizedtesting.SeedInfo.seed([D1C269F1868D356A:5996562B28715892]:0)
at org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase$ESClientYamlSuiteErrorCollector.verify(ESClientYamlSuiteTestCase.java:590)
at org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase.test(ESClientYamlSuiteTestCase.java:504)
at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
at java.base/java.lang.reflect.Method.invoke(Method.java:565)
at com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1763)
at com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:946)
at com.carrotsearch.randomizedtesting.RandomizedRunner$9.evaluate(RandomizedRunner.java:982)
at com.carrotsearch.randomizedtesting.RandomizedRunner$10.evaluate(RandomizedRunner.java:996)
at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
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:390)
at com.carrotsearch.randomizedtesting.ThreadLeakControl.forkTimeoutingTask(ThreadLeakControl.java:843)
at com.carrotsearch.randomizedtesting.ThreadLeakControl$3.evaluate(ThreadLeakControl.java:490)
at com.carrotsearch.randomizedtesting.RandomizedRunner.runSingleTest(RandomizedRunner.java:955)
at com.carrotsearch.randomizedtesting.RandomizedRunner$5.evaluate(RandomizedRunner.java:840)
at com.carrotsearch.randomizedtesting.RandomizedRunner$6.evaluate(RandomizedRunner.java:891)
at com.carrotsearch.randomizedtesting.RandomizedRunner$7.evaluate(RandomizedRunner.java:902)
at org.elasticsearch.test.cluster.local.DefaultLocalElasticsearchCluster$1.evaluate(DefaultLocalElasticsearchCluster.java:50)
at org.elasticsearch.cluster.metadata.TemplateDecoratorRule$1.evaluate(TemplateDecoratorRule.java:48)
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:390)
at com.carrotsearch.randomizedtesting.ThreadLeakControl.lambda$forkTimeoutingTask$0(ThreadLeakControl.java:850)
at java.base/java.lang.Thread.run(Thread.java:1474)
Caused by:
java.lang.AssertionError:
Expected: "float"
but: was "dense_vector"
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
at org.junit.Assert.assertThat(Assert.java:964)
at org.junit.Assert.assertThat(Assert.java:930)
at org.elasticsearch.test.rest.yaml.section.MatchAssertion.doAssert(MatchAssertion.java:100)
at org.elasticsearch.test.rest.yaml.section.Assertion.execute(Assertion.java:68)
at org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase.lambda$executeSection$3(ESClientYamlSuiteTestCase.java:536)
at org.junit.rules.ErrorCollector.checkSucceeds(ErrorCollector.java:90)
at org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase.executeSection(ESClientYamlSuiteTestCase.java:534)
at org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase.test(ESClientYamlSuiteTestCase.java:502)
... 42 more
Tests with failures:
- org.elasticsearch.test.rest.ClientYamlTestSuiteIT.test {yaml=search.vectors/60_dense_vector_dynamic_mapping/We respect values set in dynamic_templates for floats and dense vectors}
16 tests completed, 1 failed
> Task :rest-api-spec:yamlRestTest FAILED
[Incubating] Problems report is available at: file:///Users/some-user-name/ghq/github.com/elastic/elasticsearch/build/reports/problems/problems-report.html
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':rest-api-spec:yamlRestTest'.
> There were failing tests. See the report at: file:///Users/some-user-name/ghq/github.com/elastic/elasticsearch/rest-api-spec/build/reports/tests/yamlRestTest/index.html
BUILD FAILED in 31s
211 actionable tasks: 14 executed, 197 up-to-date🟢 green
And it passes with the patch:
$ ./gradlew :rest-api-spec:yamlRestTest \
--tests "org.elasticsearch.test.rest.ClientYamlTestSuiteIT" \
-Dtests.method="test {yaml=search.vectors/60_dense_vector_dynamic_mapping/*}"
=======================================
Elasticsearch Build Hamster says Hello!
Gradle Version : 9.4.0
OS Info : Mac OS X 26.3 (aarch64)
JDK Version : 21.0.7+6-LTS (Eclipse Temurin)
JAVA_HOME : /Users/some-user-name/.gradle/jdks/eclipse_adoptium-21-aarch64-os_x.2/jdk-21.0.7+6/Contents/Home
Random Testing Seed : E7AD85EC23A7290
In FIPS 140 mode : false
=======================================
> Task :server:compileJava
> Task :rest-api-spec:yamlRestTest
WARNING: Using incubator modules: jdk.incubator.vector
[Incubating] Problems report is available at: file:///Users/some-user-name/ghq/github.com/elastic/elasticsearch/build/reports/problems/problems-report.html
BUILD SUCCESSFUL in 32s
211 actionable tasks: 14 executed, 197 up-to-date
Consider enabling configuration cache to speed up this build: https://docs.gradle.org/9.4.0/userguide/configuration_cache_enabling.html| if (applyMatchingTemplate(context, name, matchType, dateFormatter)) { | ||
| context.markFieldAsAppliedFromTemplate(name); | ||
| String fullFieldName = context.path().pathAsText(name); | ||
| context.markFieldAsAppliedFromTemplate(fullFieldName); |
There was a problem hiding this comment.
I checked the codebase again, this markFieldAsAppliedFromTemplate() method is only used for the automatic dense_vector conversion context.
It is not used anywhere else, so changing name to fullFieldName should not have any other unexpected side effect.
|
@giga811 My bad, you're right - the new test does not pass without the fix. Thanks for checking again! |
carlosdelest
left a comment
There was a problem hiding this comment.
LGTM, thank you for your contribution @giga811 !
I've added the changelog for release notes, and will look into backporting the fix.
|
@elasticsearchmachine test this please |
|
@elasticsearchmachine test this please |
1 similar comment
|
@elasticsearchmachine test this please |
|
bwc tests are failing of course - I added a test feature to guard the new test in 53971bd |
|
@elasticsearchmachine test this please |
|
@elasticsearchmachine run elasticsearch-ci/part-3 |
|
@elasticsearchmachine test this please |
💔 Backport failed
You can use sqren/backport to manually backport by running |
…locations * upstream/main: (126 commits) Update KnnIndexTester to use more settings from datasets (elastic#143869) fix: dynamic template vector array is overridden by automatic dense_vector mapping (elastic#143733) ES|QL: Don't reuse the same alias for _fork column (elastic#143909) Close and initialize clients after each node upgrade in logsdb rolling upgrade tests. (elastic#143823) ESQL: Added GroupedTopNOperator for LIMIT BY, compute only (elastic#143476) Handle views in ResolveIndexAction (elastic#143561) Improve reindex rethrottle API in stateless (elastic#143771) Use a copy of the SearchExecutionContext for each Percolator execution (elastic#142765) Log the stacktrace when we encounter a deprecation warning for `default_metric` (elastic#143929) ESQL: evaluate ReferenceAttributes to potentially FieldAttributes for full-text functions restriction (elastic#143893) Add ClusterStateSerializationStats Serializatation Tests (elastic#142703) Adds Coordination Diagnostics Tests (elastic#142709) Upgrade Elasticsearch to Apache Lucene 10.4 (elastic#141882) ESQL: Add configurable bracket-based multi-value support for CSV reader (elastic#143890) time series es819 binary dv use up to a 1mb block size (elastic#143049) Dynamically enable / disable plugins in correspondence to stateless mode. (elastic#142147) ES|QL: Implement first/last_over_time for tdigest (elastic#143832) Document CHANGE_POINT limitation (elastic#143877) Fix OperationsOnSeqNoDisabledIndicesIT (elastic#143892) [Test] Test that sequence numbers are not pruned with retention lease (elastic#143825) ...
…ector mapping (elastic#143733) (cherry picked from commit c8f1d0e) # Conflicts: # server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java
…ector mapping (elastic#143733) (cherry picked from commit c8f1d0e) # Conflicts: # server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
…ector mapping (elastic#143733) (cherry picked from commit c8f1d0e) # Conflicts: # server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java
…ense_vector mapping (elastic#143733) Backport of elastic#143733 to 9.1.
fixes: #143732
More details are in the issue above.
This essentially fixes issue where dynamic templating is overridden to dense_vector by its automatic assignment logic here:
elasticsearch/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java
Line 823 in cc5852b
Previously the
markFieldAsAppliedFromTemplateis marking on field name(not the full path).But the dense vector assigning is checked on full path.
This PR is to do the marking based on full path instead of field name.
related PR: #98512