fix: set map entries/key to non-nullable #107
Conversation
Codecov Report
@@ Coverage Diff @@
## main #107 +/- ##
==========================================
+ Coverage 92.88% 92.91% +0.03%
==========================================
Files 7 7
Lines 1644 1652 +8
Branches 52 52
==========================================
+ Hits 1527 1535 +8
Misses 84 84
Partials 33 33
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Hmm, seems na_map in R does trip the new validation |
|
Not sure if this is the right way to fix the R issue - maybe we should copy the key type and set it to non-nullable in na_map? |
src/nanoarrow/schema_test.cc
Outdated
| ASSERT_STREQ("entries", schema.children[0]->name); | ||
| ASSERT_STREQ("key", schema.children[0]->children[0]->name); | ||
| ASSERT_STREQ("value", schema.children[0]->children[1]->name); | ||
|
|
||
| ASSERT_FALSE(schema.children[0]->flags & ARROW_FLAG_NULLABLE); | ||
| ASSERT_FALSE(schema.children[0]->children[0]->flags & ARROW_FLAG_NULLABLE); |
There was a problem hiding this comment.
Is there a reason why these are ASSERTs and not EXPECTs? The other expectations I've written also use XXX(actual, expected) (but I never looked up which was correct or if there is a correct way to do it...that's just how R's testing works).
There was a problem hiding this comment.
ah, no particular reason other than habit.
IIRC, in Googletest:
- Assert aborts the test if it fails, Expect lets it continue
- Googletest goes
expected, actual
There was a problem hiding this comment.
Hmm, though the actual docs don't list this - they just use val1, val2. I'll look at it tomorrow
There was a problem hiding this comment.
Made them consistent with the surrounding code.
Fixes #106.