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.

Sign up for GitHub

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

Jump to bottom

Fix: #249 panic during head skipping #253

Merged
merged 4 commits into from
Sep 9, 2023
Merged

Fix: #249 panic during head skipping #253

merged 4 commits into from
Sep 9, 2023

Conversation

V0ldek
Copy link
Member

@V0ldek V0ldek commented Sep 8, 2023

Short description

Track down, reproduce, and fix the bug in head skipping caused by one of two scenarios:

  • "long" (longer than block size) keys being looked for by head skip;
  • very specific scenario where the end of a subtree (the closing character) is exactly at the end of a block, the key being looked for is found in the next block, but whitespace cause the colon to occur in the next-next block (I know).

Issue

Resolves: #249

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.

@V0ldek V0ldek force-pushed the v0ldek/#249-panic-during-head-skipping branch from c4ba4d1 to 848881b Compare September 9, 2023 01:28
zwerdlds
zwerdlds reviewed Sep 9, 2023
View reviewed changes
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 items to think about - the logic changes here are beyond me at the moment, but generally look good.

crates/rsonpath-lib/src/classification.rs Outdated Show resolved Hide resolved
crates/rsonpath-lib/src/classification/quotes/nosimd.rs Show resolved Hide resolved
@V0ldek
chore: apply clippy --fix for nightly lints
2e50d3c
@V0ldek
fix: panic when head-skipping block boundary
70e76bd 
- Fixed an issue when head-skipping acceleration
in nodes result mode would panic in very specific
input circumstances,
or if the input had really long JSON keys.

Ref: #249
@V0ldek
chore: fix invalid log statement
70d647a
@V0ldek
chore: address review comments
66768f2
@V0ldek V0ldek force-pushed the v0ldek/#249-panic-during-head-skipping branch from 848881b to 66768f2 Compare September 9, 2023 21:38
@V0ldek V0ldek enabled auto-merge (squash) September 9, 2023 21:41
@V0ldek V0ldek merged commit e505ab6 into main Sep 9, 2023
49 checks passed
@V0ldek V0ldek deleted the v0ldek/#249-panic-during-head-skipping branch September 9, 2023 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zwerdlds zwerdlds zwerdlds approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Panic when reporting a result during head-skipping
2 participants
@V0ldek @zwerdlds