Skip to content

Fix offsets not recording duplicate values #125354

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 21, 2025
+102 −19
Merged

Fix offsets not recording duplicate values #125354

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9c966da
Add test demonstrating missing duplicate issue
jordan-powers Mar 20, 2025
29b68c4
Convert values to sortable longs before recording offsets
jordan-powers Mar 20, 2025
c4a1371
Document NumberType#toSortableLong
jordan-powers Mar 21, 2025
e99d8d8
Merge remote-tracking branch 'upstream/main' into fix-offsets-missing…
jordan-powers Mar 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 46 additions & 1 deletion server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,11 @@ public void addFields(LuceneDocument document, String name, Number value, boolea
}
}

@Override
public long toSortableLong(Number value) {
return HalfFloatPoint.halfFloatToSortableShort(value.floatValue());
}

@Override
public IndexFieldData.Builder getFieldDataBuilder(MappedFieldType ft, ValuesSourceType valuesSourceType) {
return new SortedDoublesIndexFieldData.Builder(
Expand Down Expand Up @@ -622,6 +627,11 @@ public void addFields(LuceneDocument document, String name, Number value, boolea
}
}

@Override
public long toSortableLong(Number value) {
return NumericUtils.floatToSortableInt(value.floatValue());
}

@Override
public IndexFieldData.Builder getFieldDataBuilder(MappedFieldType ft, ValuesSourceType valuesSourceType) {
return new SortedDoublesIndexFieldData.Builder(
Expand Down Expand Up @@ -772,6 +782,11 @@ public void addFields(LuceneDocument document, String name, Number value, boolea
}
}

@Override
public long toSortableLong(Number value) {
return NumericUtils.doubleToSortableLong(value.doubleValue());
}

@Override
public IndexFieldData.Builder getFieldDataBuilder(MappedFieldType ft, ValuesSourceType valuesSourceType) {
return new SortedDoublesIndexFieldData.Builder(
Expand Down Expand Up @@ -891,6 +906,11 @@ public void addFields(LuceneDocument document, String name, Number value, boolea
INTEGER.addFields(document, name, value, indexed, docValued, stored);
}

@Override
public long toSortableLong(Number value) {
return INTEGER.toSortableLong(value);
}

@Override
Number valueForSearch(Number value) {
return value.byteValue();
Expand Down Expand Up @@ -1009,6 +1029,11 @@ public void addFields(LuceneDocument document, String name, Number value, boolea
INTEGER.addFields(document, name, value, indexed, docValued, stored);
}

@Override
public long toSortableLong(Number value) {
return INTEGER.toSortableLong(value);
}

@Override
Number valueForSearch(Number value) {
return value.shortValue();
Expand Down Expand Up @@ -1206,6 +1231,11 @@ public void addFields(LuceneDocument document, String name, Number value, boolea
}
}

@Override
public long toSortableLong(Number value) {
return value.intValue();
}

@Override
public IndexFieldData.Builder getFieldDataBuilder(MappedFieldType ft, ValuesSourceType valuesSourceType) {
return new SortedNumericIndexFieldData.Builder(
Expand Down Expand Up @@ -1358,6 +1388,11 @@ public void addFields(LuceneDocument document, String name, Number value, boolea
}
}

@Override
public long toSortableLong(Number value) {
return value.longValue();
}

@Override
public IndexFieldData.Builder getFieldDataBuilder(MappedFieldType ft, ValuesSourceType valuesSourceType) {
return new SortedNumericIndexFieldData.Builder(
Expand Down Expand Up @@ -1506,6 +1541,13 @@ public abstract void addFields(
boolean stored
);

/**
* For a given {@code Number}, returns the sortable long representation that will be stored in the doc values.
* @param value number to convert
* @return sortable long representation
*/
public abstract long toSortableLong(Number value);
Copy link
Member

@martijnvg martijnvg Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we have a good reason to have this method :).
Maybe add documentation on this method to explain why we need to method?


public FieldValues<Number> compile(String fieldName, Script script, ScriptCompiler compiler) {
// only implemented for long and double fields
throw new IllegalArgumentException("Unknown parameter [script] for mapper [" + fieldName + "]");
Expand Down Expand Up @@ -2140,7 +2182,10 @@ protected void parseCreateField(DocumentParserContext context) throws IOExceptio
}
if (offsetsFieldName != null && context.isImmediateParentAnArray() && context.canAddIgnoredField()) {
if (value != null) {
context.getOffSetContext().recordOffset(offsetsFieldName, (Comparable<?>) value);
// We cannot simply cast value to Comparable<> because we need to also capture the potential loss of precision that occurs
// when the value is stored into the doc values.
long sortableLongValue = type.toSortableLong(value);
context.getOffSetContext().recordOffset(offsetsFieldName, sortableLongValue);
} else {
context.getOffSetContext().recordNull(offsetsFieldName);
}
Expand Down
24 changes: 24 additions & 0 deletions ...a/org/elasticsearch/index/mapper/HalfFloatSyntheticSourceNativeArrayIntegrationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,32 @@

import org.apache.lucene.sandbox.document.HalfFloatPoint;

import static org.elasticsearch.xcontent.XContentFactory.jsonBuilder;

public class HalfFloatSyntheticSourceNativeArrayIntegrationTests extends NativeArrayIntegrationTestCase {

public void testSynthesizeArray() throws Exception {
var inputArrayValues = new Float[][] { new Float[] { 0.78151345F, 0.6886488F, 0.6882413F } };
var expectedArrayValues = new Float[inputArrayValues.length][inputArrayValues[0].length];
for (int i = 0; i < inputArrayValues.length; i++) {
for (int j = 0; j < inputArrayValues[i].length; j++) {
expectedArrayValues[i][j] = HalfFloatPoint.sortableShortToHalfFloat(
HalfFloatPoint.halfFloatToSortableShort(inputArrayValues[i][j])
);
}
}

var mapping = jsonBuilder().startObject()
.startObject("properties")
.startObject("field")
.field("type", getFieldTypeName())
.endObject()
.endObject()
.endObject();

verifySyntheticArray(inputArrayValues, expectedArrayValues, mapping, "_id");
}

@Override
protected String getFieldTypeName() {
return "half_float";
Expand Down
50 changes: 32 additions & 18 deletions server/src/test/java/org/elasticsearch/index/mapper/NativeArrayIntegrationTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,47 +259,61 @@ protected void verifySyntheticArray(Object[][] arrays) throws IOException {
}

protected void verifySyntheticArray(Object[][] arrays, XContentBuilder mapping, String... expectedStoredFields) throws IOException {
verifySyntheticArray(arrays, arrays, mapping, expectedStoredFields);
}

private XContentBuilder arrayToSource(Object[] array) throws IOException {
var source = jsonBuilder().startObject();
if (array != null) {
source.startArray("field");
for (Object arrayValue : array) {
source.value(arrayValue);
}
source.endArray();
} else {
source.field("field").nullValue();
}
return source.endObject();
}

protected void verifySyntheticArray(
Object[][] inputArrays,
Object[][] expectedArrays,
XContentBuilder mapping,
String... expectedStoredFields
) throws IOException {
assertThat(inputArrays.length, equalTo(expectedArrays.length));

var indexService = createIndex(
"test-index",
Settings.builder().put("index.mapping.source.mode", "synthetic").put("index.mapping.synthetic_source_keep", "arrays").build(),
mapping
);
for (int i = 0; i < arrays.length; i++) {
var array = arrays[i];

for (int i = 0; i < inputArrays.length; i++) {
var indexRequest = new IndexRequest("test-index");
indexRequest.id("my-id-" + i);
var source = jsonBuilder().startObject();
if (array != null) {
source.startArray("field");
for (Object arrayValue : array) {
source.value(arrayValue);
}
source.endArray();
} else {
source.field("field").nullValue();
}
source.endObject();
var expectedSource = Strings.toString(source);
indexRequest.source(source);
var inputSource = arrayToSource(inputArrays[i]);
indexRequest.source(inputSource);
indexRequest.setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE);
client().index(indexRequest).actionGet();

var expectedSource = arrayToSource(expectedArrays[i]);

var searchRequest = new SearchRequest("test-index");
searchRequest.source().query(new IdsQueryBuilder().addIds("my-id-" + i));
var searchResponse = client().search(searchRequest).actionGet();
try {
var hit = searchResponse.getHits().getHits()[0];
assertThat(hit.getId(), equalTo("my-id-" + i));
assertThat(hit.getSourceAsString(), equalTo(expectedSource));
assertThat(hit.getSourceAsString(), equalTo(Strings.toString(expectedSource)));
} finally {
searchResponse.decRef();
}
}

try (var searcher = indexService.getShard(0).acquireSearcher(getTestName())) {
var reader = searcher.getDirectoryReader();
for (int i = 0; i < arrays.length; i++) {
for (int i = 0; i < expectedArrays.length; i++) {
var document = reader.storedFields().document(i);
// Verify that there is no ignored source:
Set<String> storedFieldNames = new LinkedHashSet<>(document.getFields().stream().map(IndexableField::name).toList());
Expand Down