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

CBL-5180: Implement Array Index API #3327

Open
wants to merge 5 commits into
base: release/3.2
Choose a base branch
from
Open

CBL-5180: Implement Array Index API #3327

wants to merge 5 commits into from

Conversation

velicuvlad
Copy link
Contributor

No description provided.

@velicuvlad velicuvlad marked this pull request as ready for review September 9, 2024 15:58
expressions: @[@""]];
} else {
for (NSString* expression in expressions) {
if (!expression) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • NSArray/NSMutableArray doesn't allow nil so there is no need to check this for iOS.
  • The only thing that is needed to evaluate is whether expressions is empty or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for reminding me about NSArray/MutableArray, changing now

Copy link
Member

@borrrden borrrden left a comment

Choose a reason for hiding this comment

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

Probably fine but I also check that there is at least one non-empty element in the array before sending it to Core.

As you know, [""] works, but I would guess more often than not this is an error in the user's logic that ended up with empty string, rather than the user purposefully sending an empty string in the array (as opposed to nil). If there is at least one non-empty string in the list then any empty strings will then be caught and errored by Core.

@velicuvlad
Copy link
Contributor Author

velicuvlad commented Sep 10, 2024

@borrrden if is nil, I pass to Core [""] anyway. IMO, if we go ahead and check whether a non-empty list has any empty values, we might as well fail straight away in the case that is nil as well. Which then makes us handle most, if not all, scenarios, instead of letting Core to do it.

EDIT:
I've read it a couple more times and yes, indeed, from the user's perspective if I want it empty I would just pass nil, meanwhile that being potential empty can be an error my in code.

But what does LiteCore do in that scenario?
I know for nil passed, the path must contain scalar values. If that's not the case, I guess LiteCore throws? (we can't figure that out on client side so we don't care)
If path is not scalar (which again we can't tell), Is it just picking the non-empty values from expressions and computes with it or is throwing?

@velicuvlad
Copy link
Contributor Author

velicuvlad commented Sep 10, 2024

["class", "", "name"] will result into class,,name string representing n1ql. Now, as n1ql, that is invalid, but Jens mentioned on slack that as long as we pass a comma sep string to Core, it should be good enough for Core to sort out if is good or bad.

I've chat with Jianmin, and confirmed that will throw as invalid n1ql, which can be confusing depending of it actually showing where the problem is, but my understanding is that Core shouldn't even query with the above from Jens, but throw.

I will add this additional check if it ends up not being covered by Core.

@pasin
Copy link
Contributor

pasin commented Sep 14, 2024

@velicuvlad

I know for nil passed, the path must contain scalar values. If that's not the case, I guess LiteCore throws? (we can't figure that out on client side so we don't care)

LiteCore will index the complex value which will turn out to be unuseful. There will be no error.

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.

3 participants