You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "paleolimbot (via GitHub)" <gi...@apache.org> on 2023/11/27 20:44:46 UTC
[PR] feat: Add integration testing reader for column and batch [arrow-nanoarrow]
paleolimbot opened a new pull request, #325:
URL: https://github.com/apache/arrow-nanoarrow/pull/325
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add integration testing reader for column and batch [arrow-nanoarrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #325:
URL: https://github.com/apache/arrow-nanoarrow/pull/325#discussion_r1409321638
##########
src/nanoarrow/nanoarrow_testing.hpp:
##########
@@ -651,6 +652,33 @@ class TestingJSONReader {
}
}
+ ArrowErrorCode ReadColumn(const std::string& value, const ArrowSchema* schema,
+ ArrowArray* out, ArrowError* error = nullptr) {
+ try {
+ auto obj = json::parse(value);
+
+ // ArrowArrayView to enable validation
+ nanoarrow::UniqueArrayView array_view;
+ NANOARROW_RETURN_NOT_OK(ArrowArrayViewInitFromSchema(
+ array_view.get(), const_cast<ArrowSchema*>(schema), error));
+
+ // ArrowArray to hold memory
+ nanoarrow::UniqueArray array;
+ NANOARROW_RETURN_NOT_OK(
+ ArrowArrayInitFromSchema(array.get(), const_cast<ArrowSchema*>(schema), error));
+
+ // Parse the JSON into the array
+ NANOARROW_RETURN_NOT_OK(SetArrayColumn(obj, array_view.get(), array.get(), error));
+
+ // Return the result
+ ArrowArrayMove(array.get(), out);
+ return NANOARROW_OK;
+ } catch (std::exception& e) {
Review Comment:
Ok, thanks. Yes, the caller shouldn't have to expect an exception here. Do you know if the `json` library only throws its own exception subclasses?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add integration testing reader for column and batch [arrow-nanoarrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #325:
URL: https://github.com/apache/arrow-nanoarrow/pull/325#discussion_r1409320692
##########
src/nanoarrow/nanoarrow_testing.hpp:
##########
@@ -1053,6 +1081,390 @@ class TestingJSONReader {
return NANOARROW_OK;
}
+ ArrowErrorCode SetArrayColumn(const json& value, ArrowArrayView* array_view,
+ ArrowArray* array, ArrowError* error,
+ const std::string& parent_error_prefix = "") {
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.is_object(), error, "Expected Column to be a JSON object"));
+
+ // Check + resolve name early to generate better error messages
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.contains("name"), error, "Column missing key 'name'"));
+
+ const auto& name = value["name"];
+ NANOARROW_RETURN_NOT_OK(Check(name.is_null() || name.is_string(), error,
+ "Column name must be string or null"));
Review Comment:
Ok. I don't expect this in actual JSON integration files, but it's also not a problem if you accept it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add integration testing reader for column and batch [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #325:
URL: https://github.com/apache/arrow-nanoarrow/pull/325#discussion_r1409389042
##########
src/nanoarrow/nanoarrow_testing.hpp:
##########
@@ -651,6 +652,33 @@ class TestingJSONReader {
}
}
+ ArrowErrorCode ReadColumn(const std::string& value, const ArrowSchema* schema,
Review Comment:
Good call, I updated this + added documentation stubs!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add integration testing reader for Column [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #325:
URL: https://github.com/apache/arrow-nanoarrow/pull/325
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add integration testing reader for column and batch [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #325:
URL: https://github.com/apache/arrow-nanoarrow/pull/325#discussion_r1409308912
##########
src/nanoarrow/nanoarrow_testing.hpp:
##########
@@ -651,6 +652,33 @@ class TestingJSONReader {
}
}
+ ArrowErrorCode ReadColumn(const std::string& value, const ArrowSchema* schema,
+ ArrowArray* out, ArrowError* error = nullptr) {
+ try {
+ auto obj = json::parse(value);
+
+ // ArrowArrayView to enable validation
+ nanoarrow::UniqueArrayView array_view;
+ NANOARROW_RETURN_NOT_OK(ArrowArrayViewInitFromSchema(
+ array_view.get(), const_cast<ArrowSchema*>(schema), error));
+
+ // ArrowArray to hold memory
+ nanoarrow::UniqueArray array;
+ NANOARROW_RETURN_NOT_OK(
+ ArrowArrayInitFromSchema(array.get(), const_cast<ArrowSchema*>(schema), error));
+
+ // Parse the JSON into the array
+ NANOARROW_RETURN_NOT_OK(SetArrayColumn(obj, array_view.get(), array.get(), error));
+
+ // Return the result
+ ArrowArrayMove(array.get(), out);
+ return NANOARROW_OK;
+ } catch (std::exception& e) {
Review Comment:
I think any usage of the `json` object methods *can* throw (although I try to avoid it with `Check()`). In the medium-likely event I forgot something it's probably more useful if the caller of `ReadColumn()` doesn't have to handle them?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add integration testing reader for column and batch [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #325:
URL: https://github.com/apache/arrow-nanoarrow/pull/325#discussion_r1409293534
##########
src/nanoarrow/nanoarrow_testing.hpp:
##########
@@ -651,6 +652,33 @@ class TestingJSONReader {
}
}
+ ArrowErrorCode ReadColumn(const std::string& value, const ArrowSchema* schema,
+ ArrowArray* out, ArrowError* error = nullptr) {
+ try {
+ auto obj = json::parse(value);
+
+ // ArrowArrayView to enable validation
+ nanoarrow::UniqueArrayView array_view;
+ NANOARROW_RETURN_NOT_OK(ArrowArrayViewInitFromSchema(
+ array_view.get(), const_cast<ArrowSchema*>(schema), error));
Review Comment:
Definitely! #215 is the issue I was planning for that one (there are a lot of places where read-only input arguments should be marked `const`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add integration testing reader for column and batch [arrow-nanoarrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #325:
URL: https://github.com/apache/arrow-nanoarrow/pull/325#discussion_r1409427823
##########
python/src/nanoarrow/nanoarrow_testing.hpp:
##########
@@ -0,0 +1,1508 @@
+// Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
Did you add this file by mistake?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add integration testing reader for column and batch [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #325:
URL: https://github.com/apache/arrow-nanoarrow/pull/325#discussion_r1409388475
##########
src/nanoarrow/nanoarrow_testing.hpp:
##########
@@ -651,6 +652,33 @@ class TestingJSONReader {
}
}
+ ArrowErrorCode ReadColumn(const std::string& value, const ArrowSchema* schema,
+ ArrowArray* out, ArrowError* error = nullptr) {
+ try {
+ auto obj = json::parse(value);
+
+ // ArrowArrayView to enable validation
+ nanoarrow::UniqueArrayView array_view;
+ NANOARROW_RETURN_NOT_OK(ArrowArrayViewInitFromSchema(
+ array_view.get(), const_cast<ArrowSchema*>(schema), error));
+
+ // ArrowArray to hold memory
+ nanoarrow::UniqueArray array;
+ NANOARROW_RETURN_NOT_OK(
+ ArrowArrayInitFromSchema(array.get(), const_cast<ArrowSchema*>(schema), error));
+
+ // Parse the JSON into the array
+ NANOARROW_RETURN_NOT_OK(SetArrayColumn(obj, array_view.get(), array.get(), error));
+
+ // Return the result
+ ArrowArrayMove(array.get(), out);
+ return NANOARROW_OK;
+ } catch (std::exception& e) {
Review Comment:
Good call, I updated to catch just `json::exception`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add integration testing reader for column and batch [arrow-nanoarrow]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #325:
URL: https://github.com/apache/arrow-nanoarrow/pull/325#issuecomment-1828576328
## [Codecov](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/325?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`d37d2b0`)](https://app.codecov.io/gh/apache/arrow-nanoarrow/commit/d37d2b0dd2d37ee682582d01c391a1aec5e43d1f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 87.62% compared to head [(`e710119`)](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/325?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 89.10%.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #325 +/- ##
==========================================
+ Coverage 87.62% 89.10% +1.48%
==========================================
Files 72 4 -68
Lines 10906 101 -10805
==========================================
- Hits 9556 90 -9466
+ Misses 1350 11 -1339
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/325?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add integration testing reader for column and batch [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #325:
URL: https://github.com/apache/arrow-nanoarrow/pull/325#discussion_r1409387487
##########
src/nanoarrow/nanoarrow_testing.hpp:
##########
@@ -1053,6 +1081,390 @@ class TestingJSONReader {
return NANOARROW_OK;
}
+ ArrowErrorCode SetArrayColumn(const json& value, ArrowArrayView* array_view,
+ ArrowArray* array, ArrowError* error,
+ const std::string& parent_error_prefix = "") {
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.is_object(), error, "Expected Column to be a JSON object"));
+
+ // Check + resolve name early to generate better error messages
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.contains("name"), error, "Column missing key 'name'"));
+
+ const auto& name = value["name"];
+ NANOARROW_RETURN_NOT_OK(Check(name.is_null() || name.is_string(), error,
+ "Column name must be string or null"));
+
+ std::string error_prefix;
+ if (name.is_string()) {
+ error_prefix = parent_error_prefix + "-> Column '" + name.get<std::string>() + "' ";
+ } else {
+ error_prefix = parent_error_prefix + "-> Column <name is null> ";
+ }
+
+ // Check, resolve, and recurse children
+ NANOARROW_RETURN_NOT_OK(
+ Check(array_view->n_children == 0 || value.contains("children"), error,
+ error_prefix + "missing key children"));
+
+ if (value.contains("children")) {
+ const auto& children = value["children"];
+ NANOARROW_RETURN_NOT_OK(
+ Check(children.is_array(), error, error_prefix + "children must be array"));
+ NANOARROW_RETURN_NOT_OK(Check(children.size() == array_view->n_children, error,
+ error_prefix + "children has incorrect size"));
+
+ for (int64_t i = 0; i < array_view->n_children; i++) {
+ NANOARROW_RETURN_NOT_OK(SetArrayColumn(children[i], array_view->children[i],
+ array->children[i], error, error_prefix));
+ }
+ }
+
+ // Build buffers
+ for (int i = 0; i < 3; i++) {
Review Comment:
I added a define for this (`NANOARROW_MAX_FIXED_BUFFERS`). The `ArrowArrayView` and `ArrowLayout` define fixed-length arrays for buffer things and it's a pretty common pattern to loop over all three and switch on `layout.buffer_type[i]`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add integration testing reader for column and batch [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #325:
URL: https://github.com/apache/arrow-nanoarrow/pull/325#discussion_r1409299772
##########
src/nanoarrow/nanoarrow_testing.hpp:
##########
@@ -1053,6 +1081,390 @@ class TestingJSONReader {
return NANOARROW_OK;
}
+ ArrowErrorCode SetArrayColumn(const json& value, ArrowArrayView* array_view,
+ ArrowArray* array, ArrowError* error,
+ const std::string& parent_error_prefix = "") {
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.is_object(), error, "Expected Column to be a JSON object"));
+
+ // Check + resolve name early to generate better error messages
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.contains("name"), error, "Column missing key 'name'"));
+
+ const auto& name = value["name"];
+ NANOARROW_RETURN_NOT_OK(Check(name.is_null() || name.is_string(), error,
+ "Column name must be string or null"));
Review Comment:
I think `name` MAY be null (at least in the C data spec: https://arrow.apache.org/docs/format/CDataInterface.html#c.ArrowSchema.name ), so the writer serializes `name == NULL` as a JSON null (rather than omitting or setting to `""`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add integration testing reader for column and batch [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #325:
URL: https://github.com/apache/arrow-nanoarrow/pull/325#discussion_r1409383744
##########
src/nanoarrow/nanoarrow_testing.hpp:
##########
@@ -1053,6 +1081,390 @@ class TestingJSONReader {
return NANOARROW_OK;
}
+ ArrowErrorCode SetArrayColumn(const json& value, ArrowArrayView* array_view,
+ ArrowArray* array, ArrowError* error,
+ const std::string& parent_error_prefix = "") {
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.is_object(), error, "Expected Column to be a JSON object"));
+
+ // Check + resolve name early to generate better error messages
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.contains("name"), error, "Column missing key 'name'"));
+
+ const auto& name = value["name"];
+ NANOARROW_RETURN_NOT_OK(Check(name.is_null() || name.is_string(), error,
+ "Column name must be string or null"));
+
+ std::string error_prefix;
+ if (name.is_string()) {
+ error_prefix = parent_error_prefix + "-> Column '" + name.get<std::string>() + "' ";
+ } else {
+ error_prefix = parent_error_prefix + "-> Column <name is null> ";
+ }
+
+ // Check, resolve, and recurse children
+ NANOARROW_RETURN_NOT_OK(
+ Check(array_view->n_children == 0 || value.contains("children"), error,
+ error_prefix + "missing key children"));
+
+ if (value.contains("children")) {
+ const auto& children = value["children"];
+ NANOARROW_RETURN_NOT_OK(
+ Check(children.is_array(), error, error_prefix + "children must be array"));
+ NANOARROW_RETURN_NOT_OK(Check(children.size() == array_view->n_children, error,
+ error_prefix + "children has incorrect size"));
+
+ for (int64_t i = 0; i < array_view->n_children; i++) {
+ NANOARROW_RETURN_NOT_OK(SetArrayColumn(children[i], array_view->children[i],
+ array->children[i], error, error_prefix));
+ }
+ }
+
+ // Build buffers
+ for (int i = 0; i < 3; i++) {
+ NANOARROW_RETURN_NOT_OK(
+ PrefixError(SetArrayColumnBuffers(value, array_view, array, i, error), error,
+ error_prefix));
+ }
+
+ // Check + resolve count
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.contains("count"), error, error_prefix + "missing key 'count'"));
+ const auto& count = value["count"];
+ NANOARROW_RETURN_NOT_OK(
+ Check(count.is_number_integer(), error, error_prefix + "count must be integer"));
+ array_view->length = count.get<int64_t>();
+
+ // Set ArrayView buffer views. This is because ArrowArrayInitFromSchema() doesn't
+ // support custom type ids for unions but the ArrayView does (otherwise
+ // ArrowArrayFinishBuilding() would work).
+ for (int i = 0; i < 3; i++) {
+ ArrowBuffer* buffer = ArrowArrayBuffer(array, i);
+ ArrowBufferView* buffer_view = array_view->buffer_views + i;
+ buffer_view->data.as_uint8 = buffer->data;
+ buffer_view->size_bytes = buffer->size_bytes;
+ }
+
+ // Validate the array view
+ NANOARROW_RETURN_NOT_OK(PrefixError(
+ ArrowArrayViewValidate(array_view, NANOARROW_VALIDATION_LEVEL_FULL, error), error,
+ error_prefix + "failed to validate: "));
+
+ // Flush length and buffer pointers to the Array
+ array->length = array_view->length;
+ NANOARROW_RETURN_NOT_OK_WITH_ERROR(
+ ArrowArrayFinishBuilding(array, NANOARROW_VALIDATION_LEVEL_NONE, nullptr), error);
+
+ return NANOARROW_OK;
+ }
+
+ ArrowErrorCode SetArrayColumnBuffers(const json& value, ArrowArrayView* array_view,
+ ArrowArray* array, int buffer_i,
+ ArrowError* error) {
+ ArrowBuffer* buffer = ArrowArrayBuffer(array, buffer_i);
+
+ switch (array_view->layout.buffer_type[buffer_i]) {
+ case NANOARROW_BUFFER_TYPE_VALIDITY: {
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.contains("VALIDITY"), error, "missing key 'VALIDITY'"));
+ const auto& validity = value["VALIDITY"];
+ NANOARROW_RETURN_NOT_OK(
+ SetBufferBitmap(validity, ArrowArrayValidityBitmap(array), error));
+ break;
+ }
+ case NANOARROW_BUFFER_TYPE_TYPE_ID: {
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.contains("TYPE_ID"), error, "missing key 'TYPE_ID'"));
+ const auto& type_id = value["TYPE_ID"];
+ NANOARROW_RETURN_NOT_OK(SetBufferInt<int32_t>(type_id, buffer, error));
+ break;
+ }
+ case NANOARROW_BUFFER_TYPE_UNION_OFFSET: {
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.contains("OFFSET"), error, "missing key 'OFFSET'"));
+ const auto& offset = value["OFFSET"];
+ NANOARROW_RETURN_NOT_OK(SetBufferInt<int32_t>(offset, buffer, error));
+ break;
+ }
+ case NANOARROW_BUFFER_TYPE_DATA_OFFSET: {
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.contains("OFFSET"), error, "missing key 'OFFSET'"));
+ const auto& offset = value["OFFSET"];
+
+ if (array_view->layout.element_size_bits[buffer_i] == 32) {
+ NANOARROW_RETURN_NOT_OK(SetBufferInt<int32_t>(offset, buffer, error));
+ } else {
+ NANOARROW_RETURN_NOT_OK(SetBufferInt<int64_t>(offset, buffer, error));
+ }
+ break;
+ }
+
+ case NANOARROW_BUFFER_TYPE_DATA: {
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.contains("DATA"), error, "missing key 'DATA'"));
+ const auto& data = value["DATA"];
+
+ switch (array_view->storage_type) {
+ case NANOARROW_TYPE_BOOL: {
+ nanoarrow::UniqueBitmap bitmap;
+ NANOARROW_RETURN_NOT_OK(SetBufferBitmap(data, bitmap.get(), error));
+ ArrowBufferMove(&bitmap->buffer, buffer);
+ return NANOARROW_OK;
+ }
+ case NANOARROW_TYPE_INT8:
+ return SetBufferInt<int8_t>(data, buffer, error);
+ case NANOARROW_TYPE_UINT8:
+ return SetBufferInt<uint8_t>(data, buffer, error);
+ case NANOARROW_TYPE_INT16:
+ return SetBufferInt<int16_t>(data, buffer, error);
+ case NANOARROW_TYPE_UINT16:
+ return SetBufferInt<uint16_t>(data, buffer, error);
+ case NANOARROW_TYPE_INT32:
+ return SetBufferInt<int32_t>(data, buffer, error);
+ case NANOARROW_TYPE_UINT32:
+ return SetBufferInt<uint32_t>(data, buffer, error);
+ case NANOARROW_TYPE_INT64:
+ return SetBufferInt<int64_t>(data, buffer, error);
+ case NANOARROW_TYPE_UINT64:
+ return SetBufferInt<uint64_t, uint64_t>(data, buffer, error);
+
+ case NANOARROW_TYPE_FLOAT:
+ return SetBufferFloatingPoint<float>(data, buffer, error);
+ case NANOARROW_TYPE_DOUBLE:
+ return SetBufferFloatingPoint<double>(data, buffer, error);
+
+ case NANOARROW_TYPE_STRING:
+ return SetBuffersString<int32_t>(data, ArrowArrayBuffer(array, buffer_i - 1),
+ buffer, error);
+ case NANOARROW_TYPE_LARGE_STRING:
+ return SetBuffersString<int64_t>(data, ArrowArrayBuffer(array, buffer_i - 1),
+ buffer, error);
+ case NANOARROW_TYPE_BINARY:
+ return SetBuffersBinary<int32_t>(data, ArrowArrayBuffer(array, buffer_i - 1),
+ buffer, error);
+ case NANOARROW_TYPE_LARGE_BINARY:
+ return SetBuffersBinary<int64_t>(data, ArrowArrayBuffer(array, buffer_i - 1),
+ buffer, error);
+ case NANOARROW_TYPE_FIXED_SIZE_BINARY:
+ return SetBuffersBinary<int64_t>(
+ data, nullptr, buffer, error,
+ array_view->layout.element_size_bits[buffer_i] / 8);
Review Comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add integration testing reader for column and batch [arrow-nanoarrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #325:
URL: https://github.com/apache/arrow-nanoarrow/pull/325#discussion_r1409228925
##########
src/nanoarrow/nanoarrow_testing.hpp:
##########
@@ -651,6 +652,33 @@ class TestingJSONReader {
}
}
+ ArrowErrorCode ReadColumn(const std::string& value, const ArrowSchema* schema,
+ ArrowArray* out, ArrowError* error = nullptr) {
+ try {
+ auto obj = json::parse(value);
+
+ // ArrowArrayView to enable validation
+ nanoarrow::UniqueArrayView array_view;
+ NANOARROW_RETURN_NOT_OK(ArrowArrayViewInitFromSchema(
+ array_view.get(), const_cast<ArrowSchema*>(schema), error));
Review Comment:
Perhaps `ArrowArrayViewInitFromSchema` should take a `const ArrowSchema*`?
##########
src/nanoarrow/nanoarrow_testing.hpp:
##########
@@ -651,6 +652,33 @@ class TestingJSONReader {
}
}
+ ArrowErrorCode ReadColumn(const std::string& value, const ArrowSchema* schema,
Review Comment:
What is `value`? Is it the column name? The JSON key name? Something else?
##########
src/nanoarrow/nanoarrow_testing.hpp:
##########
@@ -651,6 +652,33 @@ class TestingJSONReader {
}
}
+ ArrowErrorCode ReadColumn(const std::string& value, const ArrowSchema* schema,
+ ArrowArray* out, ArrowError* error = nullptr) {
+ try {
+ auto obj = json::parse(value);
+
+ // ArrowArrayView to enable validation
+ nanoarrow::UniqueArrayView array_view;
+ NANOARROW_RETURN_NOT_OK(ArrowArrayViewInitFromSchema(
+ array_view.get(), const_cast<ArrowSchema*>(schema), error));
+
+ // ArrowArray to hold memory
+ nanoarrow::UniqueArray array;
+ NANOARROW_RETURN_NOT_OK(
+ ArrowArrayInitFromSchema(array.get(), const_cast<ArrowSchema*>(schema), error));
+
+ // Parse the JSON into the array
+ NANOARROW_RETURN_NOT_OK(SetArrayColumn(obj, array_view.get(), array.get(), error));
+
+ // Return the result
+ ArrowArrayMove(array.get(), out);
+ return NANOARROW_OK;
+ } catch (std::exception& e) {
Review Comment:
What is supposed to throw here? If it's only `json::parse`, can you narrow the scope of the `try` block?
##########
src/nanoarrow/nanoarrow_testing.hpp:
##########
@@ -1053,6 +1081,390 @@ class TestingJSONReader {
return NANOARROW_OK;
}
+ ArrowErrorCode SetArrayColumn(const json& value, ArrowArrayView* array_view,
+ ArrowArray* array, ArrowError* error,
+ const std::string& parent_error_prefix = "") {
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.is_object(), error, "Expected Column to be a JSON object"));
+
+ // Check + resolve name early to generate better error messages
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.contains("name"), error, "Column missing key 'name'"));
+
+ const auto& name = value["name"];
+ NANOARROW_RETURN_NOT_OK(Check(name.is_null() || name.is_string(), error,
+ "Column name must be string or null"));
Review Comment:
Do we expect to receive a null column name? This seems a bit weird.
##########
src/nanoarrow/nanoarrow_testing.hpp:
##########
@@ -1053,6 +1081,390 @@ class TestingJSONReader {
return NANOARROW_OK;
}
+ ArrowErrorCode SetArrayColumn(const json& value, ArrowArrayView* array_view,
+ ArrowArray* array, ArrowError* error,
+ const std::string& parent_error_prefix = "") {
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.is_object(), error, "Expected Column to be a JSON object"));
+
+ // Check + resolve name early to generate better error messages
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.contains("name"), error, "Column missing key 'name'"));
+
+ const auto& name = value["name"];
+ NANOARROW_RETURN_NOT_OK(Check(name.is_null() || name.is_string(), error,
+ "Column name must be string or null"));
+
+ std::string error_prefix;
+ if (name.is_string()) {
+ error_prefix = parent_error_prefix + "-> Column '" + name.get<std::string>() + "' ";
+ } else {
+ error_prefix = parent_error_prefix + "-> Column <name is null> ";
+ }
+
+ // Check, resolve, and recurse children
+ NANOARROW_RETURN_NOT_OK(
+ Check(array_view->n_children == 0 || value.contains("children"), error,
+ error_prefix + "missing key children"));
+
+ if (value.contains("children")) {
+ const auto& children = value["children"];
+ NANOARROW_RETURN_NOT_OK(
+ Check(children.is_array(), error, error_prefix + "children must be array"));
+ NANOARROW_RETURN_NOT_OK(Check(children.size() == array_view->n_children, error,
+ error_prefix + "children has incorrect size"));
+
+ for (int64_t i = 0; i < array_view->n_children; i++) {
+ NANOARROW_RETURN_NOT_OK(SetArrayColumn(children[i], array_view->children[i],
+ array->children[i], error, error_prefix));
+ }
+ }
+
+ // Build buffers
+ for (int i = 0; i < 3; i++) {
+ NANOARROW_RETURN_NOT_OK(
+ PrefixError(SetArrayColumnBuffers(value, array_view, array, i, error), error,
+ error_prefix));
+ }
+
+ // Check + resolve count
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.contains("count"), error, error_prefix + "missing key 'count'"));
+ const auto& count = value["count"];
+ NANOARROW_RETURN_NOT_OK(
+ Check(count.is_number_integer(), error, error_prefix + "count must be integer"));
+ array_view->length = count.get<int64_t>();
+
+ // Set ArrayView buffer views. This is because ArrowArrayInitFromSchema() doesn't
+ // support custom type ids for unions but the ArrayView does (otherwise
+ // ArrowArrayFinishBuilding() would work).
+ for (int i = 0; i < 3; i++) {
+ ArrowBuffer* buffer = ArrowArrayBuffer(array, i);
+ ArrowBufferView* buffer_view = array_view->buffer_views + i;
+ buffer_view->data.as_uint8 = buffer->data;
+ buffer_view->size_bytes = buffer->size_bytes;
+ }
+
+ // Validate the array view
+ NANOARROW_RETURN_NOT_OK(PrefixError(
+ ArrowArrayViewValidate(array_view, NANOARROW_VALIDATION_LEVEL_FULL, error), error,
+ error_prefix + "failed to validate: "));
+
+ // Flush length and buffer pointers to the Array
+ array->length = array_view->length;
+ NANOARROW_RETURN_NOT_OK_WITH_ERROR(
+ ArrowArrayFinishBuilding(array, NANOARROW_VALIDATION_LEVEL_NONE, nullptr), error);
+
+ return NANOARROW_OK;
+ }
+
+ ArrowErrorCode SetArrayColumnBuffers(const json& value, ArrowArrayView* array_view,
+ ArrowArray* array, int buffer_i,
+ ArrowError* error) {
+ ArrowBuffer* buffer = ArrowArrayBuffer(array, buffer_i);
+
+ switch (array_view->layout.buffer_type[buffer_i]) {
+ case NANOARROW_BUFFER_TYPE_VALIDITY: {
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.contains("VALIDITY"), error, "missing key 'VALIDITY'"));
+ const auto& validity = value["VALIDITY"];
+ NANOARROW_RETURN_NOT_OK(
+ SetBufferBitmap(validity, ArrowArrayValidityBitmap(array), error));
+ break;
+ }
+ case NANOARROW_BUFFER_TYPE_TYPE_ID: {
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.contains("TYPE_ID"), error, "missing key 'TYPE_ID'"));
+ const auto& type_id = value["TYPE_ID"];
+ NANOARROW_RETURN_NOT_OK(SetBufferInt<int32_t>(type_id, buffer, error));
+ break;
+ }
+ case NANOARROW_BUFFER_TYPE_UNION_OFFSET: {
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.contains("OFFSET"), error, "missing key 'OFFSET'"));
+ const auto& offset = value["OFFSET"];
+ NANOARROW_RETURN_NOT_OK(SetBufferInt<int32_t>(offset, buffer, error));
+ break;
+ }
+ case NANOARROW_BUFFER_TYPE_DATA_OFFSET: {
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.contains("OFFSET"), error, "missing key 'OFFSET'"));
+ const auto& offset = value["OFFSET"];
+
+ if (array_view->layout.element_size_bits[buffer_i] == 32) {
+ NANOARROW_RETURN_NOT_OK(SetBufferInt<int32_t>(offset, buffer, error));
+ } else {
+ NANOARROW_RETURN_NOT_OK(SetBufferInt<int64_t>(offset, buffer, error));
+ }
+ break;
+ }
+
+ case NANOARROW_BUFFER_TYPE_DATA: {
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.contains("DATA"), error, "missing key 'DATA'"));
+ const auto& data = value["DATA"];
+
+ switch (array_view->storage_type) {
+ case NANOARROW_TYPE_BOOL: {
+ nanoarrow::UniqueBitmap bitmap;
+ NANOARROW_RETURN_NOT_OK(SetBufferBitmap(data, bitmap.get(), error));
+ ArrowBufferMove(&bitmap->buffer, buffer);
+ return NANOARROW_OK;
+ }
+ case NANOARROW_TYPE_INT8:
+ return SetBufferInt<int8_t>(data, buffer, error);
+ case NANOARROW_TYPE_UINT8:
+ return SetBufferInt<uint8_t>(data, buffer, error);
+ case NANOARROW_TYPE_INT16:
+ return SetBufferInt<int16_t>(data, buffer, error);
+ case NANOARROW_TYPE_UINT16:
+ return SetBufferInt<uint16_t>(data, buffer, error);
+ case NANOARROW_TYPE_INT32:
+ return SetBufferInt<int32_t>(data, buffer, error);
+ case NANOARROW_TYPE_UINT32:
+ return SetBufferInt<uint32_t>(data, buffer, error);
+ case NANOARROW_TYPE_INT64:
+ return SetBufferInt<int64_t>(data, buffer, error);
+ case NANOARROW_TYPE_UINT64:
+ return SetBufferInt<uint64_t, uint64_t>(data, buffer, error);
+
+ case NANOARROW_TYPE_FLOAT:
+ return SetBufferFloatingPoint<float>(data, buffer, error);
+ case NANOARROW_TYPE_DOUBLE:
+ return SetBufferFloatingPoint<double>(data, buffer, error);
+
+ case NANOARROW_TYPE_STRING:
+ return SetBuffersString<int32_t>(data, ArrowArrayBuffer(array, buffer_i - 1),
+ buffer, error);
+ case NANOARROW_TYPE_LARGE_STRING:
+ return SetBuffersString<int64_t>(data, ArrowArrayBuffer(array, buffer_i - 1),
+ buffer, error);
+ case NANOARROW_TYPE_BINARY:
+ return SetBuffersBinary<int32_t>(data, ArrowArrayBuffer(array, buffer_i - 1),
+ buffer, error);
+ case NANOARROW_TYPE_LARGE_BINARY:
+ return SetBuffersBinary<int64_t>(data, ArrowArrayBuffer(array, buffer_i - 1),
+ buffer, error);
+ case NANOARROW_TYPE_FIXED_SIZE_BINARY:
+ return SetBuffersBinary<int64_t>(
+ data, nullptr, buffer, error,
+ array_view->layout.element_size_bits[buffer_i] / 8);
Review Comment:
It would probably be more readable if there was a separate helper function for fixed-size binary?
##########
src/nanoarrow/nanoarrow_testing.hpp:
##########
@@ -1053,6 +1081,390 @@ class TestingJSONReader {
return NANOARROW_OK;
}
+ ArrowErrorCode SetArrayColumn(const json& value, ArrowArrayView* array_view,
+ ArrowArray* array, ArrowError* error,
+ const std::string& parent_error_prefix = "") {
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.is_object(), error, "Expected Column to be a JSON object"));
+
+ // Check + resolve name early to generate better error messages
+ NANOARROW_RETURN_NOT_OK(
+ Check(value.contains("name"), error, "Column missing key 'name'"));
+
+ const auto& name = value["name"];
+ NANOARROW_RETURN_NOT_OK(Check(name.is_null() || name.is_string(), error,
+ "Column name must be string or null"));
+
+ std::string error_prefix;
+ if (name.is_string()) {
+ error_prefix = parent_error_prefix + "-> Column '" + name.get<std::string>() + "' ";
+ } else {
+ error_prefix = parent_error_prefix + "-> Column <name is null> ";
+ }
+
+ // Check, resolve, and recurse children
+ NANOARROW_RETURN_NOT_OK(
+ Check(array_view->n_children == 0 || value.contains("children"), error,
+ error_prefix + "missing key children"));
+
+ if (value.contains("children")) {
+ const auto& children = value["children"];
+ NANOARROW_RETURN_NOT_OK(
+ Check(children.is_array(), error, error_prefix + "children must be array"));
+ NANOARROW_RETURN_NOT_OK(Check(children.size() == array_view->n_children, error,
+ error_prefix + "children has incorrect size"));
+
+ for (int64_t i = 0; i < array_view->n_children; i++) {
+ NANOARROW_RETURN_NOT_OK(SetArrayColumn(children[i], array_view->children[i],
+ array->children[i], error, error_prefix));
+ }
+ }
+
+ // Build buffers
+ for (int i = 0; i < 3; i++) {
Review Comment:
Why the hardcoded 3?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org