ARROW-4036: [C++] Pluggable Status message, by exposing an abstract delegate class. #4484

emkornfield · 2019-06-06T06:20:57Z

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).

emkornfield · 2019-06-06T06:23:55Z

This also assumes subsystem codes don't map to top level codes (I think I would prefer they do, but I could see them going both ways).

pitrou · 2019-06-06T07:18:55Z

cpp/src/arrow/status.h

@@ -92,6 +92,7 @@ enum class StatusCode : char {
  SerializationError = 11,
  PythonError = 12,
  RError = 13,
+  SubsystemError = 14,


I don't think that's the right approach. If I have a Python ValueError, I want it both to map to StatusCode::Invalid and to retain the original Python exception.

Yep, I tend to agree. So two issues to improve on:

top-level codes should be orthogonal from subsytem codes

Potentially need the ability to directly link to underlying exception (e.g. PyObject for a python error)

Any other high level concepts/functionallty missing?

Right. I also think subsystem codes can be optional (for example, Python would just use the underlying exception instead of defining integer codes).

So instead of having a subsystem code, we could have an opaque optional detail instance, e.g.:

class StatusDetail { public: virtual ~StatusDetail(); virtual std::string ToString() const = 0; };

Yeah, I came to the same conclusion after stepping away from the computer. Will try to update tonight.

I've updated to just have a status detail, that wraps has a message accessor and unify the current implementation with a private implementation of the interface. I can see arguments for keeping a separate message member as well, so I'm happy to add it back.

I would be in favour of having a separate message member, so that the detail can remain null if there's no error-specific detail to carry.

Added msg field. back.

lidavidm · 2019-06-07T02:03:56Z

cpp/src/arrow/status.h

+
+};
+
+enum class Subsystem : char {


Does Flight also warrant a subsystem? It would be nice to express errors like unauthorized, service unavailable/overloaded, etc. (tagging my work account @lihalite)

Going to remove this abd go with the approach suggested by Antoine

@lihalite when this lands, I think some codes should probably be top level (unavailable seems generic across multiple service type things) but some might be flight specific.

Ah ok, great. I started sketching out a set of codes for Flight/Java, I can pick up the C++/Python side too once this lands.

codecov-io · 2019-06-08T17:54:42Z

Codecov Report

Merging #4484 into master will increase coverage by 21.19%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           master    #4484       +/-   ##
===========================================
+ Coverage   67.96%   89.15%   +21.19%     
===========================================
  Files         676      588       -88     
  Lines       74307    70058     -4249     
  Branches     1253        0     -1253     
===========================================
+ Hits        50501    62460    +11959     
+ Misses      23559     7598    -15961     
+ Partials      247        0      -247

Impacted Files	Coverage Δ
cpp/src/arrow/status-test.cc	`100% <100%> (ø)`
cpp/src/arrow/status.cc	`37.75% <100%> (+24.71%)`	⬆️
cpp/src/arrow/status.h	`95.69% <100%> (+22.28%)`	⬆️
cpp/src/arrow/array/builder_union.cc	`0% <0%> (-100%)`	⬇️
cpp/src/arrow/csv/reader.h	`0% <0%> (-100%)`	⬇️
cpp/src/arrow/csv/reader.cc	`0% <0%> (-92.9%)`	⬇️
cpp/src/arrow/adapters/orc/adapter_util.cc	`15.74% <0%> (-64.97%)`	⬇️
cpp/src/arrow/array/builder_union.h	`0% <0%> (-61.91%)`	⬇️
cpp/src/arrow/util/cpu-info.h	`0% <0%> (-50%)`	⬇️
cpp/src/arrow/csv/options.h	`66.66% <0%> (-33.34%)`	⬇️
... and 677 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2ef7d9...e20eb9b. Read the comment docs.

emkornfield · 2019-06-09T06:17:29Z

If you want me to try to cleanup one of the subsystem (python?) in this PR let me know and I can give it a shot.

pitrou

I think this looks nice on the principle. It would be useful to validate this by migrating the Gandiva, Plasma... status codes.

cpp/src/arrow/status-test.cc

cpp/src/arrow/status.h

emkornfield · 2019-06-12T06:35:53Z

I think this looks nice on the principle. It would be useful to validate this by migrating the Gandiva, Plasma... status codes.

Will give this a try I'm not sure about downstream dependencies though.

emkornfield · 2019-06-22T08:12:00Z

@pitrou this demonstrates using the the StatusDetail with Python and Plasma. This breaks some C++ and Python APIs, I'm not sure if we are OK with that.

If this approach is OK, I will do a follow-up PR to remove Gandiva codes as well.

CC @pcmoritz

emkornfield · 2019-06-22T08:15:28Z

putting back in WIP status until I can get CI green (python tests passed for me locally)

emkornfield · 2019-07-02T07:33:09Z

@pitrou after a lot of debugging I figured out why the builds were failing only to discover you had fixed with with your PR to provide better errors (there was a copy of Status in a macro that I did not find initally). Thank you! I think this is ready to review. I'll file a follow-up JIRA once this is merged to migrade Gandiva away from this as well.

pitrou · 2019-07-02T12:08:11Z

Will review, thank you :-)

emkornfield · 2019-07-02T12:10:15Z

I'm looking into the lint failure, but I'm guessing i wll need to make some changes anyways.

(and restore it in check_status())

pitrou · 2019-07-02T16:07:33Z

I've done my thing on Python. Let's wait for CI a bit.

pitrou · 2019-07-02T17:15:45Z

@ursabot build test

ursabot · 2019-07-02T17:15:46Z

Got unexpected extra argument (test)

pitrou · 2019-07-02T17:15:54Z

@ursabot build

kszucs · 2019-07-02T17:40:08Z

@ursabot build

pitrou · 2019-07-02T17:49:33Z

The failed ursabot builds are unrelated, @kszucs is busy doing things on BuildBot.
The failed AppVeyor build is a transient download failure

Travis-CI build: https://travis-ci.org/pitrou/arrow/builds/553376058
AppVeyor build: https://ci.appveyor.com/project/pitrou/arrow/builds/25698396

pitrou

A couple more things to change. I can tackle those tomorrow if you like.

cpp/src/plasma/common.h

cpp/src/plasma/common.cc

python/pyarrow/__init__.py

emkornfield · 2019-07-03T03:53:48Z

cpp/src/arrow/python/common.cc

+    return std::string("Python exception: ") + ty->tp_name;
+  }
+
+  void RestorePyError() const {


thanks for doing this, I started down this path and decided to keep it simpler because there were semantics of the GIL I was hazy on.

pitrou · 2019-07-03T09:21:47Z

I'm going to merge once CI is green.

…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

revert some changes replace event loop with asio update plasma store protocol fix qualifiers update plasma store client protocol Remove all native socket operations. Implement general io support Fix bugs fix all compiling bugs fix bug Fix all tests. Add license header. try to fix cmake try to make asio standalone simplify code add license update url lint lint & fix fix restore entrypoint remove unused unix headers fix Update LICENSE fix rename handle signal move the function to its original place fix doc hide classes stop installing asio headers fix doc reverse changes minor fix tiny fix fix comments minor fix resolve conflicts fix optimize cmake fix update formatter fix according to github comments lint prevent the store from dying fix ARROW_CHECK Fix ARROW-4036: [C++] Pluggable Status message, by exposing an abstract delegate 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 apache#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 fix merge fix update update update update fix update fix update update revert some unknown comments rebase CMakeLists rebase eviction_policy.h rebase CMakeLists rebase

…pe (int/string) Pandas dataframe to pyarrow Table This PR homogenizes error messages for mixed-type `Pandas` inputs to `pa.Table`. The message for `Pandas` column with `int` followed by `string` is now ``` In [2]: table = pa.Table.from_pandas(pd.DataFrame({'a': [ 19, 'a']})) (... traceback...) ArrowInvalid: ('Could not convert a with type str: tried to convert to int', 'Conversion failed for column a with type object') ``` the same as for `double` followed by `string`: ``` In [3]: table = pa.Table.from_pandas(pd.DataFrame({'a': [ 19.0, 'a']})) (... traceback...) ArrowInvalid: ('Could not convert a with type str: tried to convert to double', 'Conversion failed for column a with type object') ``` As a side effect, this snippet [xref #5866, ARROW-7168] now throws an `ArrowInvalid` (has been `FutureWarning` since 0.16): ``` In [8]: cat = pd.Categorical.from_codes(np.array([0, 1], dtype='int8'), np.array(['a', 'b'], dtype=object)) ...: typ = pa.dictionary(index_type=pa.int8(), value_type=pa.int64()) ...: result = pa.array(cat, type=typ) (... traceback...) ArrowInvalid: Could not convert a with type str: tried to convert to int ``` Finally, this *does* break a test [xref #4484, ARROW-4036] - see code comment Closes #8044 from arw2019/ARROW-7663 Authored-by: arw2019 <[email protected]> Signed-off-by: Joris Van den Bossche <[email protected]>

…pe (int/string) Pandas dataframe to pyarrow Table This PR homogenizes error messages for mixed-type `Pandas` inputs to `pa.Table`. The message for `Pandas` column with `int` followed by `string` is now ``` In [2]: table = pa.Table.from_pandas(pd.DataFrame({'a': [ 19, 'a']})) (... traceback...) ArrowInvalid: ('Could not convert a with type str: tried to convert to int', 'Conversion failed for column a with type object') ``` the same as for `double` followed by `string`: ``` In [3]: table = pa.Table.from_pandas(pd.DataFrame({'a': [ 19.0, 'a']})) (... traceback...) ArrowInvalid: ('Could not convert a with type str: tried to convert to double', 'Conversion failed for column a with type object') ``` As a side effect, this snippet [xref apache#5866, ARROW-7168] now throws an `ArrowInvalid` (has been `FutureWarning` since 0.16): ``` In [8]: cat = pd.Categorical.from_codes(np.array([0, 1], dtype='int8'), np.array(['a', 'b'], dtype=object)) ...: typ = pa.dictionary(index_type=pa.int8(), value_type=pa.int64()) ...: result = pa.array(cat, type=typ) (... traceback...) ArrowInvalid: Could not convert a with type str: tried to convert to int ``` Finally, this *does* break a test [xref apache#4484, ARROW-4036] - see code comment Closes apache#8044 from arw2019/ARROW-7663 Authored-by: arw2019 <[email protected]> Signed-off-by: Joris Van den Bossche <[email protected]>

emkornfield mentioned this pull request Jun 6, 2019

[EXP] ARROW-4036: [C++] Pluggable status codes #3199

Closed

emkornfield changed the title ~~[Proposal/Sketch] ARROW-4036: [C++]not-quite pluggable error codes~~ [Proposal/Sketch] ARROW-4036: [C++] not-quite pluggable error codes Jun 6, 2019

pitrou reviewed Jun 6, 2019

View reviewed changes

lidavidm reviewed Jun 7, 2019

View reviewed changes

emkornfield changed the title ~~[Proposal/Sketch] ARROW-4036: [C++] not-quite pluggable error codes~~ ARROW-4036: [C++] Pluggable Status message, buy exposing an abstract delegate class. Jun 9, 2019

emkornfield changed the title ~~ARROW-4036: [C++] Pluggable Status message, buy exposing an abstract delegate class.~~ ARROW-4036: [C++] Pluggable Status message, by exposing an abstract delegate class. Jun 9, 2019

pitrou reviewed Jun 11, 2019

View reviewed changes

cpp/src/arrow/status-test.cc Outdated Show resolved Hide resolved

cpp/src/arrow/status.h Outdated Show resolved Hide resolved

cpp/src/arrow/status.h Show resolved Hide resolved

emkornfield force-pushed the status_code_proposal branch from e20eb9b to c78f74d Compare June 12, 2019 06:37

fsaintjacques added the Component: C++ label Jun 14, 2019

emkornfield changed the title ~~ARROW-4036: [C++] Pluggable Status message, by exposing an abstract delegate class.~~ ARROW-4036: [WIP] [C++] Pluggable Status message, by exposing an abstract delegate class. Jun 16, 2019

emkornfield force-pushed the status_code_proposal branch from ff5fa6e to 814d55f Compare June 17, 2019 06:32

This was referenced Jun 17, 2019

ARROW-4626: [Flight] Add application-defined metadata to DoGet/DoPut #4282

Closed

ARROW-5643: [FlightRPC] Add ability to override SSL hostname checking #4608

Closed

ARROW-5681: [FlightRPC][Java] Add FlightStatusCode/FlightRuntimeException #4652

Closed

emkornfield force-pushed the status_code_proposal branch from 814d55f to 93737ac Compare June 22, 2019 07:59

emkornfield changed the title ~~ARROW-4036: [WIP] [C++] Pluggable Status message, by exposing an abstract delegate class.~~ ARROW-4036: [C++][Python] Pluggable Status message, by exposing an abstract delegate class. Jun 22, 2019

emkornfield changed the title ~~ARROW-4036: [C++][Python] Pluggable Status message, by exposing an abstract delegate class.~~ ARROW-4036: [WIP][C++] Pluggable Status message, by exposing an abstract delegate class. Jun 22, 2019

emkornfield force-pushed the status_code_proposal branch 2 times, most recently from e2f9a14 to 12c5c61 Compare June 28, 2019 15:35

emkornfield force-pushed the status_code_proposal branch from 12c5c61 to 8433843 Compare July 2, 2019 07:24

emkornfield changed the title ~~ARROW-4036: [WIP][C++] Pluggable Status message, by exposing an abstract delegate class.~~ ARROW-4036: [C++] Pluggable Status message, by exposing an abstract delegate class. Jul 2, 2019

emkornfield and others added 4 commits July 2, 2019 17:58

fixes

74d563c

fix lint

9c905b0

fix compilation

21e1b95

Fix PythonErrorDetail to store Python error state

ea56d1e

(and restore it in check_status())

pitrou force-pushed the status_code_proposal branch from 68c9d62 to ea56d1e Compare July 2, 2019 16:01

Fix Py2 issues (hopefully)

729bba1

pitrou requested changes Jul 2, 2019

View reviewed changes

cpp/src/plasma/common.h Outdated Show resolved Hide resolved

cpp/src/plasma/common.cc Outdated Show resolved Hide resolved

python/pyarrow/__init__.py Outdated Show resolved Hide resolved

emkornfield commented Jul 3, 2019

View reviewed changes

emkornfield added 3 commits July 2, 2019 21:01

fixes for comments outside python

040216d

make format

a66f999

don't import plasma errors directly into top level pyarrow module

4d1ab8d

pitrou approved these changes Jul 3, 2019

View reviewed changes

pitrou closed this in a9a82ec Jul 3, 2019

arw2019 mentioned this pull request Aug 25, 2020

ARROW-7663: [Python] Raise better error message when passing mixed-type (int/string) Pandas dataframe to pyarrow Table #8044

Closed

asfimport mentioned this pull request Aug 1, 2019

[C++] Make status codes pluggable #20636

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-4036: [C++] Pluggable Status message, by exposing an abstract delegate class. #4484

ARROW-4036: [C++] Pluggable Status message, by exposing an abstract delegate class. #4484

emkornfield commented Jun 6, 2019

emkornfield commented Jun 6, 2019

pitrou Jun 6, 2019

emkornfield Jun 6, 2019

pitrou Jun 6, 2019

emkornfield Jun 7, 2019

emkornfield Jun 9, 2019

pitrou Jun 10, 2019

emkornfield Jun 11, 2019

lidavidm Jun 7, 2019

emkornfield Jun 7, 2019

emkornfield Jul 2, 2019

ghost Jul 2, 2019

codecov-io commented Jun 8, 2019 •

edited

Loading

emkornfield commented Jun 9, 2019

pitrou left a comment

emkornfield commented Jun 12, 2019 •

edited

Loading

emkornfield commented Jun 22, 2019

emkornfield commented Jun 22, 2019

emkornfield commented Jul 2, 2019

pitrou commented Jul 2, 2019

emkornfield commented Jul 2, 2019

pitrou commented Jul 2, 2019

pitrou commented Jul 2, 2019

ursabot commented Jul 2, 2019

pitrou commented Jul 2, 2019

kszucs commented Jul 2, 2019

pitrou commented Jul 2, 2019 •

edited

Loading

pitrou left a comment

emkornfield Jul 3, 2019

pitrou commented Jul 3, 2019

ARROW-4036: [C++] Pluggable Status message, by exposing an abstract delegate class. #4484

ARROW-4036: [C++] Pluggable Status message, by exposing an abstract delegate class. #4484

Conversation

emkornfield commented Jun 6, 2019

emkornfield commented Jun 6, 2019

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

codecov-io commented Jun 8, 2019 • edited Loading

Codecov Report

emkornfield commented Jun 9, 2019

pitrou left a comment

Choose a reason for hiding this comment

emkornfield commented Jun 12, 2019 • edited Loading

emkornfield commented Jun 22, 2019

emkornfield commented Jun 22, 2019

emkornfield commented Jul 2, 2019

pitrou commented Jul 2, 2019

emkornfield commented Jul 2, 2019

pitrou commented Jul 2, 2019

pitrou commented Jul 2, 2019

ursabot commented Jul 2, 2019

pitrou commented Jul 2, 2019

kszucs commented Jul 2, 2019

pitrou commented Jul 2, 2019 • edited Loading

pitrou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou commented Jul 3, 2019

codecov-io commented Jun 8, 2019 •

edited

Loading

emkornfield commented Jun 12, 2019 •

edited

Loading

pitrou commented Jul 2, 2019 •

edited

Loading