Conversation
d02c90a to
d9c6d74
Compare
|
@github-actions crossbow submit -g cpp |
|
Revision: a4dea4b Submitted crossbow builds: ursacomputing/crossbow @ actions-9fde492db2
|
a4dea4b to
48626ae
Compare
|
@github-actions crossbow submit -g cpp |
|
Revision: 713496d Submitted crossbow builds: ursacomputing/crossbow @ actions-884a0df4e2
|
|
@pitrou @rok I’ve rebased the parser PR and incorporated the latest feedback, including reusing the The native C++ build and tests pass, but Crossbow’s Do you have any idea what I need to do to fix this? |
|
@Reranko05 No idea without taking a deeper look :-) But I'd like us to merge the chunker PR first and then come back to this one. |
|
@pitrou Okay, will wait until chunker is merged. |
| if (stream.truncated_bytes() != 0) { | ||
| return ParseError("The document is empty"); |
There was a problem hiding this comment.
I don't understand this error message. The doc says:
If truncated_bytes() differs from zero, then the input was truncated maybe because incomplete JSON documents were found at the end of the stream.
There was a problem hiding this comment.
I initially associated it with the empty-document handling from the previous implementation. I have updated the message.
| if (internal::ConsumeJsonWhitespace(input, /*trailing=*/false) == input_size) { | ||
| return Status::OK(); | ||
| } |
There was a problem hiding this comment.
As replied in another comment, let's find out if this is really necessary.
| ASSERT_RAISES(Invalid, | ||
| ParseFromString(options, R"({"known": 1, "ignored": [1,]})", &parsed)); |
There was a problem hiding this comment.
Thanks. Can you also test the error message?
|
|
||
| /// Returns the number of leading whitespace characters when trailing is false, | ||
| /// or the number of trailing whitespace characters when trailing is true. | ||
| // XXX We could try to SIMD-accelerate this routine. |
There was a problem hiding this comment.
The original comment says this, why modify it?
// XXX We could try to SIMD-accelerate this routine but it's called only
// once per chunk and also will presumably examine a minimal amount of bytes.There was a problem hiding this comment.
I removed that part because the helper is now also used by the parser, where it may examine more bytes than the original chunker-only use case. If that assumption is still valid, I can restore the comment.
There was a problem hiding this comment.
I removed that part because the helper is now also used by the parser, where it may examine more bytes than the original chunker-only use case. If that assumption is still valid, I can restore the comment.
Well, can you investigate to find out whether it still applies?
| int64_t ConsumeJsonWhitespace(std::string_view view, bool trailing) { | ||
| if (!trailing) { | ||
| const auto pos = view.find_first_not_of(" \t\r\n"); | ||
| return static_cast<int64_t>(pos == std::string_view::npos ? view.size() : pos); |
There was a problem hiding this comment.
The docstring says "Returns the number of leading whitespace characters", but this actually returns the view size (not 0) when there is no leading whitespace. Why?
There was a problem hiding this comment.
Okay, my bad I got confused. I changed it to
/// Returns the position of the first non-whitespace character when trailing is false,
/// or the number of trailing whitespace characters when trailing is true.
is this fine?
There was a problem hiding this comment.
So it's returning the number of leading/trailing whitespace characters in both cases? Perhaps say it like that to make it appear less confusing?
|
@Reranko05 Can you please follow the PR template for your PR description? |
9fdd065 to
b27b054
Compare
|
@pitrou Addressed most of the comments. I will now investigate the BTW, thanks for the reviews. I know you already have a lot of work on your plate, so I really appreciate you taking the time to go through all of this feedback. I am learning a lot from it :) |
|
@pitrou I investigated this further. When the In both cases, The difference comes afterward: I also tried a fresh The Also regarding the // XXX We could try to SIMD-accelerate this routine, but it is called only
// a few times per input and is expected to examine a small amount of data. |
I see. Perhaps Can you perhaps open an issue on https://github.com/simdjson/simdjson/issues so that we can more insight on this? |
|
@pitrou I was going to open an issue on simdjson, but their issue template asks for the problem to be reproducible with a standalone program using the vendored simdjson source. I tried that, but couldn't reproduce the behavior outside of Arrow, so I don't think I have enough to file an issue yet. Within Arrow, I narrowed it down to |
| # Keep simdjson's threading configuration consistent with Arrow's, | ||
| # which is required for Emscripten where Arrow threading is disabled. | ||
| set(SIMDJSON_ENABLE_THREADS ${ARROW_ENABLE_THREADING}) |
There was a problem hiding this comment.
https://github.com/simdjson/simdjson/blob/master/doc/iterate_many.md
Thread support is only active if thread supported is detected in which case the macro SIMDJSON_THREADS_ENABLED is set. You can also manually pass SIMDJSON_THREADS_ENABLED=1 flag to the library. Otherwise the library runs in single-thread mode.
Note the code section for this auto-detection:
// Is threading enabled?
#if defined(_REENTRANT) || defined(_MT)
#ifndef SIMDJSON_THREADS_ENABLED
#define SIMDJSON_THREADS_ENABLED
#endif
#endif
So even if we do not set the cmake option SIMDJSON_ENABLE_THREADS, we can still get threading enabled
| if (stream.truncated_bytes() != 0) { | ||
| return ParseError("JSON document was truncated"); | ||
| } |
There was a problem hiding this comment.
I think truncated_bytes can also have problems when we have partial utf-8 characters at the end of the input.
Also conceptually, we did not use truncated_bytes in the json chunker. It is now not obvious to me why we use simdjson's api in a different way for the parser. To me this will make the code more brittle and we should be consistent with our api usage.
Note that we could replace the FindLast method in the chunker with a single call to truncated_bytes(). But this can lead to some weird behavior with partial utf-8 characters at the end of the stream. Also, as @pitrou already identified, the api contract of truncated_bytes is a little unclear / difficult to reason about
There was a problem hiding this comment.
Also conceptually, we did not use
truncated_bytesin the json chunker. It is now not obvious to me why we use simdjson's api in a different way for the parser. To me this will make the code more brittle and we should be consistent with our api usage.
Do you want to experiment with this @Reranko05 ?
There was a problem hiding this comment.
Sure, I will try.
This is what the docstring says, which I think explains the fluctuating behaviour you're seeing:
That's if you open an issue in the "bug" category, but I think you can use the more general "standard issue" template and let them decide how they feel about it. |
|
You need to merge/rebase and fix conflicts @Reranko05 , btw. |
|
@pitrou rebased :) |
|
Before i create an issue just to verify I am wording it correctly:- Title : Desc : @pitrou is this fine? |
|
@Reranko05 Yes, that seems good to me. |
Rationale for this change
This PR continues the simdjson migration by replacing the RapidJSON-based parsing implementation used by the JSON parser.
The existing parser uses RapidJSON's SAX/handler interface to parse JSON values and populate Arrow builders. This change replaces that implementation with simdjson's ondemand API while retaining the existing builder and type-inference logic.
Changes
simdjson::ondemand::parser::iterate_many.ResolveSimdjsonResult()consistently when handling simdjson results.Error,Ignore, andInferType.Status::Invalid.Are there any user-facing changes?
No
Was AI used for this PR?
In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.
PR code and description written by:
Reviewed before submission by:
Fixes: #51037