ARROW-4036: [C++] Pluggable Status message, by exposing an abstract d…

…elegate class. This provides less "pluggability" but I think still offers a clean model for extension (subsystems can wrap the constructor for there purposes, and provide external static methods to check for particular types of errors). Author: Micah Kornfield <[email protected]> Author: Antoine Pitrou <[email protected]> Closes #4484 from emkornfield/status_code_proposal and squashes the following commits: 4d1ab8d <Micah Kornfield> don't import plasma errors directly into top level pyarrow module a66f999 <Micah Kornfield> make format 040216d <Micah Kornfield> fixes for comments outside python 729bba1 <Antoine Pitrou> Fix Py2 issues (hopefully) ea56d1e <Antoine Pitrou> Fix PythonErrorDetail to store Python error state (and restore it in check_status()) 21e1b95 <Micah Kornfield> fix compilation 9c905b0 <Micah Kornfield> fix lint 74d563c <Micah Kornfield> fixes 85786ef <Micah Kornfield> change messages 3626a90 <Micah Kornfield> try removing message a4e6a1f <Micah Kornfield> add logging for debug 4586fd1 <Micah Kornfield> fix typo 8f011b3 <Micah Kornfield> fix status propagation 317ea9c <Micah Kornfield> fix complie 9f59160 <Micah Kornfield> don't make_shared inline 484b3a2 <Micah Kornfield> style fix 14e3467 <Micah Kornfield> dont rely on rtti cd22df6 <Micah Kornfield> format dec4585 <Micah Kornfield> not-quite pluggable error codes
apache · Jul 3, 2019 · a9a82ec · a9a82ec
1 parent 86f0a3a
commit a9a82ec
Show file tree

Hide file tree

Showing 28 changed files with 586 additions and 267 deletions.
diff --git a/c_glib/arrow-glib/error.cpp b/c_glib/arrow-glib/error.cpp
@@ -65,22 +65,15 @@ garrow_error_code(const arrow::Status &status)
     return GARROW_ERROR_NOT_IMPLEMENTED;
   case arrow::StatusCode::SerializationError:
     return GARROW_ERROR_SERIALIZATION;
-  case arrow::StatusCode::PythonError:
-    return GARROW_ERROR_PYTHON;
-  case arrow::StatusCode::PlasmaObjectExists:
-    return GARROW_ERROR_PLASMA_OBJECT_EXISTS;
-  case arrow::StatusCode::PlasmaObjectNonexistent:
-    return GARROW_ERROR_PLASMA_OBJECT_NONEXISTENT;
-  case arrow::StatusCode::PlasmaStoreFull:
-    return GARROW_ERROR_PLASMA_STORE_FULL;
-  case arrow::StatusCode::PlasmaObjectAlreadySealed:
-    return GARROW_ERROR_PLASMA_OBJECT_ALREADY_SEALED;
   case arrow::StatusCode::CodeGenError:
     return GARROW_ERROR_CODE_GENERATION;
   case arrow::StatusCode::ExpressionValidationError:
     return GARROW_ERROR_EXPRESSION_VALIDATION;
   case arrow::StatusCode::ExecutionError:
     return GARROW_ERROR_EXECUTION;
+  case arrow::StatusCode::AlreadyExists:
+    return GARROW_ERROR_ALREADY_EXISTS;
+
   default:
     return GARROW_ERROR_UNKNOWN;
   }

diff --git a/c_glib/arrow-glib/error.h b/c_glib/arrow-glib/error.h
@@ -35,15 +35,11 @@ G_BEGIN_DECLS
  * @GARROW_ERROR_UNKNOWN: Unknown error.
  * @GARROW_ERROR_NOT_IMPLEMENTED: The feature is not implemented.
  * @GARROW_ERROR_SERIALIZATION: Serialization error.
- * @GARROW_ERROR_PYTHON: Python error.
- * @GARROW_ERROR_PLASMA_OBJECT_EXISTS: Object already exists on Plasma.
- * @GARROW_ERROR_PLASMA_OBJECT_NONEXISTENT: Object doesn't exist on Plasma.
- * @GARROW_ERROR_PLASMA_STORE_FULL: Store full error on Plasma.
- * @GARROW_ERROR_PLASMA_OBJECT_ALREADY_SEALED: Object already sealed on Plasma.
  * @GARROW_ERROR_CODE_GENERATION: Error generating code for expression evaluation
  *   in Gandiva.
  * @GARROW_ERROR_EXPRESSION_VALIDATION: Validation errors in expression given for code generation.
  * @GARROW_ERROR_EXECUTION: Execution error while evaluating the expression against a record batch.
+ * @GARROW_ALREADY_EXISTS: Item already exists error.
  *
  * The error codes are used by all arrow-glib functions.
  *
@@ -60,14 +56,10 @@ typedef enum {
   GARROW_ERROR_UNKNOWN = 9,
   GARROW_ERROR_NOT_IMPLEMENTED,
   GARROW_ERROR_SERIALIZATION,
-  GARROW_ERROR_PYTHON,
-  GARROW_ERROR_PLASMA_OBJECT_EXISTS = 20,
-  GARROW_ERROR_PLASMA_OBJECT_NONEXISTENT,
-  GARROW_ERROR_PLASMA_STORE_FULL,
-  GARROW_ERROR_PLASMA_OBJECT_ALREADY_SEALED,
   GARROW_ERROR_CODE_GENERATION = 40,
   GARROW_ERROR_EXPRESSION_VALIDATION = 41,
   GARROW_ERROR_EXECUTION = 42,
+  GARROW_ERROR_ALREADY_EXISTS = 45,
 } GArrowError;
 
 #define GARROW_ERROR garrow_error_quark()

diff --git a/c_glib/test/plasma/test-plasma-created-object.rb b/c_glib/test/plasma/test-plasma-created-object.rb
@@ -45,7 +45,7 @@ def teardown
 
   test("#abort") do
     @object.data.set_data(0, @data)
-    assert_raise(Arrow::Error::PlasmaObjectExists) do
+    assert_raise(Arrow::Error::AlreadyExists) do
       @client.create(@id, @data.bytesize, @options)
     end
     @object.abort

diff --git a/cpp/src/arrow/compute/kernels/cast.cc b/cpp/src/arrow/compute/kernels/cast.cc
@@ -52,7 +52,7 @@
     if (ARROW_PREDICT_FALSE(!_s.ok())) {                                            \
       std::stringstream ss;                                                         \
       ss << __FILE__ << ":" << __LINE__ << " code: " << #s << "\n" << _s.message(); \
-      ctx->SetStatus(Status(_s.code(), ss.str()));                                  \
+      ctx->SetStatus(Status(_s.code(), ss.str(), s.detail()));                      \
       return;                                                                       \
     }                                                                               \
   } while (0)

diff --git a/cpp/src/arrow/csv/column-builder.cc b/cpp/src/arrow/csv/column-builder.cc
@@ -76,7 +76,7 @@ class TypedColumnBuilder : public ColumnBuilder {
     } else {
       std::stringstream ss;
       ss << "In column #" << col_index_ << ": " << st.message();
-      return Status(st.code(), ss.str());
+      return Status(st.code(), ss.str(), st.detail());
     }
   }
 

diff --git a/cpp/src/arrow/python/common.cc b/cpp/src/arrow/python/common.cc
@@ -23,11 +23,15 @@
 
 #include "arrow/memory_pool.h"
 #include "arrow/status.h"
+#include "arrow/util/checked_cast.h"
 #include "arrow/util/logging.h"
 
 #include "arrow/python/helpers.h"
 
 namespace arrow {
+
+using internal::checked_cast;
+
 namespace py {
 
 static std::mutex memory_pool_mutex;
@@ -47,6 +51,129 @@ MemoryPool* get_memory_pool() {
   }
 }
 
+// ----------------------------------------------------------------------
+// PythonErrorDetail
+
+namespace {
+
+const char kErrorDetailTypeId[] = "arrow::py::PythonErrorDetail";
+
+// Try to match the Python exception type with an appropriate Status code
+StatusCode MapPyError(PyObject* exc_type) {
+  StatusCode code;
+
+  if (PyErr_GivenExceptionMatches(exc_type, PyExc_MemoryError)) {
+    code = StatusCode::OutOfMemory;
+  } else if (PyErr_GivenExceptionMatches(exc_type, PyExc_IndexError)) {
+    code = StatusCode::IndexError;
+  } else if (PyErr_GivenExceptionMatches(exc_type, PyExc_KeyError)) {
+    code = StatusCode::KeyError;
+  } else if (PyErr_GivenExceptionMatches(exc_type, PyExc_TypeError)) {
+    code = StatusCode::TypeError;
+  } else if (PyErr_GivenExceptionMatches(exc_type, PyExc_ValueError) ||
+             PyErr_GivenExceptionMatches(exc_type, PyExc_OverflowError)) {
+    code = StatusCode::Invalid;
+  } else if (PyErr_GivenExceptionMatches(exc_type, PyExc_EnvironmentError)) {
+    code = StatusCode::IOError;
+  } else if (PyErr_GivenExceptionMatches(exc_type, PyExc_NotImplementedError)) {
+    code = StatusCode::NotImplemented;
+  } else {
+    code = StatusCode::UnknownError;
+  }
+  return code;
+}
+
+// PythonErrorDetail indicates a Python exception was raised.
+class PythonErrorDetail : public StatusDetail {
+ public:
+  const char* type_id() const override { return kErrorDetailTypeId; }
+
+  std::string ToString() const override {
+    // This is simple enough not to need the GIL
+    const auto ty = reinterpret_cast<const PyTypeObject*>(exc_type_.obj());
+    // XXX Should we also print traceback?
+    return std::string("Python exception: ") + ty->tp_name;
+  }
+
+  void RestorePyError() const {
+    Py_INCREF(exc_type_.obj());
+    Py_INCREF(exc_value_.obj());
+    Py_INCREF(exc_traceback_.obj());
+    PyErr_Restore(exc_type_.obj(), exc_value_.obj(), exc_traceback_.obj());
+  }
+
+  PyObject* exc_type() const { return exc_type_.obj(); }
+
+  PyObject* exc_value() const { return exc_value_.obj(); }
+
+  static std::shared_ptr<PythonErrorDetail> FromPyError() {
+    PyObject* exc_type = nullptr;
+    PyObject* exc_value = nullptr;
+    PyObject* exc_traceback = nullptr;
+
+    PyErr_Fetch(&exc_type, &exc_value, &exc_traceback);
+    PyErr_NormalizeException(&exc_type, &exc_value, &exc_traceback);
+    ARROW_CHECK(exc_type)
+        << "PythonErrorDetail::FromPyError called without a Python error set";
+    DCHECK(PyType_Check(exc_type));
+    DCHECK(exc_value);  // Ensured by PyErr_NormalizeException, double-check
+    if (exc_traceback == nullptr) {
+      // Needed by PyErr_Restore()
+      Py_INCREF(Py_None);
+      exc_traceback = Py_None;
+    }
+
+    std::shared_ptr<PythonErrorDetail> detail(new PythonErrorDetail);
+    detail->exc_type_.reset(exc_type);
+    detail->exc_value_.reset(exc_value);
+    detail->exc_traceback_.reset(exc_traceback);
+    return detail;
+  }
+
+ protected:
+  PythonErrorDetail() = default;
+
+  OwnedRefNoGIL exc_type_, exc_value_, exc_traceback_;
+};
+
+}  // namespace
+
+// ----------------------------------------------------------------------
+// Python exception <-> Status
+
+Status ConvertPyError(StatusCode code) {
+  auto detail = PythonErrorDetail::FromPyError();
+  if (code == StatusCode::UnknownError) {
+    code = MapPyError(detail->exc_type());
+  }
+
+  std::string message;
+  RETURN_NOT_OK(internal::PyObject_StdStringStr(detail->exc_value(), &message));
+  return Status(code, message, detail);
+}
+
+Status PassPyError() {
+  if (PyErr_Occurred()) {
+    return ConvertPyError();
+  }
+  return Status::OK();
+}
+
+bool IsPyError(const Status& status) {
+  if (status.ok()) {
+    return false;
+  }
+  auto detail = status.detail();
+  bool result = detail != nullptr && detail->type_id() == kErrorDetailTypeId;
+  return result;
+}
+
+void RestorePyError(const Status& status) {
+  ARROW_CHECK(IsPyError(status));
+  const auto& detail = checked_cast<const PythonErrorDetail&>(*status.detail());
+  detail.RestorePyError();
+}
+
 // ----------------------------------------------------------------------
 // PyBuffer
 
@@ -64,7 +191,7 @@ Status PyBuffer::Init(PyObject* obj) {
     }
     return Status::OK();
   } else {
-    return Status(StatusCode::PythonError, "");
+    return ConvertPyError(StatusCode::Invalid);
   }
 }
 
@@ -83,56 +210,5 @@ PyBuffer::~PyBuffer() {
   }
 }
 
-// ----------------------------------------------------------------------
-// Python exception -> Status
-
-Status ConvertPyError(StatusCode code) {
-  PyObject* exc_type = nullptr;
-  PyObject* exc_value = nullptr;
-  PyObject* traceback = nullptr;
-
-  PyErr_Fetch(&exc_type, &exc_value, &traceback);
-  PyErr_NormalizeException(&exc_type, &exc_value, &traceback);
-
-  DCHECK_NE(exc_type, nullptr) << "ConvertPyError called without an exception set";
-
-  OwnedRef exc_type_ref(exc_type);
-  OwnedRef exc_value_ref(exc_value);
-  OwnedRef traceback_ref(traceback);
-
-  std::string message;
-  RETURN_NOT_OK(internal::PyObject_StdStringStr(exc_value, &message));
-
-  if (code == StatusCode::UnknownError) {
-    // Try to match the Python exception type with an appropriate Status code
-    if (PyErr_GivenExceptionMatches(exc_type, PyExc_MemoryError)) {
-      code = StatusCode::OutOfMemory;
-    } else if (PyErr_GivenExceptionMatches(exc_type, PyExc_IndexError)) {
-      code = StatusCode::IndexError;
-    } else if (PyErr_GivenExceptionMatches(exc_type, PyExc_KeyError)) {
-      code = StatusCode::KeyError;
-    } else if (PyErr_GivenExceptionMatches(exc_type, PyExc_TypeError)) {
-      code = StatusCode::TypeError;
-    } else if (PyErr_GivenExceptionMatches(exc_type, PyExc_ValueError) ||
-               PyErr_GivenExceptionMatches(exc_type, PyExc_OverflowError)) {
-      code = StatusCode::Invalid;
-    } else if (PyErr_GivenExceptionMatches(exc_type, PyExc_EnvironmentError)) {
-      code = StatusCode::IOError;
-    } else if (PyErr_GivenExceptionMatches(exc_type, PyExc_NotImplementedError)) {
-      code = StatusCode::NotImplemented;
-    }
-  }
-  return Status(code, message);
-}
-
-Status PassPyError() {
-  if (PyErr_Occurred()) {
-    // Do not call PyErr_Clear, the assumption is that someone further
-    // up the call stack will want to deal with the Python error.
-    return Status(StatusCode::PythonError, "");
-  }
-  return Status::OK();
-}
-
 }  // namespace py
 }  // namespace arrow
diff --git a/cpp/src/arrow/python/common.h b/cpp/src/arrow/python/common.h
@@ -36,7 +36,15 @@ class Result;
 
 namespace py {
 
+// Convert current Python error to a Status.  The Python error state is cleared
+// and can be restored with RestorePyError().
 ARROW_PYTHON_EXPORT Status ConvertPyError(StatusCode code = StatusCode::UnknownError);
+// Same as ConvertPyError(), but returns Status::OK() if no Python error is set.
+ARROW_PYTHON_EXPORT Status PassPyError();
+// Query whether the given Status is a Python error (as wrapped by ConvertPyError()).
+ARROW_PYTHON_EXPORT bool IsPyError(const Status& status);
+// Restore a Python error wrapped in a Status.
+ARROW_PYTHON_EXPORT void RestorePyError(const Status& status);
 
 // Catch a pending Python exception and return the corresponding Status.
 // If no exception is pending, Status::OK() is returned.
@@ -48,9 +56,6 @@ inline Status CheckPyError(StatusCode code = StatusCode::UnknownError) {
   }
 }
 
-ARROW_PYTHON_EXPORT Status PassPyError();
-
-// TODO(wesm): We can just let errors pass through. To be explored later
 #define RETURN_IF_PYERROR() ARROW_RETURN_NOT_OK(CheckPyError());
 
 #define PY_RETURN_IF_ERROR(CODE) ARROW_RETURN_NOT_OK(CheckPyError(CODE));
@@ -97,6 +102,18 @@ class ARROW_PYTHON_EXPORT PyAcquireGIL {
   ARROW_DISALLOW_COPY_AND_ASSIGN(PyAcquireGIL);
 };
 
+// A RAII-style helper that releases the GIL until the end of a lexical block
+class ARROW_PYTHON_EXPORT PyReleaseGIL {
+ public:
+  PyReleaseGIL() { saved_state_ = PyEval_SaveThread(); }
+
+  ~PyReleaseGIL() { PyEval_RestoreThread(saved_state_); }
+
+ private:
+  PyThreadState* saved_state_;
+  ARROW_DISALLOW_COPY_AND_ASSIGN(PyReleaseGIL);
+};
+
 // A helper to call safely into the Python interpreter from arbitrary C++ code.
 // The GIL is acquired, and the current thread's error status is preserved.
 template <typename Function>
@@ -109,7 +126,7 @@ Status SafeCallIntoPython(Function&& func) {
   Status st = std::forward<Function>(func)();
   // If the return Status is a "Python error", the current Python error status
   // describes the error and shouldn't be clobbered.
-  if (!st.IsPythonError() && exc_type != NULLPTR) {
+  if (!IsPyError(st) && exc_type != NULLPTR) {
     PyErr_Restore(exc_type, exc_value, exc_traceback);
   }
   return st;