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

Define range/2 by jq-coded function #1960

Closed
wants to merge 3 commits into from

Conversation

itchyny
Copy link
Contributor

@itchyny itchyny commented Aug 14, 2019

Implementation of builtin functions in bytecode occasionally results into some complex bugs. As for range/2, there was #886, which was fixed by a47b329. But I think the implementation is complicated enough and it's worth make it simple by jq-coded function. The RANGE is unnecessary and reducing the instructions simplifies the interpreter loop.

@pkoppstein
Copy link
Contributor

Unfortunately, defining range/2 in this way is significantly slower than having it as a builtin.

The slow-down is especially relevant because range is often nested.

Also, range/2 gives a useful error message if the args are not as they should be. With the proposed def, range(0;null) yields empty, and range(0;"") produces an infinite stream. Of course it would be easy to address this issue, but the slow-down, it seems to me, renders it moot.

@itchyny
Copy link
Contributor Author

itchyny commented Aug 14, 2019

Yeah, it is actually slower, but I'm not sure this is significantly slower. I measured with the time command but I don't think this slow-down is unacceptably significant. It is fast enough to iterate to 10000000 which may be too large for normal use case isn't it?

 % time jq -n 'range(10000000)' > /dev/null
jq -n 'range(10000000)' > /dev/null  7.26s user 0.03s system 98% cpu 7.426 total
 % time ./jq  -n 'range(10000000)' > /dev/null
./jq -n 'range(10000000)' > /dev/null  11.68s user 0.03s system 99% cpu 11.816 total

Also I'm not sure how it is important to keep the behavior of range(0; ""), which is inconsistent with range(0; ""; 1).
Anyway, the comment Note that we can now define range as a jq-coded function should be replaced with the reason why range/2 is implemented as bytecoded function.

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

2 participants