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.

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

Jump to bottom

Fix #633: prevent use of "$similarity" in projection #635

Merged
merged 13 commits into from
Nov 13, 2023
Merged

Fix #633: prevent use of "$similarity" in projection #635

merged 13 commits into from
Nov 13, 2023

Conversation

tatu-at-datastax
Copy link
Contributor

What this PR does:

Prevents use of $similarity in Projection clause, since it is not a real field and there is different mechanism for inclusion via options.

Which issue(s) this PR fixes:
Fixes #633

Checklist

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CLA Signed: DataStax CLA

@tatu-at-datastax tatu-at-datastax self-assigned this Nov 10, 2023
@tatu-at-datastax tatu-at-datastax marked this pull request as ready for review November 10, 2023 23:16
@tatu-at-datastax tatu-at-datastax requested a review from a team as a code owner November 10, 2023 23:16
@Yuqi-Du
Copy link
Contributor

Yuqi-Du commented Nov 13, 2023 •
edited
Loading

Could you please also delete two corresponding NB tests

https://github.com/stargate/jsonapi/blob/cf3f5789862aa7c618edfcdb43a859f449ed3235/fallout/vector-test-nosqlbench.yaml#L119C1-L119C1

https://github.com/stargate/jsonapi/blob/cf3f5789862aa7c618edfcdb43a859f449ed3235/nosqlbench/http-jsonapi-vector-crud.yaml#L139

Perhaps just delete the ops.

LGTM.

@tatu-at-datastax
Copy link
Contributor Author

Could you please also delete two corresponding NB tests

https://github.com/stargate/jsonapi/blob/cf3f5789862aa7c618edfcdb43a859f449ed3235/fallout/vector-test-nosqlbench.yaml#L119C1-L119C1

https://github.com/stargate/jsonapi/blob/cf3f5789862aa7c618edfcdb43a859f449ed3235/nosqlbench/http-jsonapi-vector-crud.yaml#L139

Perhaps just delete the ops.

LGTM.

I changed to use options instead: does this look good now?

@Yuqi-Du
Copy link
Contributor

Yuqi-Du commented Nov 13, 2023

Could you please also delete two corresponding NB tests
https://github.com/stargate/jsonapi/blob/cf3f5789862aa7c618edfcdb43a859f449ed3235/fallout/vector-test-nosqlbench.yaml#L119C1-L119C1
https://github.com/stargate/jsonapi/blob/cf3f5789862aa7c618edfcdb43a859f449ed3235/nosqlbench/http-jsonapi-vector-crud.yaml#L139

Perhaps just delete the ops.
LGTM.

I changed to use options instead: does this look good now?

Perfect! Thanks!

@tatu-at-datastax
Copy link
Contributor Author

For some reason, CI again stuck -- perhaps since merge from main pulled doc-only commits?
Need to force merge.

@tatu-at-datastax tatu-at-datastax merged commit 02193c8 into main Nov 13, 2023
3 of 4 checks passed
@tatu-at-datastax tatu-at-datastax deleted the tatu/633-no-similarity-on-projection branch November 13, 2023 20:00
@eolivelli
Copy link

FYI It seems that this change broke the LangChain integration, see this patch
langchain-ai/langchain#13512

Currently with LangChain you cannot use the JSON API

@tatu-at-datastax
Copy link
Contributor Author

tatu-at-datastax commented Nov 20, 2023 •
edited
Loading

@eolivelli Thank you for reporting this. It is unfortunate, I'll let others know so they are aware of temporary breakage of Langchain (thank you for the link to PR).

At this point we hope to solidify API but there may be some last breaking changes (in some cases we can keep backwards compatibility; here unfortunately not).

vkarpov15
vkarpov15 reviewed Nov 21, 2023
View reviewed changes
Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@eolivelli
Copy link

The problem affected LlamaIndex as well
run-llama/llama_index#9193

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vkarpov15 vkarpov15 vkarpov15 left review comments

@Yuqi-Du Yuqi-Du Yuqi-Du approved these changes

Assignees

@tatu-at-datastax tatu-at-datastax

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

$similarity accepted within projection
4 participants
@tatu-at-datastax @Yuqi-Du @eolivelli @vkarpov15