-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: add projection support to TapeDecoder for skipping unknown fields in json parsing (1.4x speedup) #9097
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
base: main
Are you sure you want to change the base?
Conversation
scovich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the idea of skipping unwanted fields -- pure overhead to keep them -- but this PR feels overly complex/nested. I wonder if there's a "flatter" way to handle the situation?
arrow-json/src/reader/tape.rs
Outdated
| const SKIP_IN_STRING: u8 = 1 << 0; // 0x01 | ||
| const SKIP_ESCAPE: u8 = 1 << 1; // 0x02 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just use the hex constants directly, out of curiosity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also -- my intuition is that these two flags are only needed because SkipValue does too much. The newly introduced code has a lot of looping and nesting, where the existing enum variants are quite flat. The difference seems to be that the existing variants hand off to a new state whenever they detect a state change?
So e.g. instead of messing with flags, one might declare three new enum variants, SkipValue, SkipString and SkipEscape, where each nests exclusively inside the one before it? e.g. if the projection skipped field foo, then the following JSON fragment:
{
"foo": {
"bar": "hello\nworld!"
}
}would:
- push a
SkipValueas soon as:detects thatfoois not selected - push a
SkipStringas soon as it hits the opening"of the string - push a
SkipEscapeas soon as it hits the\inside the string - pop once the escape was processed
- pop once the closing
"is found - pop once the next field starts (or whatever is currently the ending condition for
SkipValue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the same token, one would arguably want to push multiple SkipValue states instead of tracking nesting depth with a new variable? But then enum variants start to proliferate (basically need two of each).
Would it instead make sense to have a single skip offset that is the first stack index being skipped?
And then have pairs of match arms that decide what state gets pushed vs. merely traversed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to enum variant in a255860
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, when I ran the benchmark on my laptop:
- Your original PR had 8% overhead (wide run) and 36% benefit (narrow run)
- The revised PR has overhead 9% and benefit 30%
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to remove the regression, I added a projection in ReaderBuilder to enable projection-aware parsing.
When enabled, JSON fields not present in the schema are skipped during tape parsing rather than being fully parsed and later ignored. This improves performance for narrow projections over wide JSON data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I threw an LLM at this whole situation during a boring meeting, and arrived at a surprisingly different potential approach, if you're game to try it out?
The short version is:
- Keep the existing (highly optimized and efficient) decoding logic, but factor it out to a helper method that is generic over
const SKIP: boolthat says whether to actually store the parsed output. - Wrap that helper in
decodeanddecode_skipmethods, with a clean transition between the two: enter at the:match (like the PR does today), anddecode_skipbreaks back out todecodewhen the stack length drops back down. - We need a new boolean
skippingfield to handle cases where input bytes were exhausted while skipping (so the next call todecodecan jump straight todecode_skipwhen starting the next buffer of bytes) - The state stack tracks everything related to skipping (small memory cost but very efficient).
- No new tape decoder enum variants needed.
In theory, the approach should be simpler (less duplicated source code) while also having friendlier branching (fewer and/or more predictable branches).
Is that something you'd want me to put a bit more time into exploring further?
Or something you'd prefer to dig into yourself?
b427eeb to
dfcbc97
Compare
08fcaaa to
8c1f4e9
Compare

Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
This PR implements projection-aware field skipping in the arrow-json reader:
ReaderBuilder::with_projection(bool)enables opt-in field filteringBehavior matrix:
Are these changes tested?
Yes, all existing tests pass
Are there any user-facing changes?
Yes, new public API:
ReaderBuilder::with_projection(bool)- opt-in to skip unknown JSON fields during parsingThis is additive and does not break existing behavior (default is
false).