Skip to content

Fix StoredFieldsSpec#merge(...) issue #138306

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
+53 −3
Open

Fix StoredFieldsSpec#merge(...) issue #138306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ba798b0
Fix StoredFieldsSpec#merge(...) issue
martijnvg Nov 19, 2025
fbd0086
fix yaml test
martijnvg Nov 19, 2025
840a0d1
fix yaml test 2
martijnvg Nov 19, 2025
d912b09
fix yaml test 3
martijnvg Nov 20, 2025
6fc551c
adjust unit test
martijnvg Nov 20, 2025
ed3dbc3
[CI] Auto commit changes from spotless
Nov 20, 2025
37552f2
Merge remote-tracking branch 'es/main' into fix_StoredFieldsSpec_merge
martijnvg Nov 20, 2025
471a415
added LIMIT 10 to avoid warning, doesn't change the test.
martijnvg Nov 20, 2025
94b779f
fix query
martijnvg Nov 20, 2025
a6fe534
Merge remote-tracking branch 'es/main' into fix_StoredFieldsSpec_merge
martijnvg Nov 20, 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
12 changes: 10 additions & 2 deletions server/src/main/java/org/elasticsearch/search/fetch/StoredFieldsSpec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,17 @@ public StoredFieldsSpec merge(StoredFieldsSpec other) {
mergedSourcePaths = new HashSet<>(this.sourcePaths);
mergedSourcePaths.addAll(other.sourcePaths);
} else if (this.sourcePaths.isEmpty() == false) {
mergedSourcePaths = this.sourcePaths;
if (other.requiresSource) {
Copy link

@quackaplop quackaplop Nov 21, 2025

Choose a reason for hiding this comment

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

Ah! So our intuition was right - every time we apply the filter, we are stripping down the rest of the source. The fix is to always keep the source if it's been asked for

Copy link

@quackaplop quackaplop Nov 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

As a side note, there seems to be quite a bit of boiler plate in this method, with some repeated outcomes. Do we want to shove them into separate tiny inlinable functions?

Copy link
Member Author

@martijnvg martijnvg Nov 21, 2025

Choose a reason for hiding this comment

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

Let me see if that actually makes to code cleaner.

mergedSourcePaths = Set.of();
} else {
mergedSourcePaths = this.sourcePaths;
}
} else if (other.sourcePaths.isEmpty() == false) {
mergedSourcePaths = other.sourcePaths;
if (this.requiresSource) {
mergedSourcePaths = Set.of();
} else {
mergedSourcePaths = other.sourcePaths;
}
} else {
mergedSourcePaths = Set.of();
}
Expand Down
12 changes: 11 additions & 1 deletion server/src/test/java/org/elasticsearch/search/fetch/StoredFieldsSpecTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ public void testMergeSourcePaths() {

spec = spec.merge(
new StoredFieldsSpec(
true,
false,
Copy link

@quackaplop quackaplop Nov 21, 2025

Choose a reason for hiding this comment

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

So this may be silly question, but it seems like we very much changed the meaning of this test, right? The way I read this, we changed requiresSource to false, which - in some sense - triggers the "pre-bug" behavior. This makes sense.
Should we add a mirror test with this flag set to true?

Copy link
Member Author

@martijnvg martijnvg Nov 21, 2025

Choose a reason for hiding this comment

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

The test just walks through a few scenarios and I just appended the scenario that triggered the bug. I can add a dedicated unit test for the bug here.

false,
Set.of("other_field"),
IgnoredSourceFieldMapper.IgnoredSourceFormat.NO_IGNORED_SOURCE,
Expand All @@ -133,6 +133,16 @@ public void testMergeSourcePaths() {
assertThat(spec.requiredStoredFields(), containsInAnyOrder("other_field"));
assertThat(spec.sourcePaths(), containsInAnyOrder("cat", "dog", "hamster"));
assertThat(spec.sourcePaths(), sameInstance(pref));

// Clears source paths, because the spec requires complete source (since no source paths are defined)
spec = spec.merge(
new StoredFieldsSpec(true, false, Set.of(), IgnoredSourceFieldMapper.IgnoredSourceFormat.NO_IGNORED_SOURCE, Set.of())
);
assertThat(spec.ignoredSourceFormat(), equalTo(IgnoredSourceFieldMapper.IgnoredSourceFormat.NO_IGNORED_SOURCE));
assertThat(spec.requiresSource(), equalTo(true));
assertThat(spec.requiresMetadata(), equalTo(false));
assertThat(spec.requiredStoredFields(), containsInAnyOrder("other_field"));
assertThat(spec.sourcePaths(), empty());
}

private static SearchContext searchContext(SearchSourceBuilder sourceBuilder) {
Expand Down
32 changes: 32 additions & 0 deletions x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/140_metadata.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,3 +210,35 @@ setup:
"keyword" : "ok",
"case" : "ok"
}}

---
"source meta field and dynamic false":
- do:
indices.create:
index: my-index2
body:
mappings:
dynamic: false
Copy link

@quackaplop quackaplop Nov 21, 2025

Choose a reason for hiding this comment

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

Is this important? The dynamic I mean

Copy link
Member Author

@martijnvg martijnvg Nov 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

For the query being tested here, yes. Otherwise the test wouldn't fail.
The FROM my-index2 METADATA _source | LIMIT 10 requests all mapped fields, but because baz isn't mapped without the fix to StoredFieldSpec, we would filter it out. If the baz field was mapped, the stored field spec of the other column would include it in the final stored field spec.

properties:
foo:
type: text

- do:
bulk:
index: my-index2
refresh: true
body:
- { "index": { } }
- { "baz": "dasd" }
- do:
esql.query:
body:
query: 'FROM my-index2 METADATA _source | LIMIT 10'
Copy link

@quackaplop quackaplop Nov 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Not to be annoying, but are we testing here with what we actually want to be testing?

Here we seem to have an index with a single text field, so even if we had not fixed the bug, I think this still would've worked, right - we would've stripped the source of everything but the field, which doesn't actually change the _source.

Feels like we need the mapping with at least one another field, and the query that reads the entire source and the text field. We should be testing that the filtered out field is still returned in source

This thing - #138188 - has the correctly shaped example I think.

Copy link
Member Author

@martijnvg martijnvg Nov 21, 2025

Choose a reason for hiding this comment

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

Here we seem to have an index with a single text field, so even if we had not fixed the bug, I think this still would've worked, right - we would've stripped the source of everything but the field, which doesn't actually change the _source.

Without the fix to StoredFieldSpec this test fails.

I can add a @timestamp field, but that doesn't change the outcome of the test.


- match: {columns.0.name: "foo"}
- match: {columns.0.type: "text"}
- match: {columns.1.name: "_source"}
- match: {columns.1.type: "_source"}

- match: {values.0.0: null}
- match: {values.0.1: { "baz": "dasd" }}