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

Big pile of builtin changes #1845

Merged
merged 9 commits into from Feb 26, 2019
Merged

Conversation

muhmuhten
Copy link
Contributor

@muhmuhten muhmuhten commented Feb 26, 2019

Rationale:

  • Remove scalars_or_empty per scalars_or_empty is undocumented #1840

  • The isempty/any/all are coming out of the discussion in Enhanced any and all. #1839. Some benchmarking indicated that an extra not in the loop really does make any significantly slower.

  • I was trying to get fromstream going faster, but it doesn't want to go any faster. This implementation uses an explicit emit/reset flag to handle when it outputs, instead of implicitly clearing the object being built by shuffling it between .[0] and .[1] on array/object closure, and thereby avoids duplicating what-are-we-doing-with-the-path logic between the update and extract steps. I think this this makes the control flow easier to follow so I rolled it in, but it's certainly arguable.

  • It is pretty hard to explain this tostream. That last reduce in particular does not look like it should be optimal but everything I've tried doing with it makes it slower. It feels like there should be a better way to keep the last key than going through the array/object a second time, but keeping extra state to do that makes it slower. Maybe someone else might have a better idea. The key insights are (a) .[]? produces zero elements for precisely the leaf nodes of a stream; (b) paths on that provides the streaming keys we apparently wanted; and (c) oh, so reduce is how you implement last/1.

  • bsearch was doing a really weird thing if its argument generated multiple arguments because it'd get contradictory-looking comparisons by using different values each go-around. That's, uh, bad. Added a test, too.

  • walk/1: another one based on the "paths of backward (post-order) recurse" primitive, which, I'm starting to feel is more use than the pre-order one. It is actually really cool that this one Just Works when you think about it with jq goggles on. See also walk function not applied to child object of match #1724. Retracted because |= does not do the right thing for multiple/zero outputs on arrays and uh, actually does some really weird stuff on arrays.

  • As Make INDEX/2 more efficient #1654 observed, if type != "string" then tojson else . end winds up doing precisely tostring, except the latter does the type check in C: https://github.com/stedolan/jq/blob/9a0d5be790f0c89c02ffde11144157f067b19fd1/src/builtin.c#L452-L458 And note that $row is always going to be one value that doesn't depend on its input, so we can assign rather than modify here.

  • turns out IN was just a shouty way of writing any, except with less short-circuiting and a lot more if ... then true else false end. This became a lot more obvious after rewriting any and all!

These should be reasonably cherry-pickable if some of them are disagreeable, too.

Fixes the bug where all and any evaluate one more item than needed.
New implementation in terms of while/2, and branches immediately on $by
to avoid checking the sign of $by *in* the loop.
(`path(.[])` is a streaming `keys`!)
@coveralls
Copy link

coveralls commented Feb 26, 2019

Coverage Status

Coverage decreased (-0.02%) to 84.585% when pulling 93ff9ca on muhmuhten:builtin_improvements into 9a0d5be on stedolan:master.

@nicowilliams nicowilliams merged commit 76e72a3 into jqlang:master Feb 26, 2019
@nicowilliams
Copy link
Contributor

Thanks!!!

This was referenced Feb 26, 2019
@muhmuhten muhmuhten deleted the builtin_improvements branch February 26, 2019 23:24
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.

None yet

3 participants