You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/11/25 20:24:02 UTC
[GitHub] [arrow-nanoarrow] lidavidm opened a new pull request, #74: [C] Fix some MAP type support issues
lidavidm opened a new pull request, #74:
URL: https://github.com/apache/arrow-nanoarrow/pull/74
Generally, make sure MAP is consistent with LIST:
- MAP has 2 buffers (validity + offset)
- MAP must be finished like LIST (we could go further and have FinishElement also finish the child in the case of MAP, since we know it must always be STRUCT)
--
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
[GitHub] [arrow-nanoarrow] lidavidm commented on pull request #74: [C] Fix some MAP type support issues
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #74:
URL: https://github.com/apache/arrow-nanoarrow/pull/74#issuecomment-1329317450
Map is a "logical type" so that doc defers an explanation to Schema.fbs which has
```
/// A Map is a logical nested type that is represented as
///
/// List<entries: Struct<key: K, value: V>>
```
Maybe we should explicitly list out the logical types so you can control+F easily though?
--
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
[GitHub] [arrow-nanoarrow] paleolimbot commented on a diff in pull request #74: [C] Fix some MAP type support issues
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on code in PR #74:
URL: https://github.com/apache/arrow-nanoarrow/pull/74#discussion_r1033644933
##########
src/nanoarrow/array_test.cc:
##########
@@ -849,6 +849,79 @@ TEST(ArrayTest, ArrayTestAppendToLargeListArray) {
EXPECT_TRUE(arrow_array.ValueUnsafe()->Equals(expected_array.ValueUnsafe()));
}
+TEST(ArrayTest, ArrayTestAppendToMapArray) {
+ struct ArrowArray array;
+ struct ArrowSchema schema;
+ struct ArrowError error;
+
+ ASSERT_EQ(ArrowSchemaInit(&schema, NANOARROW_TYPE_MAP), NANOARROW_OK);
+ ASSERT_EQ(ArrowSchemaAllocateChildren(&schema, 1), NANOARROW_OK);
+ ASSERT_EQ(ArrowSchemaInit(schema.children[0], NANOARROW_TYPE_STRUCT), NANOARROW_OK);
+ ASSERT_EQ(ArrowSchemaSetName(schema.children[0], "item"), NANOARROW_OK);
Review Comment:
```suggestion
ASSERT_EQ(ArrowSchemaSetName(schema.children[0], "entries"), NANOARROW_OK);
```
##########
src/nanoarrow/array_test.cc:
##########
@@ -849,6 +849,79 @@ TEST(ArrayTest, ArrayTestAppendToLargeListArray) {
EXPECT_TRUE(arrow_array.ValueUnsafe()->Equals(expected_array.ValueUnsafe()));
}
+TEST(ArrayTest, ArrayTestAppendToMapArray) {
+ struct ArrowArray array;
+ struct ArrowSchema schema;
+ struct ArrowError error;
+
+ ASSERT_EQ(ArrowSchemaInit(&schema, NANOARROW_TYPE_MAP), NANOARROW_OK);
+ ASSERT_EQ(ArrowSchemaAllocateChildren(&schema, 1), NANOARROW_OK);
+ ASSERT_EQ(ArrowSchemaInit(schema.children[0], NANOARROW_TYPE_STRUCT), NANOARROW_OK);
+ ASSERT_EQ(ArrowSchemaSetName(schema.children[0], "item"), NANOARROW_OK);
+ ASSERT_EQ(ArrowSchemaAllocateChildren(schema.children[0], 2), NANOARROW_OK);
+
+ ASSERT_EQ(ArrowSchemaInit(schema.children[0]->children[0], NANOARROW_TYPE_INT32),
+ NANOARROW_OK);
+ ASSERT_EQ(ArrowSchemaSetName(schema.children[0]->children[0], "key"), NANOARROW_OK);
+ ASSERT_EQ(ArrowSchemaInit(schema.children[0]->children[1], NANOARROW_TYPE_STRING),
+ NANOARROW_OK);
+ ASSERT_EQ(ArrowSchemaSetName(schema.children[0]->children[1], "value"), NANOARROW_OK);
+
+ ASSERT_EQ(ArrowArrayInitFromSchema(&array, &schema, nullptr), NANOARROW_OK);
+
+ ASSERT_EQ(ArrowArrayStartAppending(&array), NANOARROW_OK);
+
+ // Check that we can reserve recursively without erroring
+ ASSERT_EQ(ArrowArrayReserve(&array, 5), NANOARROW_OK);
+ EXPECT_EQ(ArrowArrayBuffer(array.children[0], 1)->capacity_bytes, 0);
+
+ struct ArrowStringView string_value;
+ string_value.data = "foobar";
+ string_value.n_bytes = 6;
+ ASSERT_EQ(ArrowArrayAppendInt(array.children[0]->children[0], 123), NANOARROW_OK);
+ ASSERT_EQ(ArrowArrayAppendString(array.children[0]->children[1], string_value),
+ NANOARROW_OK);
+ EXPECT_EQ(ArrowArrayFinishElement(array.children[0]), NANOARROW_OK);
+ EXPECT_EQ(ArrowArrayFinishElement(&array), NANOARROW_OK);
+
+ ASSERT_EQ(ArrowArrayAppendNull(&array, 1), NANOARROW_OK);
+
+ // Make sure number of children is checked at finish
+ array.n_children = 0;
+ EXPECT_EQ(ArrowArrayFinishBuilding(&array, &error), EINVAL);
+ EXPECT_STREQ(ArrowErrorMessage(&error),
+ "Expected 1 child of list array but found 0 child arrays");
Review Comment:
Should probably say "map array" to avoid confusion?
##########
src/nanoarrow/array_test.cc:
##########
@@ -849,6 +849,79 @@ TEST(ArrayTest, ArrayTestAppendToLargeListArray) {
EXPECT_TRUE(arrow_array.ValueUnsafe()->Equals(expected_array.ValueUnsafe()));
}
+TEST(ArrayTest, ArrayTestAppendToMapArray) {
+ struct ArrowArray array;
+ struct ArrowSchema schema;
+ struct ArrowError error;
+
+ ASSERT_EQ(ArrowSchemaInit(&schema, NANOARROW_TYPE_MAP), NANOARROW_OK);
+ ASSERT_EQ(ArrowSchemaAllocateChildren(&schema, 1), NANOARROW_OK);
+ ASSERT_EQ(ArrowSchemaInit(schema.children[0], NANOARROW_TYPE_STRUCT), NANOARROW_OK);
+ ASSERT_EQ(ArrowSchemaSetName(schema.children[0], "item"), NANOARROW_OK);
+ ASSERT_EQ(ArrowSchemaAllocateChildren(schema.children[0], 2), NANOARROW_OK);
+
+ ASSERT_EQ(ArrowSchemaInit(schema.children[0]->children[0], NANOARROW_TYPE_INT32),
+ NANOARROW_OK);
+ ASSERT_EQ(ArrowSchemaSetName(schema.children[0]->children[0], "key"), NANOARROW_OK);
+ ASSERT_EQ(ArrowSchemaInit(schema.children[0]->children[1], NANOARROW_TYPE_STRING),
+ NANOARROW_OK);
+ ASSERT_EQ(ArrowSchemaSetName(schema.children[0]->children[1], "value"), NANOARROW_OK);
+
+ ASSERT_EQ(ArrowArrayInitFromSchema(&array, &schema, nullptr), NANOARROW_OK);
+
+ ASSERT_EQ(ArrowArrayStartAppending(&array), NANOARROW_OK);
+
+ // Check that we can reserve recursively without erroring
+ ASSERT_EQ(ArrowArrayReserve(&array, 5), NANOARROW_OK);
+ EXPECT_EQ(ArrowArrayBuffer(array.children[0], 1)->capacity_bytes, 0);
+
+ struct ArrowStringView string_value;
+ string_value.data = "foobar";
+ string_value.n_bytes = 6;
Review Comment:
```suggestion
struct ArrowStringView string_value = ArrowCharView("foobar");
```
##########
src/nanoarrow/array_test.cc:
##########
@@ -849,6 +849,79 @@ TEST(ArrayTest, ArrayTestAppendToLargeListArray) {
EXPECT_TRUE(arrow_array.ValueUnsafe()->Equals(expected_array.ValueUnsafe()));
}
+TEST(ArrayTest, ArrayTestAppendToMapArray) {
+ struct ArrowArray array;
+ struct ArrowSchema schema;
+ struct ArrowError error;
+
+ ASSERT_EQ(ArrowSchemaInit(&schema, NANOARROW_TYPE_MAP), NANOARROW_OK);
+ ASSERT_EQ(ArrowSchemaAllocateChildren(&schema, 1), NANOARROW_OK);
+ ASSERT_EQ(ArrowSchemaInit(schema.children[0], NANOARROW_TYPE_STRUCT), NANOARROW_OK);
+ ASSERT_EQ(ArrowSchemaSetName(schema.children[0], "item"), NANOARROW_OK);
Review Comment:
Right? (At least that's what it says in the C Data documentation)
--
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
[GitHub] [arrow-nanoarrow] lidavidm commented on pull request #74: [C] Fix some MAP type support issues
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #74:
URL: https://github.com/apache/arrow-nanoarrow/pull/74#issuecomment-1329326614
No, the list sizes are variable. Each list item represents a dictionary, where the Struct values of each list entry are key-value pairs.
--
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
[GitHub] [arrow-nanoarrow] lidavidm commented on pull request #74: [C] Fix some MAP type support issues
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #74:
URL: https://github.com/apache/arrow-nanoarrow/pull/74#issuecomment-1329389856
Ah ok, cool. It would be nice to set up something automated, but otherwise, it would be good to at least document the expected clang-format version somewhere.
--
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
[GitHub] [arrow-nanoarrow] paleolimbot commented on pull request #74: [C] Fix some MAP type support issues
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on PR #74:
URL: https://github.com/apache/arrow-nanoarrow/pull/74#issuecomment-1329368496
> Side note: how do you run clang-format? I don't see it in CI and I don't see a pre-commit
Sorry, forgot this note: I use VSCode and just do Control-shift-P + choose "Format". I can set up something automated, it just hasn't been a problem for me so far.
--
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
[GitHub] [arrow-nanoarrow] lidavidm commented on pull request #74: [C] Fix some MAP type support issues
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #74:
URL: https://github.com/apache/arrow-nanoarrow/pull/74#issuecomment-1329330677
Trying this new issue flow out: https://github.com/apache/arrow/issues/14752
--
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
[GitHub] [arrow-nanoarrow] codecov-commenter commented on pull request #74: [C] Fix some MAP type support issues
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #74:
URL: https://github.com/apache/arrow-nanoarrow/pull/74#issuecomment-1327869144
# [Codecov](https://codecov.io/gh/apache/arrow-nanoarrow/pull/74?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#74](https://codecov.io/gh/apache/arrow-nanoarrow/pull/74?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (992acf6) into [main](https://codecov.io/gh/apache/arrow-nanoarrow/commit/52688b5d12fb5c93414250d5d6829bb741954610?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (52688b5) will **not change** coverage.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## main #74 +/- ##
=======================================
Coverage 91.98% 91.98%
=======================================
Files 6 6
Lines 1498 1498
Branches 48 48
=======================================
Hits 1378 1378
Misses 87 87
Partials 33 33
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-nanoarrow/pull/74?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [src/nanoarrow/array.c](https://codecov.io/gh/apache/arrow-nanoarrow/pull/74/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL25hbm9hcnJvdy9hcnJheS5j) | `90.68% <ø> (ø)` | |
| [src/nanoarrow/array\_inline.h](https://codecov.io/gh/apache/arrow-nanoarrow/pull/74/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL25hbm9hcnJvdy9hcnJheV9pbmxpbmUuaA==) | `100.00% <ø> (ø)` | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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
[GitHub] [arrow-nanoarrow] lidavidm merged pull request #74: [C] Fix some MAP type support issues
Posted by GitBox <gi...@apache.org>.
lidavidm merged PR #74:
URL: https://github.com/apache/arrow-nanoarrow/pull/74
--
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
[GitHub] [arrow-nanoarrow] paleolimbot commented on pull request #74: [C] Fix some MAP type support issues
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on PR #74:
URL: https://github.com/apache/arrow-nanoarrow/pull/74#issuecomment-1329321360
That probably would have helped me get this right the first time!
Am I correct in guessing that all the list items must be of size 1 for a Map?
--
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
[GitHub] [arrow-nanoarrow] lidavidm commented on pull request #74: [C] Fix some MAP type support issues
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #74:
URL: https://github.com/apache/arrow-nanoarrow/pull/74#issuecomment-1327866919
Side note: how do you run clang-format? I don't see it in CI and I don't see a pre-commit
--
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