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

Type errors for operators? #26

Open
yozlet opened this issue Jun 13, 2020 · 3 comments
Open

Type errors for operators? #26

yozlet opened this issue Jun 13, 2020 · 3 comments

Comments

@yozlet
Copy link

yozlet commented Jun 13, 2020

At present, operators just return null on type failure (e.g.: 3 + "str"). I'd much rather have this blow up with a useful error than have to check all query results for buried nulls.

@judofyr
Copy link
Contributor

judofyr commented Jun 15, 2020

I understand that it's annoying to not get type errors and we do have a plan for improving the situation, but let me first explain the reasons for why we can't just naively add type errors:

  1. By having type errors return null it means that we can safely re-order boolean expression. Example: *[_type == "event" && externalID == "abc"] and *[externalID == "abc" && _type == "event"]. If we say that type mismatch should return an error, then the second query would return an error (assuming that there are other types where externalID is a number). Having the ability to reorder && and || is very convenient when optimizing the GROQ query at the backend.

  2. It's more expensive to fetch the data. For a query like *[_type == "event" && externalID == "abc"] with the current semantics we only need to fetch all documents which has the given _type and externalID. If we want to this to return an error on type failures then we also need to find all documents which has _type == "event" and an externalID which is not a string. The more fields there are, the more there is to fetch.

We do agree that it's annoying during development so the plan is to introduce a warning concept (see #16). This would be a flag you can pass to the backend (warnings=true) which will return additional warnings "on the side". The expression itself will return null (and the query will succeed as before), but next to the result there will be a warnings array which contains information.

Does this sound good?

@yozlet
Copy link
Author

yozlet commented Jul 2, 2020

Thanks, that's a helpful explanation. I confess that I find the two given reasons to be a little odd, but my questions would probably prove your overall point: that adding type errors to an existing system turns up all kinds of ugly edge case questions.

The warning idea sounds like the best in the circumstances, as long as GROQ queries are read only (and it looks like there's no plan to change that). The nice side effect of providing multiple warnings from a query is that it shows the user all the distinct problems with the query, not just the first one. (I would consider the example you give in reason 2 to be a single problem, not a separate problem for each document with numeric IDs.)

@judofyr
Copy link
Contributor

judofyr commented Jul 4, 2020

Thanks, that's a helpful explanation. I confess that I find the two given reasons to be a little odd, but my questions would probably prove your overall point: that adding type errors to an existing system turns up all kinds of ugly edge case questions.

I understand that some of these issues are bit vague when you're not aware of the processing we do internally to optimize it. Here's maybe a better example if you're interested: Look at the query *[_type == "person" && (upVotes - downVotes) > 0 && age >= 18]. By looking at the query alone we would assume that a document with "upVotes": 5, "downVotes": 0, "age": "18" should give a type error (since age is a string), but "upVotes": 5, "downVotes": 10, "age": "18" should be fine (because we filter it away on (upVotes - downVotes) > 0 before we try to access the age attribute).

Internally we have an index on the _type and age attributes which means that we can very quickly find the people with age >= 18. (upVotes - downVotes) > 0 on the other hand is way too complicated. What we want to do in this case is to optimize the query to use the index for finding all people over 18, and then later filter away those with (upVotes - downVotes) <= 0. You will notice that with this optimization we would not return any type error for either of those cases mentioned above.

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

No branches or pull requests

2 participants