feat(extensions/nanoarrow_testing): Add nanoarrow_testing extension with testing JSON writer #317

paleolimbot · 2023-11-15T16:48:40Z

This PR adds the first few bits of infrastructure needed to implement integration testing:

nanoarrow_testing.hpp testing utility header
CI to build/run the tests
Batch + Column JSON writer for easy types to get things going

The design of the testing helper library is intentionally header-only to facilitate dropping in to projects where needed (although I'm happy to change that if there are opinions otherwise).

Some obvious follow-ups not included yet:

Implement ArrowSchema -> JSON
Support decimal and interval types
Schema/Array equality checking
JSON -> ArrowSchema
JSON -> ArrowArray

felipecrv · 2023-11-20T16:10:46Z

src/nanoarrow/nanoarrow_testing.hpp

+        out << R"(, "VALIDITY": )";
+        WriteBitmap(out, value->buffer_views[0].data.as_uint8, value->length);


Does the spec requires that a [1, 1, 1...] VALIDITY be provided when the buffer[0] is null? Isn't it OK with omitting the entry altogether? I imagine that is required if we are to have a 1-to-1 mapping between values and JSON representation.

That's a great point...the example files in apache/arrow-testing seem to all have [1, 1, 1, 1] although I agree that it might be nice to know if the bitmap was allocated or not. When nanoarrow gets plugged into the integration tests we'll find out in a hurry if that assumption is correct!

felipecrv · 2023-11-20T16:16:29Z

src/nanoarrow/nanoarrow_testing.hpp

+      out << values[0];
+      for (int64_t i = 1; i < n_values; i++) {
+        out << ", " << static_cast<int64_t>(values[i]);
+      }


This would work without for both branches of the if, right? static_cast<int64_t> of an int64_t becomes a no-op.

felipecrv · 2023-11-20T16:19:19Z

src/nanoarrow/nanoarrow_testing.hpp

+        // Strings
+        out << R"(")" << ArrowArrayViewGetIntUnsafe(value, 0) << R"(")";
+        for (int64_t i = 1; i < value->length; i++) {
+          out << R"(, ")" << ArrowArrayViewGetIntUnsafe(value, i) << R"(")";
+        }
+        break;
+      case NANOARROW_TYPE_UINT64:
+        // Strings


Misplaced // Strings comments?

I'll clarify this...I was trying to highlight the "quotedness" of the values here.

felipecrv · 2023-11-20T16:20:08Z

src/nanoarrow/nanoarrow_testing.hpp

+          out << ", " << ArrowArrayViewGetIntUnsafe(value, i);
+        }
+        break;
+      case NANOARROW_TYPE_INT64:


This case is the same as above, right?

It's subtle, but it generates "1234567" rather than 1234567. I'll make sure the comments make that more clear!

Oh, right! Because of JavaScript's 53-bit limitation on integers.

felipecrv · 2023-11-20T16:27:45Z

src/nanoarrow/nanoarrow_testing_test.cc

+        return ArrowSchemaInitFromType(schema, NANOARROW_TYPE_NA);
+      },
+      [](ArrowArray* array) { return NANOARROW_OK; }, &TestingJSON::WriteColumn,
+      R"({"name": null, "count": 0})");


You probably want to take a look at the Go implementation examples and paste them here as test data in separate files. This way you don't have to think about interesting examples yourself and makes the test more data-driven.

That's a great idea! I'll probably keep these tests (bare minimum to get 100% coverage over the code) and add in those tests as well (for full type coverage!).

paleolimbot

Thank you for the review!

paleolimbot · 2023-11-21T18:42:21Z

src/nanoarrow/nanoarrow_testing.hpp

+        out << R"(, "VALIDITY": )";
+        WriteBitmap(out, value->buffer_views[0].data.as_uint8, value->length);


That's a great point...the example files in apache/arrow-testing seem to all have [1, 1, 1, 1] although I agree that it might be nice to know if the bitmap was allocated or not. When nanoarrow gets plugged into the integration tests we'll find out in a hurry if that assumption is correct!

paleolimbot · 2023-11-21T18:43:49Z

src/nanoarrow/nanoarrow_testing.hpp

+          out << ", " << ArrowArrayViewGetIntUnsafe(value, i);
+        }
+        break;
+      case NANOARROW_TYPE_INT64:


It's subtle, but it generates "1234567" rather than 1234567. I'll make sure the comments make that more clear!

paleolimbot · 2023-11-21T18:44:34Z

src/nanoarrow/nanoarrow_testing.hpp

+        // Strings
+        out << R"(")" << ArrowArrayViewGetIntUnsafe(value, 0) << R"(")";
+        for (int64_t i = 1; i < value->length; i++) {
+          out << R"(, ")" << ArrowArrayViewGetIntUnsafe(value, i) << R"(")";
+        }
+        break;
+      case NANOARROW_TYPE_UINT64:
+        // Strings


I'll clarify this...I was trying to highlight the "quotedness" of the values here.

paleolimbot · 2023-11-21T18:47:24Z

src/nanoarrow/nanoarrow_testing_test.cc

+        return ArrowSchemaInitFromType(schema, NANOARROW_TYPE_NA);
+      },
+      [](ArrowArray* array) { return NANOARROW_OK; }, &TestingJSON::WriteColumn,
+      R"({"name": null, "count": 0})");


That's a great idea! I'll probably keep these tests (bare minimum to get 100% coverage over the code) and add in those tests as well (for full type coverage!).

codecov-commenter · 2023-11-21T20:25:10Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (032cdd9) 87.01% compared to head (649d8b1) 87.17%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
src/nanoarrow/nanoarrow_testing.hpp	97.75%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #317      +/-   ##
==========================================
+ Coverage   87.01%   87.17%   +0.16%     
==========================================
  Files          70       72       +2     
  Lines       10574    10496      -78     
==========================================
- Hits         9201     9150      -51     
+ Misses       1373     1346      -27

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

paleolimbot and others added 19 commits November 15, 2023 11:36

init extension project

d0c64d6

basic structure

aa93a08

with passing int tests

55512d2

double

c733e38

with strings

07011b3

binary

e8cb39f

cmake format

325b2b3

binary, sort of

af4137b

fix binary

9b7d0e1

theoretical union support

ad84284

check large string

6bd7d31

at least some unicode escapes

699554e

more flexible column tester

2020db5

struct

e96572d

tweaks

8f7f27a

more general test

648fdad

check batch

eef48b5

add testing workflow

4ef2bdc

fix name

7da39ea

paleolimbot marked this pull request as ready for review November 16, 2023 20:35

paleolimbot requested a review from zeroshade November 16, 2023 20:39

move testing to main project

a09dfa3

felipecrv reviewed Nov 20, 2023

View reviewed changes

paleolimbot commented Nov 21, 2023

View reviewed changes

paleolimbot added 6 commits November 21, 2023 14:53

clarify quoting

6f32771

make JSON writer a class

54bc350

docs

0919738

maybe fix bundled build

c7d8d23

maybe fix bundled build

fa228a9

one more file

649d8b1

paleolimbot merged commit cb2aa71 into apache:main Nov 21, 2023
27 checks passed

paleolimbot deleted the nanoarrow-testing-ext branch November 22, 2023 14:03

paleolimbot added this to the nanoarrow 0.4.0 milestone Jan 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(extensions/nanoarrow_testing): Add nanoarrow_testing extension with testing JSON writer #317

feat(extensions/nanoarrow_testing): Add nanoarrow_testing extension with testing JSON writer #317

paleolimbot commented Nov 15, 2023 •

edited

Loading

felipecrv Nov 20, 2023

paleolimbot Nov 21, 2023

felipecrv Nov 20, 2023

felipecrv Nov 20, 2023

paleolimbot Nov 21, 2023

felipecrv Nov 20, 2023

paleolimbot Nov 21, 2023

felipecrv Nov 21, 2023

felipecrv Nov 20, 2023

paleolimbot Nov 21, 2023

paleolimbot left a comment

paleolimbot Nov 21, 2023

paleolimbot Nov 21, 2023

paleolimbot Nov 21, 2023

paleolimbot Nov 21, 2023

codecov-commenter commented Nov 21, 2023 •

edited

Loading

		out << R"(, "VALIDITY": )";
		WriteBitmap(out, value->buffer_views[0].data.as_uint8, value->length);

feat(extensions/nanoarrow_testing): Add nanoarrow_testing extension with testing JSON writer #317

feat(extensions/nanoarrow_testing): Add nanoarrow_testing extension with testing JSON writer #317

Conversation

paleolimbot commented Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paleolimbot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 21, 2023 • edited Loading

Codecov Report

paleolimbot commented Nov 15, 2023 •

edited

Loading

codecov-commenter commented Nov 21, 2023 •

edited

Loading