-
Notifications
You must be signed in to change notification settings - Fork 83
Document db.create.setRelationshipVectorProperty procedure #1478
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
Document db.create.setRelationshipVectorProperty procedure #1478
Conversation
| a| | ||
| Set a vector property on a given relationship in a more space efficient representation than Cypher's link:{neo4j-docs-base-uri}/cypher-manual/{page-version}/clauses/set#set-set-a-property[`SET`]. | ||
| | Signature | ||
| m| db.create.setRelationshipVectorProperty(relationship :: RELATIONSHIP, key :: STRING, vector :: ANY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ANY is a limitation of the procedure framework, and how it currently works to try and automatically figure out the type … which is only used for that test against the JSON file.
It's real signature would be LIST<INTEGER | FLOAT>.
| m| db.create.setRelationshipVectorProperty(relationship :: RELATIONSHIP, key :: STRING, vector :: ANY) | |
| m| db.create.setRelationshipVectorProperty(relationship :: RELATIONSHIP, key :: STRING, vector :: LIST<INTEGER | FLOAT>) |
and now same above for setNodeVectorProperty, (and setVectorProperty
I'm just confirming with @nilsceberg if it is an additive change, or technically always was that and thus a correction to the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nils has just confirmed with 5.16 that it worked with a LIST<INTEGER>; therefore, updating setNodeVectorProperty and setVectorProperty to LIST<INTEGER | FLOAT> is a docs correction and shouldn't required noting it as a signature change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is from the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I believe you're specifically referring to procedures.json, which is only used in procedure and function acceptance tests from cypher ops. I guess docs use this for reference too in that case?
For reference and clarity,
The procedure framework unfortunately sees almost anything that uses one of our internal types as ANY.
Even if it was a FloatingPointArray which is LIST<FLOAT>, it would be ANY in that type detection.
Semantically, and through enforcement of manual type checking, it should be LIST<INTEGER | FLOAT> here.
We are using the internal types for performance reasons, and this is an implementation detail.
If we were writing this in scala where some of their procedures/functions are, I believe the signature checker would use the static one that is provided, and thus correctly pick up it was a LIST<INTEGER | FLOAT> but I could be wrong.
Either way, the ANY in that JSON file are to satisfy those tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we use the Cypher command SHOW PROCEDURES to validate the available procedures, their description, signature, and mode. I can see that the genai.vector.encodeBatch is missing in the json now. What happened to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going back from
ANYtoLIST<INTEGER | FLOAT>in the documentation (if we are able to that in the future) seems like more of a breaking change than treating theSHOW PROCEDURESoutput as a bug. :S
So, are you suggesting to ignore the tests and just add LIST<INTEGER | FLOAT> instead of ANY even though the users will see a different signature when they run SHOW PROCEDURES?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should at least consider that, yes. Perhaps we could note the discrepancy in the docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what we should do in such cases. The documentation reflects what's in the output of SHOW PROCEDURES because that's what users see. If the output shows ANY, the docs also show ANY. And as per the docs test, everything seems fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And as per the docs test, everything seems fine.
Wait, are the tests passing for e.g. queryNodes? It currently seems to say query :: LIST<FLOAT> in the docs but that has also recently changed to ANY in the SHOW PROCEDURES output, unless I'm mistaken 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should at least consider that, yes. Perhaps we could note the discrepancy in the docs?
What would be a good note for that? Could you provide us with some content?
|
Should we also document |
Co-authored-by: NataliaIvakina <82437520+NataliaIvakina@users.noreply.github.com>
…rocedure" This reverts commit 08b1b76.
1f69ce9 to
f5b7007
Compare
|
Thanks for the documentation updates. The preview documentation has now been torn down - reopening this PR will republish it. |
Co-authored-by: NataliaIvakina <82437520+NataliaIvakina@users.noreply.github.com>
No description provided.