You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I'm pretty sure there are pros and cons for each side and people might get into religious fights on this topic. Let's try to come to a conclusion which one we should use in this project.
The main thing I like about the "Status" style is that it makes error propagation and handling (or lack thereof) explicit. However, I have no religious preference :)
Personally I think if iceberg-cpp is just for parsing the metadata, all is ok for me. If it's also able to handle the dataset layer, I think exception might making maintaining the internal status of async fetching a bit more hard ( though they're already hard )
I'm not sure if this is a good reason to use C++23. Choosing C++20 was arbitrary decision from me.
I'm not sure if there will be any difficulty for other projects to include iceberg-cpp if we choose a fairly
new standard, other than that, I'm no objection for C++23.
While looking the std::expected usage, I'm thinking no matter we choose the std::expected way or Arrow's Result way, it is no harm that we add a Status data structure, it's useful in the following two cases:
directly return Status, example: Status Close();, the returned value can state whether the call success or failed for some reason.
return std::expected<value_type, Status> or implicitly wrapped by Result<value_type>, example: std::expected<int64_t, Status> Tell(), either return the current access position of a opened file or the failure reason.
(FWIW, I believe the...expected...way of using std::expected when there is no return value is std::expected<void, Error>. You could always using Status = expected<void, Error> + using Result = expected<T, Error>.)
I'm hesitant to introduce a dependency for C++23. Some projects might not move that fast and we'd make the adoption hard for them.
I'd still go for using exceptions rather. It would make the interfaces and the code itself way cleaner, that matters for API's and libraries like this that are used by other tools. I haven't heard anyone from the Java implementation side complaining about the difficulty of debugging code with exceptions.
Edit: I'm not saying that std::expected should be preferred over exceptions. Just that the language compatibility problem can be solved if std::expected is preferred.
Ah, by the way, a nice consequence of using exceptions is that you can throw them from constructors, while with std::expected you would have to define static factory functions instead.
[Exceptions] are not easily composable: There is only one current exception, you can’t have multiple ones. This was a problem, for example, for the C++17 parallel algorithms. What if an exception is thrown in multiple of the worker threads? How to report all of them back to the caller? The implementation gave up on solving that problem and just decided to terminate the program if any exception is thrown.
If this is still the case with C++20 (you can't store an exception on the side to later wrap or re-throw it, for example), then this sounds like a major annoyance.
There is std::current_exception/std::rethrow_exception, though it has some intricacies. (And I don't think there's anything like ExceptionGroup in Python.)
Because any consumer of libiceberg would have to be careful about API/ABI conflicts with other Abseil-using libraries.
Thank you for the response! Though, I think API-wise absl is fairly stable. Regarding ABI compatibility, I usually think in terms of static linking which - in my opinion - (pretty much) neglects this issue. On the other hand, if you want to link dynamically, yeah, I would not do that either.
As a general data point for this discussion, I would mention the case of the Arrow C++ CSV parser. Arrow C++ uses a Status class (but std::expected would be similar) for error reporting. This implies that any potentially-failing function should return a Status (or, similarly, Result<T> or std::expected<T, E>).
However, a CSV parser is a state machine that must withhold tens or hundreds of millions of transitions per second. It turns out that reporting a Status in such a critical path can reduce performance by 20-30%. The Arrow C++ CSV parser tries various tricks to avoid such reporting, which can have other consequences on security (see current discussion in apache/arrow#45498).
Edit: let me qualify/moderate this a bit. The performance overhead is not only reporting the Status, it's also simply detecting the error and branching on it in the leaf functions.
Activity
[-]Exceptions vs status codes[/-][+][DISCUSS] Exceptions vs status codes[/+]pitrou commentedon Dec 16, 2024
The main thing I like about the "Status" style is that it makes error propagation and handling (or lack thereof) explicit. However, I have no religious preference :)
wgtmac commentedon Dec 16, 2024
As an Arrow developer who has experienced both status and exception in the same repo, I feel inclined to use exception to make the code shorter and easier to debug. It is preferred (perhaps biased) to use exception for modern C++: https://learn.microsoft.com/en-us/cpp/cpp/errors-and-exception-handling-modern-cpp?view=msvc-170
zhjwpku commentedon Dec 17, 2024
I came across this
expected[0] class which seems conform to the idea of Status/Result, but it's in c++23.[0] https://en.cppreference.com/w/cpp/utility/expected
zhjwpku commentedon Dec 17, 2024
Facebook's Folly has a Expected class [0], Google's Abseil has a StatusOr class [1] all for the same purpose.
[0] https://github.com/facebook/folly/blob/main/folly/Expected.h
[1] https://github.com/abseil/abseil-cpp/blob/master/absl/status/statusor.h
mapleFU commentedon Dec 17, 2024
Personally I think if iceberg-cpp is just for parsing the metadata, all is ok for me. If it's also able to handle the dataset layer, I think exception might making maintaining the internal status of async fetching a bit more hard ( though they're already hard )
But I also have no religious preference :)
wgtmac commentedon Jan 10, 2025
I'm not sure if this is a good reason to use C++23. Choosing C++20 was arbitrary decision from me.
I just checked that
expectedimplementation is complete from major compilers: https://en.cppreference.com/w/cpp/compiler_support/23zhjwpku commentedon Jan 10, 2025
I'm not sure if there will be any difficulty for other projects to include iceberg-cpp if we choose a fairly
new standard, other than that, I'm no objection for C++23.
good to know.
zhjwpku commentedon Jan 13, 2025
While looking the std::expected usage, I'm thinking no matter we choose the std::expected way or Arrow's Result way, it is no harm that we add a Status data structure, it's useful in the following two cases:
Status Close();, the returned value can state whether the call success or failed for some reason.std::expected<int64_t, Status> Tell(), either return the current access position of a opened file or the failure reason.Thought? @wgtmac @pitrou @gaborkaszab @mapleFU
lidavidm commentedon Jan 13, 2025
(FWIW, I believe the...expected...way of using std::expected when there is no return value is
std::expected<void, Error>. You could alwaysusing Status = expected<void, Error>+using Result = expected<T, Error>.)gaborkaszab commentedon Jan 13, 2025
I'm hesitant to introduce a dependency for C++23. Some projects might not move that fast and we'd make the adoption hard for them.
I'd still go for using exceptions rather. It would make the interfaces and the code itself way cleaner, that matters for API's and libraries like this that are used by other tools. I haven't heard anyone from the Java implementation side complaining about the difficulty of debugging code with exceptions.
pitrou commentedon Jan 13, 2025
Well, you can find a
std::expectedbackport for C++20, for example https://github.com/zeus-cpp/expectedEdit: I'm not saying that
std::expectedshould be preferred over exceptions. Just that the language compatibility problem can be solved ifstd::expectedis preferred.pitrou commentedon Jan 13, 2025
Ah, by the way, a nice consequence of using exceptions is that you can throw them from constructors, while with
std::expectedyou would have to definestaticfactory functions instead.pitrou commentedon Jan 13, 2025
I went looking around for comparisons of exceptions and
std::expectedand I found an insightful tidbit here: https://www.foonathan.net/2017/12/exceptions-vs-expected/If this is still the case with C++20 (you can't store an exception on the side to later wrap or re-throw it, for example), then this sounds like a major annoyance.
lidavidm commentedon Jan 13, 2025
There is std::current_exception/std::rethrow_exception, though it has some intricacies. (And I don't think there's anything like ExceptionGroup in Python.)
5 remaining items
pitrou commentedon Jan 23, 2025
Because any consumer of libiceberg would have to be careful about API/ABI conflicts with other Abseil-using libraries.
The same applies to Boost in some ways (though Boost, at least, is not often a public dependency).
ormandi commentedon Jan 23, 2025
Thank you for the response! Though, I think API-wise
abslis fairly stable. Regarding ABI compatibility, I usually think in terms of static linking which - in my opinion - (pretty much) neglects this issue. On the other hand, if you want to link dynamically, yeah, I would not do that either.backport c++23 std::expected
backport c++23 std::expected
backport c++23 std::expected
backport c++23 std::expected
backport c++23 std::expected
backport c++23 std::expected
zhjwpku commentedon Jan 27, 2025
I've created a PR #40 to backport std::expected, I'm looking forward to hearing all of your feedback.
backport c++23 std::expected
backport c++23 std::expected
pitrou commentedon Feb 12, 2025
As a general data point for this discussion, I would mention the case of the Arrow C++ CSV parser. Arrow C++ uses a
Statusclass (butstd::expectedwould be similar) for error reporting. This implies that any potentially-failing function should return aStatus(or, similarly,Result<T>orstd::expected<T, E>).However, a CSV parser is a state machine that must withhold tens or hundreds of millions of transitions per second. It turns out that reporting a
Statusin such a critical path can reduce performance by 20-30%. The Arrow C++ CSV parser tries various tricks to avoid such reporting, which can have other consequences on security (see current discussion in apache/arrow#45498).Edit: let me qualify/moderate this a bit. The performance overhead is not only reporting the
Status, it's also simply detecting the error and branching on it in the leaf functions.backport c++23 std::expected (#40)
zhjwpku commentedon Apr 2, 2025
I think we can close this issue for now, we have exception and expected in our code base, lets see how it works.