phone-search analyzer: don't emit sip/tel prefix, int'l prefix, extension & unformatted input
#16993
Conversation
rursprung
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
dblock,
dbwiddis,
gbbafna,
jainankitk,
kotwanikunal,
linuxpi,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami and
VachaShah
as code owners
rursprung
force-pushed
the
analysis-phonenumber-no-intl-prefix-in-search
branch
from
2b023bc to
6b36f2e
Compare
|
❌ Gradle check result for 6b36f2e: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
rursprung
force-pushed
the
analysis-phonenumber-no-intl-prefix-in-search
branch
from
6b36f2e to
9e120b7
Compare
|
please add the backport 2.x label (and if there's a chance for a 2.18.1 release please also the backport to that 🙂) |
|
❕ Gradle check result for 9e120b7: UNSTABLE
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16993 +/- ##
============================================
+ Coverage 72.17% 72.19% +0.02%
+ Complexity 65251 65199 -52
============================================
Files 5301 5301
Lines 303662 303664 +2
Branches 43989 43991 +2
============================================
+ Hits 219181 219245 +64
+ Misses 66552 66430 -122
- Partials 17929 17989 +60 ☔ View full report in Codecov by Sentry. |
these are both flaky tests: |
|
@rursprung could you please resolve the conflicts? thank you |
reta
added
bug
this was an oversight in the initial implementation: if the tokenizer emits the international calling prefix in the search analyzer then all documents with the same international calling prefix will match. e.g. when searching for `+1-555-123-4567` not only documents with this number would match but also any other document with a `1` token (i.e. any other number with this prefix). thus the search functionality is currently broken for this analyzer, making it useless. the test coverage has now been extended to cover these and other use-cases. Signed-off-by: Ralph Ursprung <[email protected]>
if these tokens are emitted it meant that phone numbers with other international dialling prefixes still matched. e.g. searching for `+1 1234` would also match a number stored as `+2 1234`, which was wrong. the tokens still need to be emited for the `phone` analyzer, e.g. when the user only enters the extension / local number it should still match, the same is with the other ngrams: these are needed for search-as-you-type style queries where the user input needs to match against partial phone numbers. Signed-off-by: Ralph Ursprung <[email protected]>
rursprung
force-pushed
the
analysis-phonenumber-no-intl-prefix-in-search
branch
from
9e120b7 to
ff3c8da
Compare
rursprung
changed the title
phone-search analyzer: don't emit int'l prefixphone-search analyzer: don't emit int'l prefix, extension & unformatted input
done. i've also added a 2nd commit and am now emitting even fewer tokens. please see the commit messages for further details. with the new test coverage i'm now quite confident that this is working as intended |
|
❌ Gradle check result for ff3c8da: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
flaky test: #16658 |
|
❌ Gradle check result for ff3c8da: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
flaky test: #15826 |
in line with the previous two commits, this is something else the search analyzer shouldn't emit since otherwise searching for any number with such a prefix will match _any_ document with the same prefix. Signed-off-by: Ralph Ursprung <[email protected]>
rursprung
changed the title
phone-search analyzer: don't emit int'l prefix, extension & unformatted inputphone-search analyzer: don't emit sip/tel prefix, int'l prefix, extension & unformatted input
aaaand that confidence was misplaced, i spotted another corner case and fixed that as well (incl. increased test coverage). now it should be fine 😅 |
|
The backport to To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.x
# Create a new branch
git switch --create backport/backport-16993-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 4d943993ac93e1a140c1b58c11e812a58578f27d
# Push it to GitHub
git push --set-upstream origin backport/backport-16993-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.xThen, create a pull request where the |
|
@rursprung could you please backport to 2.x manually? thank you |
Description
see the individual commit messages for further details.
Related Issues
n/a
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.