Expose record boundary information in JSON decoder

[scovich]

Which issue does this PR close?

Closes #6522

Rationale for this change

The current JSON decoder gives no way to reliably identify record boundaries in the input (as different from mere whitespace or buffer boundaries), which makes it very difficult to correctly and efficiently parse a series of unrelated JSON values (such as from a StringArray column).

Examples of adversarial inputs include: blank strings (no rows produced), a single string containing multiple records (multiple rows produced), or multiple invalid strings whose concatenation looks like a single record (one row produced).

Such cases can be detected easily by checking the number of records parsed, and whether the last record was incomplete -- but that state is not publicly accessible (buried in the TapeDecoder struct).

What changes are included in this PR?

Expose two new methods on the TapeDecoder struct, which support three new pub methods on Decoder, which exposes the number of records the decoder has buffered up so far, and whether the last record is partial (incomplete).

This allows e.g.

fn parse_one_record(decoder: &mut Decoder, bytes: &[u8]) -> Result<(), ArrowError> {
    let existing_len = decoder.len();
    let decoded_bytes = decoder.decode(bytes)?;
    assert_eq!(decoded_bytes, bytes.len()); // all bytes consumed
    assert_eq!(decoder.len(), existing_len + 1); // exactly one record produced
    assert!(!decoder.has_partial_record()); // the record was complete
    Ok(())
}

Also update documentation and add unit tests.

Are there any user-facing changes?

Three new public methods on Decoder: has_partial_record, len, and is_empty.

[scovich]

A couple of notes for reviewers, in case it's helpful.

[scovich]

Clippy prefers is_empty() over len() == 0, so I added both methods. I debated just exposing num_buffered_records instead, but len+is_empty seemed more rustic.

Happy to adjust the approach based on feedback.

[scovich]

For whatever reason, TapeDecoder seems to call them "rows" while Decoder calls them "records" -- so I named the new methods to match.

[tustvold]

This makes sense to me, my understanding being this allows deserializing StringArray one value at a time, ensuring records are not split across value boundaries.

Whilst this probably has some additional overheads, I'd be curious to see these quantified e.g. compared to the approach of not checking, I suspect these are low relative to the inherent costs of JSON decoding, and such an approach still benefits from the vectorised tape->array conversion.

[scovich]

This makes sense to me, my understanding being this allows deserializing StringArray one value at a time, ensuring records are not split across value boundaries.

That's a good description of what I hoped to achieve, yes.

Whilst this probably has some additional overheads, I'd be curious to see these quantified e.g. compared to the approach of not checking, I suspect these are low relative to the inherent costs of JSON decoding, and such an approach still benefits from the vectorised tape->array conversion.

In the common case where all strings contain correct JSON, the check should be branch-predicted away. It's ultimately just checking two variables that should already be hot in CPU cache, if not in registers, and both branches should be not-taken almost always.

In any case tho -- this enables the user of a Decoder to express correctness constraints they care about, and the small performance overhead would be totally acceptable. The change doesn't impact normal parsing at all.

[scovich]

Whilst this probably has some additional overheads, I'd be curious to see these quantified e.g. compared to the approach of not checking, I suspect these are low relative to the inherent costs of JSON decoding, and such an approach still benefits from the vectorised tape->array conversion.

In the common case where all strings contain correct JSON, the check should be branch-predicted away. It's ultimately just checking two variables that should already be hot in CPU cache, if not in registers, and both branches should be not-taken almost always.

In any case tho -- this enables the user of a Decoder to express correctness constraints they care about, and the small performance overhead would be totally acceptable. The change doesn't impact normal parsing at all.

Actually... sending in a bunch of small (not I/O optimal) strings one at a time will probably be the biggest overhead. If we didn't need boundary validation, we could probably just pass the entire underlying byte array from the StringArray in a single call. But that's unavoidable. Especially because I don't think the underlying byte array is required to be tightly packed (there could be regions of invalid bytes between strings).

[tustvold]

If we didn't need boundary validation, we could probably just pass the entire underlying byte array

Yes this was the approach I was comparing to, which of course would not work for strings with non-empty nulls, or malicious input, and so is probably not a good idea - I'm just curious 😅 (If there is a big difference, which I doubt, it might justify a more intrusive integration).