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

Root-only query handling #283

Merged
merged 4 commits into from
Sep 22, 2023
Merged

Root-only query handling #283

merged 4 commits into from
Sep 22, 2023

Conversation

V0ldek
Copy link
Member

@V0ldek V0ldek commented Sep 22, 2023

Short description

The primary goal is to support the empty query on arbitrary JSON documents to behave as one would expect.

$ rq '$' ./empty_file.json -r count
0
$ rq '$'
$      {}          
{}
$ rq '$'
$ true
true

To facilitate that, we move the empty query special case over to a module and specialize all result modes. Most of them are really simple – asking for count is just asking if the root exists (count of 1) or not (count of 0) and can be done without looking at most of the input. All of those specializations assume valid input JSONs.

As an apropos fix, internal block padding was changed from null bytes to spaces – null bytes should not appear in a valid JSON at any point, and whitespace are properly neutral to the engine.

Issue

Resolves: #160

Checklist

All of these should be ticked off before you submit the PR.

  • I ran just verify locally and it succeeded.
  • Issue was given go ahead and is linked above OR I have included justification for a minor change.
  • Unit tests for my changes are included OR no functionality was changed.

- Previously only object and array roots were supported.

Ref: #160
Copy link
Collaborator

@charles-paperman charles-paperman left a comment

Choose a reason for hiding this comment

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

Look through it quickly. Sounds good.

@charles-paperman charles-paperman merged commit 2a88209 into main Sep 22, 2023
45 checks passed
@charles-paperman charles-paperman deleted the v0ldek/#160-atomic-roots branch September 22, 2023 11:54
@zwerdlds zwerdlds assigned zwerdlds and unassigned zwerdlds Sep 22, 2023
Copy link
Collaborator

@zwerdlds zwerdlds left a comment

Choose a reason for hiding this comment

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

Just a couple of style/legibility things for thinking on

@@ -169,7 +180,7 @@ pub(super) mod in_slice {

#[inline]
pub(super) fn pad_last_block(bytes: &[u8]) -> LastBlock {
let mut last_block_buf = [0; MAX_BLOCK_SIZE];
let mut last_block_buf = [b' '; MAX_BLOCK_SIZE];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd consider moving this to a constant, if possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Though I guess for a test it's ok.

#[inline(always)]
#[must_use]
pub(crate) fn is_json_whitespace(x: u8) -> bool {
x == b' ' || x == b'\t' || x == b'\n' || x == b'\r'
Copy link
Collaborator

Choose a reason for hiding this comment

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

No opinion on perf, but legibility here might be improved with a match

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.

Atomic roots are not given as a result for an empty query $
3 participants