[Improvement][C++] Add validation for data types for writers in C++ library #136

lixueclaire · 2023-04-14T02:52:44Z

Proposed changes

This update includes the implementation of various validation levels in the GraphAr C++ SDK writers. These new validation levels could ensure the consistency of data types between the data and meta info before being written.

Types of changes

What types of changes does your code introduce to GraphAr?
Put an x in the boxes that apply

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation Update (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

I have read the CONTRIBUTING doc
I have signed the CLA
Lint and unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

Further comments

related to issue #127

github-actions · 2023-04-14T02:53:05Z

🎊 PR Preview 070d6ea has been successfully built and deployed to https://alibaba-graphar-build-pr-136.surge.sh

_{🤖 By surge-preview}

acezen · 2023-04-18T03:00:50Z

cpp/src/arrow_chunk_writer.cc

+                                        " not found");
+      auto field = schema->field(indice);
+      if (DataType::ArrowDataTypeToDataType(field->type()) != property.type)
+        return Status::TypeError("invalid data type for property: " +


We can add more error informations like

std::string err_msg = "Invalid data type for property: " + property.name + " define as " + property.type.ToTypeName() + " but got " + DataType::ArrowDataTypeToDataType(field->type()).TypeName; return Status::TypeError(err_msg)

acezen · 2023-04-18T03:06:36Z

cpp/src/arrow_chunk_writer.cc

+    return Status::OK();
+  // weak validate
+  if (!edge_info_.ContainAdjList(adj_list_type_))
+    return Status::InvalidOperation("invalid adj list type");


Ditto, error msg should be " The adj list type: xxx is not exist in edge info"

acezen · 2023-04-18T03:07:46Z

cpp/src/arrow_chunk_writer.cc

+    return Status::InvalidOperation("invalid adj list type");
+  if (adj_list_type_ != AdjListType::ordered_by_source &&
+      adj_list_type_ != AdjListType::ordered_by_dest)
+    return Status::InvalidOperation("invalid adj list type");


Ditto, "The adj list type has to be ordered_by_source or ordered_by_dest, but got xxx"

acezen · 2023-04-18T03:08:34Z

cpp/src/arrow_chunk_writer.cc

+    return Status::InvalidOperation("invalid adj list type");
+  if (adj_list_type_ == AdjListType::ordered_by_source &&
+      input_table->num_rows() > edge_info_.GetSrcChunkSize() + 1)
+    return Status::OutOfRange();


acezen · 2023-04-18T03:08:43Z

cpp/src/arrow_chunk_writer.cc

+    return Status::OutOfRange();
+  if (adj_list_type_ == AdjListType::ordered_by_dest &&
+      input_table->num_rows() > edge_info_.GetDstChunkSize() + 1)
+    return Status::OutOfRange();


acezen · 2023-04-18T03:09:57Z

cpp/src/arrow_chunk_writer.cc

+    auto schema = input_table->schema();
+    int index = schema->GetFieldIndex(GeneralParams::kOffsetCol);
+    if (index == -1)
+      return Status::InvalidOperation("offsets not provided");


"offset column not provided"

acezen · 2023-04-18T03:11:37Z

cpp/src/arrow_chunk_writer.cc

+      return Status::InvalidOperation("offsets not provided");
+    auto field = schema->field(index);
+    if (field->type()->id() != arrow::Type::INT64)
+      return Status::TypeError("invalid data type for offsets");


"The data type of offset offset column should be INT64, but got "

acezen · 2023-04-18T03:11:56Z

cpp/src/arrow_chunk_writer.cc

+      return Status::InvalidOperation("destinations not provided");
+    field = schema->field(index);
+    if (field->type()->id() != arrow::Type::INT64)
+      return Status::TypeError("invalid data type for destinations");


acezen · 2023-04-18T03:12:17Z

cpp/src/arrow_chunk_writer.cc

+                                        " not found");
+      auto field = schema->field(indice);
+      if (DataType::ArrowDataTypeToDataType(field->type()) != property.type)
+        return Status::TypeError("invalid data type for property: " +


acezen · 2023-04-18T03:15:22Z

cpp/src/arrow_chunk_writer.cc

+  if (index == -1)
+    return Status::InvalidOperation("offsets not provided");
+  indices.push_back(index);
+  auto in_table = input_table->SelectColumns(indices).ValueOrDie();


Make it simple and elegance

GAR_ASSIGN_OR_RAISE(auto in_table, input_table->SelectColumns({index});

lixueclaire · 2023-04-18T05:55:22Z

@acezen, thanks for your suggestions, I have added more error informations when validating.

acezen

LGTM

lixueclaire changed the title ~~[WIP][Improvement][C++] Add validation for data types in VertexPropertyWriter~~ [WIP][Improvement][C++] Add validation for data types for writers in C++ library Apr 14, 2023

lixueclaire changed the title ~~[WIP][Improvement][C++] Add validation for data types for writers in C++ library~~ [Improvement][C++] Add validation for data types for writers in C++ library Apr 14, 2023

lixueclaire requested a review from acezen April 17, 2023 01:34

lixueclaire force-pushed the 127-validate-writer branch 2 times, most recently from c27905b to 8dedfb9 Compare April 18, 2023 01:27

acezen reviewed Apr 18, 2023

View reviewed changes

lixueclaire force-pushed the 127-validate-writer branch from 8dedfb9 to fb903e2 Compare April 18, 2023 05:53

acezen approved these changes Apr 18, 2023

View reviewed changes

lixueclaire added 7 commits April 18, 2023 15:51

Add validation for data types in VertexPropertyWriter

9c2c2f8

Format

b7b6ffc

Add validation for data types in EdgeChunkWriter

be4f36f

Add GetValidateLevel & Fix

829962e

Add validation for writing offsets

86cb63f

Add tests

558c465

Provide more error informations

070d6ea

lixueclaire force-pushed the 127-validate-writer branch from fb903e2 to 070d6ea Compare April 18, 2023 07:53

lixueclaire merged commit 90480a2 into apache:main Apr 18, 2023

lixueclaire deleted the 127-validate-writer branch April 18, 2023 08:15

acezen mentioned this pull request Apr 18, 2023

[Bug]: Inconsistency of data type in GraphAr chunks and in the info yaml file #127

Closed

1 task

lixueclaire mentioned this pull request May 25, 2023

[C++][Improvement] Add validation of different levels for builders in C++ library #171

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Improvement][C++] Add validation for data types for writers in C++ library #136

[Improvement][C++] Add validation for data types for writers in C++ library #136

lixueclaire commented Apr 14, 2023

github-actions bot commented Apr 14, 2023 •

edited

Loading

acezen Apr 18, 2023

acezen Apr 18, 2023

acezen Apr 18, 2023

acezen Apr 18, 2023

acezen Apr 18, 2023

acezen Apr 18, 2023

acezen Apr 18, 2023

acezen Apr 18, 2023

acezen Apr 18, 2023

acezen Apr 18, 2023 •

edited

Loading

lixueclaire commented Apr 18, 2023

acezen left a comment

[Improvement][C++] Add validation for data types for writers in C++ library #136

[Improvement][C++] Add validation for data types for writers in C++ library #136

Conversation

lixueclaire commented Apr 14, 2023

Proposed changes

Types of changes

Checklist

Further comments

github-actions bot commented Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acezen Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

lixueclaire commented Apr 18, 2023

acezen left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 14, 2023 •

edited

Loading

acezen Apr 18, 2023 •

edited

Loading