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

Sometimes secondary index functions are getting evaluated with global optargs present. #2767

Closed
srh opened this issue Jul 26, 2014 · 11 comments
Assignees
Milestone

Comments

@srh
Copy link
Contributor

srh commented Jul 26, 2014

Secondary index function evaluation should not have any global optargs present (or if they did, we'd want the global optargs captured at the secondary index creation -- but that's not how things work right now).

Right, now, at insertion things are fine -- secondary index functions are evaluated without the global optargs present (according to the testing I've done). That's why this is a low priority, v1.14 bug.

However, we can see that queries like table.get_all([2], index='foo').run(right_bound='closed') (with a contrived secondary index function lambda x: x.slice(1,2)) are at some point rerunning the secondary index function in the environment with the global optargs present, affecting their evaluation.

I'm assigning this to myself because I'm already touching some of the code in question (in rget_cb_t::handle_pair).

@srh srh added this to the 1.14 milestone Jul 26, 2014
@srh srh self-assigned this Jul 26, 2014
@srh
Copy link
Contributor Author

srh commented Jul 26, 2014

I have implemented a fix in sam_2767. However, I don't know what the best way to write a regression test is. Is there a way to specify the global optargs in a reql test?

@gchpaco
Copy link
Contributor

gchpaco commented Jul 26, 2014

Yes. See the brand new array limits reql test for a demo.

srh added a commit that referenced this issue Jul 26, 2014
@srh
Copy link
Contributor Author

srh commented Jul 26, 2014

Awaiting review 1829.

@mlucy
Copy link
Member

mlucy commented Jul 27, 2014

c.f. #2710, I think it's unlikely enough that someone is relying on the old buggy behavior for their queries that we shouldn't offer a way to turn it back on.

@srh
Copy link
Contributor Author

srh commented Jul 28, 2014

What does this have to do with #2710?

@mlucy
Copy link
Member

mlucy commented Jul 28, 2014

There's the same question of "how likely is it someone is depending on this behavior vs. how much work is it to maintain the old behavior concurrently with the new behavior".

@srh
Copy link
Contributor Author

srh commented Jul 29, 2014

Issue #2710 does not have anything to do with that question.

@srh
Copy link
Contributor Author

srh commented Jul 29, 2014

Issue #2701 is the issue you are probably talking about. That issue is different because there you want to corrupt people's database files.

@mlucy
Copy link
Member

mlucy commented Jul 30, 2014

Whoops, sorry for the transposition.

(Also, I approved the review.)

@mlucy
Copy link
Member

mlucy commented Aug 1, 2014

@srh: is this in next?

@srh
Copy link
Contributor Author

srh commented Aug 4, 2014

I'm sorry. Yes, it is.

@srh srh closed this as completed Aug 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants