danepitkin commented on code in PR #368:
URL: https://github.com/apache/arrow-nanoarrow/pull/368#discussion_r1467778778
##########
src/nanoarrow/schema_test.cc:
##########
@@ -574,6 +574,24 @@ TEST(SchemaViewTest, SchemaViewInitErrors) {
EXPECT_STREQ(ArrowErrorMessage(&error),
"Error parsing schema->format 'n*': parsed 1/2 characters");
+ ASSERT_EQ(ArrowSchemaSetFormat(&schema, "n"), NANOARROW_OK);
+ schema.flags = 0;
+ schema.flags |= ARROW_FLAG_DICTIONARY_ORDERED;
+ EXPECT_EQ(ArrowSchemaViewInit(&schema_view, &schema, &error), EINVAL);
+ EXPECT_STREQ(ArrowErrorMessage(&error),
+ "ARROW_FLAG_DICTIONARY_ORDERED is only relevant for
dictionaries");
+
+ schema.flags = 0;
+ schema.flags |= ARROW_FLAG_MAP_KEYS_SORTED;
+ EXPECT_EQ(ArrowSchemaViewInit(&schema_view, &schema, &error), EINVAL);
+ EXPECT_STREQ(ArrowErrorMessage(&error),
+ "ARROW_FLAG_MAP_KEYS_SORTED is only relevant for a map type");
+
+ schema.flags = 0;
+ schema.flags |= (ARROW_FLAG_MAP_KEYS_SORTED << 1);
Review Comment:
We could use the new macro/define here to guarantee we always get an
undefined value.
##########
src/nanoarrow/schema.c:
##########
@@ -1178,16 +1178,32 @@ ArrowErrorCode ArrowSchemaViewInit(struct
ArrowSchemaView* schema_view,
schema_view->type = NANOARROW_TYPE_DICTIONARY;
}
- result = ArrowSchemaViewValidate(schema_view, schema_view->storage_type,
error);
- if (result != NANOARROW_OK) {
- return result;
- }
+ NANOARROW_RETURN_NOT_OK(
+ ArrowSchemaViewValidate(schema_view, schema_view->storage_type, error));
if (schema_view->storage_type != schema_view->type) {
- result = ArrowSchemaViewValidate(schema_view, schema_view->type, error);
- if (result != NANOARROW_OK) {
- return result;
- }
+ NANOARROW_RETURN_NOT_OK(
+ ArrowSchemaViewValidate(schema_view, schema_view->type, error));
+ }
+
+ int64_t unknown_flags = schema->flags & ~ARROW_FLAG_DICTIONARY_ORDERED &
Review Comment:
Would this be better as a macro or #define that is defined next to these
other #defines? From a maintenance perspective, I'm mainly considering what
happens if someone adds a new flag and forgets to update this part of the code.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
