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

DON'T MERGE swap weakAnd to userInput in YQL #142

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kdutia
Copy link
Member

@kdutia kdutia commented Nov 4, 2024

Description

This is a change to fix a seeming bug that wasn't detected until removing description embeddings from the YQL query and Vespa schema. Pushing this change first to check that it. works with the current schema, then i'll push changes to the schema and YQL to remove embedding search on descriptions.

The issue this aims to fix is that weakAnd seems to sometimes act like an AND, meaning that empty documents can be removed from search results even when they match on title or summary fields.

UserInput docs for reference. This should also open us up to "i want to search this exact phrase" queries, which is supported by UserInput, so we'd just need to disable the nearestneighbour YQL clause in this case

Proposed version

Please select the option below that is most relevant from the list below. This
will be used to generate the next tag version name during auto-tagging.

  • Skip auto-tagging
  • Patch
  • Minor version
  • Major version

Visit the Semver website to understand the
difference between MAJOR, MINOR, and PATCH versions.

Type of change

Please select the option(s) below that are most relevant:

  • Bug fix
  • New feature
  • Breaking change

How Has This Been Tested?

No changes to tests, but current changes still pass locally.

Before submitting

  • I've read and followed all steps in the Making a pull request
    section of the CONTRIBUTING docs.
  • I've updated or added any relevant docstrings following the syntax described in the
    Writing docstrings section of the CONTRIBUTING docs.
  • If this PR fixes a bug, I've added a test that will fail without my fix.
  • If this PR adds a new feature, I've added tests that sufficiently cover my new functionality.

@kdutia kdutia requested a review from a team as a code owner November 4, 2024 11:16
Copy link

linear bot commented Nov 4, 2024

family_description contains(@query_string),
text_block contains(@query_string)
)
userInput(@query_string)
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be something that I'm missing when looking at the docs, but I'm struggling to get my head around how userInput 'knows' which fields to look at, do you have a clearer understanding of this you could help me with?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes - i think it's the default fields specified in the schema

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah that makes sense

Copy link
Contributor

@olaughter olaughter left a comment

Choose a reason for hiding this comment

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

lgtm! Shout if you need anything more on backend tests

family_description contains(@query_string),
text_block contains(@query_string)
)
userInput(@query_string)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah that makes sense

@kdutia kdutia changed the title swap weakAnd to userInput in YQL DON'T MERGE swap weakAnd to userInput in YQL Nov 5, 2024
@kdutia kdutia marked this pull request as draft November 5, 2024 16:22
@kdutia
Copy link
Member Author

kdutia commented Nov 5, 2024

marking as don't merge as this seems to fail tests in the backend - see comment on linear ticket

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