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

builtin.jq: simpler and faster transpose #2758

Merged
merged 3 commits into from
Jul 25, 2023

Conversation

pkoppstein
Copy link
Contributor

It turns out that using max/0 is faster than using a stream-oriented version of max

It turns out that using max/0 is faster than using a stream-oriented version of max
@itchyny
Copy link
Contributor

itchyny commented Jul 24, 2023

Previously [] | transpose produces [] but after this PR it changes to an error.

For backward-compatibility.
Add check in jq.test
@pkoppstein
Copy link
Contributor Author

@itchyny - For backward compatibility: 08da3a9

jq.test now has a test case for [] | transpose #=> []

Thanks.

src/builtin.jq Outdated
| reduce range(0; $max) as $j
([]; . + [reduce range(0;$length) as $i ([]; . + [ $in[$i][$j] ] )] )
end;
# Using map(length) turns out to be faster than using max(stream)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still unsure about the stream version of max to be included or not. Users not following the history will be get confused by this comment; where is max(stream)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@itchyny - Here, max(stream) is simply short for any valid streaming version of max. That should be clear since no specific referent is mentioned. Feel free to change the wording, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just drop the sentence, we don't need such performance notes here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Done in a20c6d5

Copy link
Contributor

@itchyny itchyny left a comment

Choose a reason for hiding this comment

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

Thank you!

@itchyny itchyny merged commit 8f49600 into jqlang:master Jul 25, 2023
28 checks passed
@pkoppstein pkoppstein deleted the transpose-efficiency branch July 25, 2023 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants