[Improvement][C++] Refine the error message of errors of C++ SDK #192

acezen · 2023-06-25T10:11:42Z

Proposed changes

This change aims to refine the error constructor and fulfill the error message detail for C++ SDK.

Refine the error constructor with parameter pack, so developers can construct the error by only passing the messages to constructor, no need to concatenate as a string.
Fulfill the detail message of error in C++ SDK to let user/developer know what the error is about and why it turn out.

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 issue: #140

github-actions · 2023-06-25T10:12:00Z

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

_{🤖 By surge-preview}

acezen · 2023-06-26T03:06:09Z

@lixueclaire The error messages maybe not very correct for writer and builder , or collections(just base on my understanding), can you take a look to the fulfill and if it's not correct, any improvement is good, thanks a lot.

lixueclaire · 2023-06-26T03:33:57Z

@lixueclaire The error messages maybe not very correct for writer and builder , or collections(just base on my understanding), can you take a look to the fulfill and if it's not correct, any improvement is good, thanks a lot.

Sure, I'll have a look soon. Thank you for making this change.

lixueclaire · 2023-06-26T07:02:55Z

cpp/include/gar/utils/status.h

@@ -69,9 +91,6 @@ enum class StatusCode : unsigned char {
  kKeyError,
  kTypeError,
  kInvalid,
-  kInvalidValue,


Could you please add some comments for the enum class to briefly introduce the types of different status code?

like this: https://arrow.apache.org/docs/cpp/api/support.html#error-return-and-reporting

lixueclaire · 2023-06-26T07:09:39Z

cpp/include/gar/graph_info.h

@@ -851,8 +881,8 @@ class EdgeInfo {
      const PropertyGroup& property_group, AdjListType adj_list_type,
      IdType vertex_chunk_index, IdType edge_chunk_index) const {
    if (!ContainPropertyGroup(property_group, adj_list_type)) {
-      return Status::KeyError(
-          "The edge info does not contain the property group.");
+      return Status::KeyError("The property group: ", property_group,


Add "." in the end. The same for the following changes.

lixueclaire · 2023-06-26T07:11:24Z

cpp/include/gar/reader/arrow_chunk_reader.h

@@ -84,7 +84,9 @@ class VertexPropertyArrowChunkReader {
      chunk_table_.reset();
    }
    if (chunk_index_ >= chunk_num_) {
-      return Status::KeyError();


For me, it is a little confused to decide when to use KeyError, and when to use Invalid

I think, KeyError should use in the scenario that user give a key, we can't find a key in our structure like that, otherwise, use a Invalid, so here raise Invalid is better. Thanks for question, I will fix it

I think, KeyError should use in the scenario that user give a key, we can't find a key in our structure like that, otherwise, use a Invalid, so here raise Invalid is better. Thanks for question, I will fix it

Then, it is not very clear that which can be called as a "Key": internal vertex id? chunk index? property name? property group? vertex/edge label? adj_list type? ....

lixueclaire · 2023-06-26T07:14:37Z

cpp/include/gar/reader/arrow_chunk_reader.h

@@ -348,7 +359,10 @@ class AdjListOffsetArrowChunkReader {
      chunk_table_.reset();
    }
    if (chunk_index_ >= vertex_chunk_num_) {
-      return Status::KeyError();
+      return Status::KeyError("Internal vertex id ", id,


I am not sure that why not to use OutofRange here

good point, as above mention, I think I confused the KeyError and OutOfRange here, in the seek scenario, the given parameter is in a range and it should raise a OutOfRange here. I will double check all the seek operation.

lixueclaire · 2023-06-26T07:20:48Z

cpp/src/graph_info.cc

@@ -108,7 +108,7 @@ Status VertexInfo::Save(const std::string& path) const {

 Result<EdgeInfo> EdgeInfo::Load(std::shared_ptr<Yaml> yaml) {
  if (yaml == nullptr) {
-    return Status::YamlError("yaml is nullptr");
+    return Status::Invalid("yaml is nullptr");


modify the message to "yaml shared pointer is nullptr" to be consistent

lixueclaire · 2023-06-26T07:30:32Z

cpp/src/arrow_chunk_writer.cc

    field = schema->field(index);
    if (field->type()->id() != arrow::Type::INT64) {
      return Status::TypeError(
-          "the data type for destination column should be INT64, but got " +
+          "The data type for destination column should be INT64, but got ",


add "index" before "column" to be consistent

lixueclaire · 2023-06-26T07:30:56Z

cpp/src/arrow_chunk_writer.cc

-    return Status::InvalidOperation("the offset column is not provided");
+  if (index == -1) {
+    return Status::Invalid("The offset column ", GeneralParams::kOffsetCol,
+                           " is not exist in the input table");


does not exist

lixueclaire · 2023-06-26T07:31:08Z

cpp/src/arrow_chunk_writer.cc

+  if (index == -1) {
+    return Status::Invalid("The source index column ",
+                           GeneralParams::kSrcIndexCol,
+                           " is not exist in the input table");


lixueclaire · 2023-06-26T07:31:17Z

cpp/src/arrow_chunk_writer.cc

+  if (index == -1) {
+    return Status::Invalid("The destination index column ",
+                           GeneralParams::kDstIndexCol,
+                           " is not exist in the input table");


lixueclaire · 2023-06-26T07:31:26Z

cpp/src/arrow_chunk_writer.cc

+      return Status::Invalid("Property ", property.name, " of property group ",
+                             property_group, " of edge ",
+                             edge_info_.GetEdgeLabel(),
+                             " is not exist in the input table.");


Signed-off-by: acezen <qiaozi.zwb@alibaba-inc.com>

lixueclaire · 2023-06-28T07:22:28Z

cpp/src/arrow_chunk_writer.cc

+    if (index == -1) {
+      return Status::Invalid("The source index column ",
+                             GeneralParams::kSrcIndexCol,
+                             " is not exist in the input table");


lixueclaire · 2023-06-28T07:22:35Z

cpp/src/arrow_chunk_writer.cc

+    if (index == -1) {
+      return Status::Invalid("The destination index column ",
+                             GeneralParams::kDstIndexCol,
+                             " is not exist in the input table");


lixueclaire · 2023-06-28T07:23:30Z

cpp/src/arrow_chunk_writer.cc

+                            " vertex info.");
+  }
+  if (chunk_index < 0) {
+    return Status::Invalid("The chunk index is negative.");


I think IndexError is more suitable here.

And the unit test need to be updated too.

acezen requested a review from lixueclaire June 26, 2023 01:51

lixueclaire reviewed Jun 26, 2023

View reviewed changes

acezen added 8 commits June 28, 2023 14:27

Refine the error message of errors

de0f3e9

Signed-off-by: acezen <qiaozi.zwb@alibaba-inc.com>

Update

393414d

Signed-off-by: acezen <qiaozi.zwb@alibaba-inc.com>

Fix format

5947437

Fix

6ccf91e

Fix

535c9be

Signed-off-by: acezen <qiaozi.zwb@alibaba-inc.com>

Refine the status type

6d5d161

Update

10779c8

Fix

29cef4f

acezen force-pushed the 140-improve-error-message branch from 6d9e435 to 29cef4f Compare June 28, 2023 06:27

lixueclaire approved these changes Jun 28, 2023

View reviewed changes

acezen added 3 commits June 28, 2023 16:12

Fix

7805c95

Add brief for status code

945508a

Update

b057b76

acezen merged commit e92575e into apache:main Jun 28, 2023

acezen deleted the 140-improve-error-message branch June 28, 2023 08:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Improvement][C++] Refine the error message of errors of C++ SDK #192

[Improvement][C++] Refine the error message of errors of C++ SDK #192

acezen commented Jun 25, 2023 •

edited

Loading

github-actions bot commented Jun 25, 2023 •

edited

Loading

acezen commented Jun 26, 2023

lixueclaire commented Jun 26, 2023

lixueclaire Jun 26, 2023

lixueclaire Jun 28, 2023

lixueclaire Jun 26, 2023

lixueclaire Jun 26, 2023

acezen Jun 28, 2023

lixueclaire Jun 28, 2023

lixueclaire Jun 26, 2023

acezen Jun 28, 2023

lixueclaire Jun 26, 2023

lixueclaire Jun 26, 2023

lixueclaire Jun 26, 2023

lixueclaire Jun 26, 2023

lixueclaire Jun 26, 2023

lixueclaire Jun 26, 2023

lixueclaire Jun 28, 2023

lixueclaire Jun 28, 2023

lixueclaire Jun 28, 2023

lixueclaire Jun 28, 2023

[Improvement][C++] Refine the error message of errors of C++ SDK #192

[Improvement][C++] Refine the error message of errors of C++ SDK #192

Conversation

acezen commented Jun 25, 2023 • edited Loading

Proposed changes

Types of changes

Checklist

Further comments

github-actions bot commented Jun 25, 2023 • edited Loading

acezen commented Jun 26, 2023

lixueclaire commented Jun 26, 2023

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

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 commented Jun 25, 2023 •

edited

Loading

github-actions bot commented Jun 25, 2023 •

edited

Loading