-
Notifications
You must be signed in to change notification settings - Fork 38
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
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
The R change is the best way to solve that right now...those "type" constructors are somewhere between a field and a type and the change you made seems the safest for now.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why these are ASSERT
s and not EXPECT
s? 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made them consistent with the surrounding code.
Fixes #106.