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

fix null queries over links to an indexed property #4460

Merged
merged 2 commits into from
Mar 2, 2021

Conversation

ironage
Copy link
Contributor

@ironage ironage commented Feb 25, 2021

This was introduced in #3432.
The optimization made there is correct except when searching for null because we consider a null link along the relationship path to the property to be a match.

@ironage ironage requested a review from jedelbo February 25, 2021 02:29
@ironage ironage self-assigned this Feb 25, 2021
@jedelbo
Copy link
Contributor

jedelbo commented Feb 26, 2021

we consider a null link along the relationship path to the property to be a match

Is that so? It kind of surprises me. If you have a query like friend.buddy.pet == NULL then I would expects to get all objects that have a friend that has a buddy that does not have a pet. It should not include the friends that have no buddies.

@ironage
Copy link
Contributor Author

ironage commented Feb 26, 2021

This fix is only to make queries for null across links on indexed properties consistent with queries for null across links on non indexed properties. Additionally, it is only the optimized path that had a problem. Without this PR friend.buddy.pet == NULL would produce different results than NULL == friend.buddy.pet. Because we only optimize for constant on the right hand side.

I agree it is surprising behaviour, but it I think changing it would be considered breaking behaviour. There may be a strong argument to change the behaviour (I don't know the historical reasons why it is this way), but a counter argument is that if you have many links it may be tedious to write queries which do check for null along the link path. Users would have to transform friend.buddy.pet == NULL into friend == NULL || friend.buddy == NULL || friend.buddy.pet == NULL whereas today we assume this behaviour and if users want to opt out of this they can write friend.buddy != NULL && friend.buddy.pet == NULL. I'm not sure which behaviour is more intuitive actually, but having this greedy link behaviour seems somewhat easier to explain.

@nirinchev
Copy link
Member

I think that Jørgen's interpretation is more intuitive. If we're concerned about the verbosity, we can introduce new syntax - ?. which will return a match if anything in the chain equals null - i.e. friend?.buddy?.pet == NULL. But I'd say that the current behavior is unexpected and if someone was relying on it, then they likely have a bug in their code.

@ironage
Copy link
Contributor Author

ironage commented Feb 26, 2021

We have tried to conform to NSPredicate perhaps that is the reason. @tgoyne do you know which behaviour Apple has chosen?

@tgoyne
Copy link
Member

tgoyne commented Feb 26, 2021

In NSPredicate, "a.b.c = nil" is equivalent to "a = nil OR a.b = nil OR a.b.c = nil". This is inherited from the obj-c semantics where the dot operator is what more modern languages have as ?..

Copy link
Contributor

@jedelbo jedelbo left a comment

Choose a reason for hiding this comment

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

Arguments accepted

@ironage ironage merged commit 2f08393 into master Mar 2, 2021
@ironage ironage deleted the js/query-index-null-over-links branch March 2, 2021 17:31
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants