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

ARROW-11838: fix offset buffer in golden file. #60

Merged
merged 4 commits into from Mar 23, 2021

Conversation

jmgpeeters
Copy link
Contributor

Chasing the Java integration errors in apache/arrow#9629 (although C++ passes), it appears I misunderstood the correct layout for the offset buffer (for ["foo", "bar", "baz"] needs to be [0, 3, 6, 9] and not [0, 3, 6]).

I have fixed this and verified locally that it works with both C++ and Java. Sorry about that.

@pitrou has context on this, so probably a good one to review.

@pitrou
Copy link
Member

pitrou commented Mar 18, 2021

Hmm, C++ shouldn't have accepted the non-conformant buffer. Can you try to find where the missing checks need to be added?

@jmgpeeters
Copy link
Contributor Author

Yep, will do.

@jmgpeeters
Copy link
Contributor Author

@pitrou I've implemented some additional checks in the Arrow sister PR. The C++ integration check (https://github.com/apache/arrow/pull/9629/checks?check_run_id=2142504660) now fails with:

RuntimeError: Command failed: /build/cpp/debug/arrow-json-integration-test --integration --arrow=/arrow/testing/data/arrow-ipc-stream/integration/4.0.0-shareddict/generated_shared_dict.arrow_file --json=/tmp/tmprjve0h7q/4.0.0-shareddict_shared_dict.gold.json --mode=VALIDATE
With output:
--------------
Error message: Invalid: JSON OFFSET array size differs from advertised array length + 1

(i.e. because [0, 3, 6] is one too short).

and it has similarly useful errors (verified locally) if

  • OFFSETs is missing altogether
  • one would do e.g. [0, 3, 6, 10] instead of the correct [0, 3, 6, 9]

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

+1, thank you @jmgpeeters

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants