Skip to content

Let random_score yaml test explicitly fail on _id field #125230

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 10 commits into from
Mar 20, 2025
Merged

Let random_score yaml test explicitly fail on _id field #125230

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0e58c98
Explicitly fail random_score on _id
tteofili Mar 19, 2025
5c1f7a3
unmuted test
tteofili Mar 19, 2025
2e5f0dc
Update docs/changelog/125230.yaml
tteofili Mar 19, 2025
7d537ba
constrain the no-field scenario to 9.x
tteofili Mar 20, 2025
a08ae2a
Merge branch 'fsrandom_yaml_fix' of github.com:tteofili/elasticsearch…
tteofili Mar 20, 2025
e9c2a7c
Merge branch 'main' of github.com:elastic/elasticsearch into fsrandom…
tteofili Mar 20, 2025
fc6d12c
revert unwanted change
tteofili Mar 20, 2025
513194e
Delete docs/changelog/125230.yaml
tteofili Mar 20, 2025
1f30dc5
Merge branch 'main' of github.com:elastic/elasticsearch into fsrandom…
tteofili Mar 20, 2025
b3a1c62
Merge branch 'fsrandom_yaml_fix' of github.com:tteofili/elasticsearch…
tteofili Mar 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
6 changes: 6 additions & 0 deletions docs/changelog/125230.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 125230
summary: Let `random_score` yaml test explicitly fail on `_id` field
area: Search
type: bug
issues:
- 125010
3 changes: 0 additions & 3 deletions muted-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,9 +393,6 @@ tests:
- class: org.elasticsearch.datastreams.lifecycle.DataStreamLifecycleServiceIT
method: testLifecycleAppliedToFailureStore
issue: https://github.com/elastic/elasticsearch/issues/124999
- class: org.elasticsearch.backwards.MixedClusterClientYamlTestSuiteIT
method: test {p0=search/610_function_score/Random}
issue: https://github.com/elastic/elasticsearch/issues/125010
- class: org.elasticsearch.packaging.test.DockerTests
method: test010Install
issue: https://github.com/elastic/elasticsearch/issues/119441
Expand Down
7 changes: 3 additions & 4 deletions rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/610_function_score.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
- match: { hits.total.value: 2 }

- do:
catch: /Fielddata access on the _id field is disallowed/
Copy link
Member

@benwtrent benwtrent Mar 19, 2025

Choose a reason for hiding this comment

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

I don't see how this improves things? I think we should block bwc testing. field is no longer necessary for v9: https://github.com/elastic/elasticsearch/pull/118671/files

I am not sure why this is failing at all. Is it failing in a mixed cluster? If so, we should adjust the assertions when hitting an older cluster vs v9.

But I do not like changing the test behavior like this. We are purposefully testing that not providing a field works. Let's continue to do that.

Copy link
Contributor

@iverase iverase Mar 19, 2025

Choose a reason for hiding this comment

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

Should this test pass if we provide rest api compatibility?

Copy link
Contributor Author

@tteofili tteofili Mar 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

the thing is that the actual field being used underneath has changed from 8.x to 9.x (_id to _seqno), so the original idea was to make sure that this works across versions even when not supplying any field.
having an explicit test with _id should fail both on 8.x and 9.x (without explicitly enabling fielddata for _id), but I am fine with your suggestion of constraining the test to 9.x versions only.

Copy link
Member

@benwtrent benwtrent Mar 19, 2025

Choose a reason for hiding this comment

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

The failure is that there is a deprecation warning indicating that "Hey, we will require you to provide a field in the future", which isn't the case any longer.

The user's API experience is largely unchanged, except now they do NOT get a deprecation warning.

Copy link
Member

@benwtrent benwtrent Mar 19, 2025

Choose a reason for hiding this comment

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

AH, I see the test was added here: #124893

If we want this test to run in 8.x, we should have a separate one that is designed for 8.x that exercises the paths we expect there. But given that this test only exists in v9.0, maybe we only have it run there.

search:
index: test
body:
Expand All @@ -71,13 +72,11 @@
}
},
"random_score": {
"seed": 10
"seed": 10,
"field": "_id"
}
}

- length: { hits.hits: 2 }
- match: { hits.total.value: 2 }

- do:
search:
index: test
Expand Down
Loading