Skip to content
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.

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

Remove cutoff_frequency, deprecated in es7.3.0, forbidden in es8 #1657

Merged

Conversation

michaelkirk
Copy link
Contributor

@michaelkirk michaelkirk commented Jul 21, 2023

Here's the reason for this change 🚀

In pursuit of eventually supporting es8, I've dropped some es6 only behavior in pelias/query#134.

It's a draft because it depends on pelias/query#134 being released first.


Here's what actually got changed 👏

  • integrates the updated version of pelias-query which drops cutoff_frequency.
  • updates the tests.

Here's how others can test the changes 👀

Other than npm test, I've run the north-america tests.

Note: The north-america tests aren't all passing, but the ones that are failing are exactly the same as when I run from current master. I'm assuming (🤞) that the failures reflect changes in the input data since the tests were updated, as hypothesized over here.

Aggregate test results
Pass: 233
Improvements: 18
Expected Failures: 28
Placeholders: 0
Regressions: 84
Total tests: 363
Took 7774ms
Test success rate 76.86%

north-america integration test output before
north-america test output after

@michaelkirk michaelkirk marked this pull request as draft July 21, 2023 23:24
@michaelkirk michaelkirk force-pushed the mkirk/remove-cutoff-freq branch from 9702be4 to f2f641a Compare July 27, 2023 01:29
@michaelkirk
Copy link
Contributor Author

re: build failure https://github.com/pelias/api/actions/runs/5627436077

Because of fa9838f, it looks like I need to add my own repo/org variables in order for tests to pass. I've done that here:

Screenshot of org settings, showing setting UBUNTU_VERSION to ubuntu-22.04

...and now tests are passing

(except for the docker --push, for which I haven't configured secrets. It seems like it'd be more convenient to use the pre-existing secrets.GH_TOKEN rather than requiring manual config, but that's a separate issue).

It's not a big deal, but I expect others will get confused by this as well.

@missinglink
Copy link
Member

Hi @michaelkirk, I've merged pelias/query#134, can you please update this PR so the package.json dependency points to the latest pelias/query (which should be 11.2):

npm install pelias-query@latest --save

.. and then open it up for review, tx!

From https://www.elastic.co/guide/en/elasticsearch/reference/8.8/migrating-8.0.html

    The cutoff_frequency parameter has been removed from the match and
    multi_match query.

    Details
    The cutoff_frequency parameter, deprecated in 7.x, has been removed
    in 8.0 from match and multi_match queries. The same functionality
    can be achieved without any configuration provided that the total
    number of hits is not tracked.

    Impact
    Discontinue use of the cutoff_frequency parameter. Search requests
    containing this parameter in a match or multi_match query will
    return an error.

Note in the above "...provided that the total number of hits is not
tracked".

`track_total_hits` does not appear in the pelias codebases, so we
shouldn't have any issues there.
@michaelkirk michaelkirk force-pushed the mkirk/remove-cutoff-freq branch from f2f641a to 9282c62 Compare April 18, 2024 19:44
@michaelkirk michaelkirk marked this pull request as ready for review April 18, 2024 19:55
@michaelkirk
Copy link
Contributor Author

Done (and rebased).

@michaelkirk michaelkirk marked this pull request as draft April 18, 2024 19:58
@michaelkirk
Copy link
Contributor Author

I missed an occurrence somehow. Let me fix this up, retest, and I'll re-open shortly.

@missinglink
Copy link
Member

You didn't miss the macrocounty one, I added that a few weeks ago in #1552 🙏

@missinglink missinglink marked this pull request as ready for review April 19, 2024 11:08
@missinglink missinglink merged commit 24d3306 into pelias:master Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants