Skip to content

Commit

Permalink
ARROW-4084: [C++] Make Status static method support variadic arguments
Browse files Browse the repository at this point in the history
- Static constructors like `Status::Invalid` now supports variadic
arguments à la `Status::Invalid("my", variable, "error message: ", i)`.

- A new macro was added `ARROW_RETURN_IF(cond, status)` which replaces
the previous `ARROW_RETURN_IF_FALSE` but also adds branch prediction
hints. Note that only gandiva was refactored with this macro as
otherwise the code review would have exploded.

- Fixed a bug in memory map implementations not checking the return
code of `mmap` and `mremap`.

Author: François Saint-Jacques <fsaintjacques@gmail.com>

Closes #3228 from fsaintjacques/ARROW-4084-variadic-status-message and squashes the following commits:

a877ab9 <François Saint-Jacques> Travis
890df68 <François Saint-Jacques> Remove gandiva expect string message testing
71ecbae <François Saint-Jacques> Use perfect forwarding.
774bf93 <François Saint-Jacques> Add missing string header
bf5cdfe <François Saint-Jacques> Removed code printing in status
1d1db49 <François Saint-Jacques> Reformat
d9fcad9 <François Saint-Jacques> ARROW-4084: Make Status static method support variadic arguments
  • Loading branch information
fsaintjacques authored and wesm committed Dec 20, 2018
1 parent c39db63 commit ce9c6e3
Show file tree
Hide file tree
Showing 68 changed files with 763 additions and 1,122 deletions.
22 changes: 6 additions & 16 deletions cpp/src/arrow/adapters/orc/adapter.cc
Expand Up @@ -206,11 +206,7 @@ Status GetArrowType(const liborc::Type* type, std::shared_ptr<DataType>* out) {
*out = union_(fields, type_codes);
break;
}
default: {
std::stringstream ss;
ss << "Unknown Orc type kind: " << kind;
return Status::Invalid(ss.str());
}
default: { return Status::Invalid("Unknown Orc type kind: ", kind); }
}
return Status::OK();
}
Expand Down Expand Up @@ -346,11 +342,9 @@ class ORCFileReader::Impl {
}

Status SelectStripe(liborc::RowReaderOptions* opts, int64_t stripe) {
if (stripe < 0 || stripe >= NumberOfStripes()) {
std::stringstream ss;
ss << "Out of bounds stripe: " << stripe;
return Status::Invalid(ss.str());
}
ARROW_RETURN_IF(stripe < 0 || stripe >= NumberOfStripes(),
Status::Invalid("Out of bounds stripe: ", stripe));

opts->range(stripes_[stripe].offset, stripes_[stripe].length);
return Status::OK();
}
Expand All @@ -359,9 +353,7 @@ class ORCFileReader::Impl {
const std::vector<int>& include_indices) {
std::list<uint64_t> include_indices_list;
for (auto it = include_indices.begin(); it != include_indices.end(); ++it) {
if (*it < 0) {
return Status::Invalid("Negative field index");
}
ARROW_RETURN_IF(*it < 0, Status::Invalid("Negative field index"));
include_indices_list.push_back(*it);
}
opts->includeTypes(include_indices_list);
Expand Down Expand Up @@ -455,9 +447,7 @@ class ORCFileReader::Impl {
case liborc::DECIMAL:
return AppendDecimalBatch(type, batch, offset, length, builder);
default:
std::stringstream ss;
ss << "Not implemented type kind: " << kind;
return Status::NotImplemented(ss.str());
return Status::NotImplemented("Not implemented type kind: ", kind);
}
}

Expand Down
60 changes: 22 additions & 38 deletions cpp/src/arrow/array.cc
Expand Up @@ -638,9 +638,8 @@ Status DictionaryArray::FromArrays(const std::shared_ptr<DataType>& type,
is_valid = ValidateDictionaryIndices<Int64Type>(indices, upper_bound);
break;
default:
std::stringstream ss;
ss << "Categorical index type not supported: " << indices->type()->ToString();
return Status::NotImplemented(ss.str());
return Status::NotImplemented("Categorical index type not supported: ",
indices->type()->ToString());
}

if (!is_valid.ok()) {
Expand Down Expand Up @@ -740,12 +739,11 @@ struct ValidateVisitor {
Status Visit(const NullArray&) { return Status::OK(); }

Status Visit(const PrimitiveArray& array) {
if (array.data()->buffers.size() != 2) {
return Status::Invalid("number of buffers was != 2");
}
if (array.values() == nullptr) {
return Status::Invalid("values was null");
}
ARROW_RETURN_IF(array.data()->buffers.size() != 2,
Status::Invalid("number of buffers was != 2"));

ARROW_RETURN_IF(array.values() == nullptr, Status::Invalid("values was null"));

return Status::OK();
}

Expand Down Expand Up @@ -776,10 +774,8 @@ struct ValidateVisitor {
return Status::Invalid("value_offsets_ was null");
}
if (value_offsets->size() / static_cast<int>(sizeof(int32_t)) < array.length()) {
std::stringstream ss;
ss << "offset buffer size (bytes): " << value_offsets->size()
<< " isn't large enough for length: " << array.length();
return Status::Invalid(ss.str());
return Status::Invalid("offset buffer size (bytes): ", value_offsets->size(),
" isn't large enough for length: ", array.length());
}

if (!array.values()) {
Expand All @@ -788,17 +784,13 @@ struct ValidateVisitor {

const int32_t last_offset = array.value_offset(array.length());
if (array.values()->length() != last_offset) {
std::stringstream ss;
ss << "Final offset invariant not equal to values length: " << last_offset
<< "!=" << array.values()->length();
return Status::Invalid(ss.str());
return Status::Invalid("Final offset invariant not equal to values length: ",
last_offset, "!=", array.values()->length());
}

const Status child_valid = ValidateArray(*array.values());
if (!child_valid.ok()) {
std::stringstream ss;
ss << "Child array invalid: " << child_valid.ToString();
return Status::Invalid(ss.str());
return Status::Invalid("Child array invalid: ", child_valid.ToString());
}

int32_t prev_offset = array.value_offset(0);
Expand All @@ -808,18 +800,14 @@ struct ValidateVisitor {
for (int64_t i = 1; i <= array.length(); ++i) {
int32_t current_offset = array.value_offset(i);
if (array.IsNull(i - 1) && current_offset != prev_offset) {
std::stringstream ss;
ss << "Offset invariant failure at: " << i
<< " inconsistent value_offsets for null slot" << current_offset
<< "!=" << prev_offset;
return Status::Invalid(ss.str());
return Status::Invalid("Offset invariant failure at: ", i,
" inconsistent value_offsets for null slot",
current_offset, "!=", prev_offset);
}
if (current_offset < prev_offset) {
std::stringstream ss;
ss << "Offset invariant failure: " << i
<< " inconsistent offset for non-null slot: " << current_offset << "<"
<< prev_offset;
return Status::Invalid(ss.str());
return Status::Invalid("Offset invariant failure: ", i,
" inconsistent offset for non-null slot: ", current_offset,
"<", prev_offset);
}
prev_offset = current_offset;
}
Expand All @@ -842,18 +830,14 @@ struct ValidateVisitor {
for (int i = 0; i < array.num_fields(); ++i) {
auto it = array.field(i);
if (it->length() != array_length) {
std::stringstream ss;
ss << "Length is not equal from field " << it->type()->ToString()
<< " at position {" << idx << "}";
return Status::Invalid(ss.str());
return Status::Invalid("Length is not equal from field ",
it->type()->ToString(), " at position [", idx, "]");
}

const Status child_valid = ValidateArray(*it);
if (!child_valid.ok()) {
std::stringstream ss;
ss << "Child array invalid: " << child_valid.ToString() << " at position {"
<< idx << "}";
return Status::Invalid(ss.str());
return Status::Invalid("Child array invalid: ", child_valid.ToString(),
" at position [", idx, "}");
}
++idx;
}
Expand Down
21 changes: 9 additions & 12 deletions cpp/src/arrow/array/builder_binary.cc
Expand Up @@ -59,21 +59,18 @@ Status BinaryBuilder::Resize(int64_t capacity) {
}

Status BinaryBuilder::ReserveData(int64_t elements) {
if (value_data_length() + elements > value_data_capacity()) {
if (value_data_length() + elements > kBinaryMemoryLimit) {
return Status::CapacityError(
"Cannot reserve capacity larger than 2^31 - 1 for binary");
}
RETURN_NOT_OK(value_data_builder_.Reserve(elements));
}
return Status::OK();
const int64_t size = value_data_length() + elements;
ARROW_RETURN_IF(
size > kBinaryMemoryLimit,
Status::CapacityError("Cannot reserve capacity larger than 2^31 - 1 for binary"));

return (size > value_data_capacity()) ? value_data_builder_.Reserve(elements)
: Status::OK();
}

Status BinaryBuilder::AppendOverflow(int64_t num_bytes) {
std::stringstream ss;
ss << "BinaryArray cannot contain more than " << kBinaryMemoryLimit << " bytes, have "
<< num_bytes;
return Status::CapacityError(ss.str());
return Status::CapacityError("BinaryArray cannot contain more than ",
kBinaryMemoryLimit, " bytes, have ", num_bytes);
}

Status BinaryBuilder::FinishInternal(std::shared_ptr<ArrayData>* out) {
Expand Down
12 changes: 5 additions & 7 deletions cpp/src/arrow/array/builder_nested.cc
Expand Up @@ -58,13 +58,11 @@ Status ListBuilder::AppendValues(const int32_t* offsets, int64_t length,
}

Status ListBuilder::AppendNextOffset() {
int64_t num_values = value_builder_->length();
if (ARROW_PREDICT_FALSE(num_values > kListMaximumElements)) {
std::stringstream ss;
ss << "ListArray cannot contain more then INT32_MAX - 1 child elements,"
<< " have " << num_values;
return Status::CapacityError(ss.str());
}
const int64_t num_values = value_builder_->length();
ARROW_RETURN_IF(
num_values > kListMaximumElements,
Status::CapacityError("ListArray cannot contain more then 2^31 - 1 child elements,",
" have ", num_values));
return offsets_builder_.Append(static_cast<int32_t>(num_values));
}

Expand Down
5 changes: 2 additions & 3 deletions cpp/src/arrow/builder.cc
Expand Up @@ -93,9 +93,8 @@ Status MakeBuilder(MemoryPool* pool, const std::shared_ptr<DataType>& type,
}

default: {
std::stringstream ss;
ss << "MakeBuilder: cannot construct builder for type " << type->ToString();
return Status::NotImplemented(ss.str());
return Status::NotImplemented("MakeBuilder: cannot construct builder for type ",
type->ToString());
}
}
}
Expand Down
51 changes: 20 additions & 31 deletions cpp/src/arrow/compute/kernels/cast.cc
Expand Up @@ -508,11 +508,9 @@ void ShiftTime(FunctionContext* ctx, const CastOptions& options, const bool is_m
out_data[i] = static_cast<out_type>(in_data[i] / factor);
}
} else {
#define RAISE_INVALID_CAST(VAL) \
std::stringstream ss; \
ss << "Casting from " << input.type->ToString() << " to " << output->type->ToString() \
<< " would lose data: " << VAL; \
ctx->SetStatus(Status::Invalid(ss.str()));
#define RAISE_INVALID_CAST(VAL) \
ctx->SetStatus(Status::Invalid("Casting from ", input.type->ToString(), " to ", \
output->type->ToString(), " would lose data: ", VAL));

if (input.null_count != 0) {
internal::BitmapReader bit_reader(input.buffers[0]->data(), input.offset,
Expand Down Expand Up @@ -795,9 +793,8 @@ struct CastFunctor<
UnpackFixedSizeBinaryDictionary<Int64Type>(ctx, indices, dictionary, output);
break;
default:
std::stringstream ss;
ss << "Invalid index type: " << indices.type()->ToString();
ctx->SetStatus(Status::Invalid(ss.str()));
ctx->SetStatus(
Status::Invalid("Invalid index type: ", indices.type()->ToString()));
return;
}
}
Expand Down Expand Up @@ -874,9 +871,8 @@ struct CastFunctor<T, DictionaryType,
(UnpackBinaryDictionary<Int64Type>(ctx, indices, dictionary, output)));
break;
default:
std::stringstream ss;
ss << "Invalid index type: " << indices.type()->ToString();
ctx->SetStatus(Status::Invalid(ss.str()));
ctx->SetStatus(
Status::Invalid("Invalid index type: ", indices.type()->ToString()));
return;
}
}
Expand Down Expand Up @@ -932,9 +928,8 @@ struct CastFunctor<T, DictionaryType,
UnpackPrimitiveDictionary<Int64Type, c_type>(indices, dictionary, out);
break;
default:
std::stringstream ss;
ss << "Invalid index type: " << indices.type()->ToString();
ctx->SetStatus(Status::Invalid(ss.str()));
ctx->SetStatus(
Status::Invalid("Invalid index type: ", indices.type()->ToString()));
return;
}
}
Expand All @@ -960,9 +955,8 @@ struct CastFunctor<O, StringType, enable_if_number<O>> {

auto str = input_array.GetView(i);
if (!converter(str.data(), str.length(), out_data)) {
std::stringstream ss;
ss << "Failed to cast String '" << str << "' into " << output->type->ToString();
ctx->SetStatus(Status(StatusCode::Invalid, ss.str()));
ctx->SetStatus(Status::Invalid("Failed to cast String '", str, "' into ",
output->type->ToString()));
return;
}
}
Expand Down Expand Up @@ -991,10 +985,9 @@ struct CastFunctor<O, StringType,
bool value;
auto str = input_array.GetView(i);
if (!converter(str.data(), str.length(), &value)) {
std::stringstream ss;
ss << "Failed to cast String '" << input_array.GetString(i) << "' into "
<< output->type->ToString();
ctx->SetStatus(Status(StatusCode::Invalid, ss.str()));
ctx->SetStatus(Status::Invalid("Failed to cast String '",
input_array.GetString(i), "' into ",
output->type->ToString()));
return;
}

Expand Down Expand Up @@ -1029,9 +1022,8 @@ struct CastFunctor<TimestampType, StringType> {

const auto str = input_array.GetView(i);
if (!converter(str.data(), str.length(), out_data)) {
std::stringstream ss;
ss << "Failed to cast String '" << str << "' into " << output->type->ToString();
ctx->SetStatus(Status(StatusCode::Invalid, ss.str()));
ctx->SetStatus(Status::Invalid("Failed to cast String '", str, "' into ",
output->type->ToString()));
return;
}
}
Expand Down Expand Up @@ -1123,9 +1115,8 @@ static Status AllocateIfNotPreallocated(FunctionContext* ctx, const ArrayData& i

if (!(is_primitive(type_id) || type_id == Type::FIXED_SIZE_BINARY ||
type_id == Type::DECIMAL)) {
std::stringstream ss;
ss << "Cannot pre-allocate memory for type: " << out->type->ToString();
return Status::NotImplemented(ss.str());
return Status::NotImplemented("Cannot pre-allocate memory for type: ",
out->type->ToString());
}

if (type_id != Type::NA) {
Expand Down Expand Up @@ -1400,10 +1391,8 @@ Status GetCastFunction(const DataType& in_type, const std::shared_ptr<DataType>&
break;
}
if (*kernel == nullptr) {
std::stringstream ss;
ss << "No cast implemented from " << in_type.ToString() << " to "
<< out_type->ToString();
return Status::NotImplemented(ss.str());
return Status::NotImplemented("No cast implemented from ", in_type.ToString(), " to ",
out_type->ToString());
}
return Status::OK();
}
Expand Down
8 changes: 3 additions & 5 deletions cpp/src/arrow/compute/kernels/hash.cc
Expand Up @@ -56,11 +56,9 @@ namespace compute {

namespace {

#define CHECK_IMPLEMENTED(KERNEL, FUNCNAME, TYPE) \
if (!KERNEL) { \
std::stringstream ss; \
ss << FUNCNAME << " not implemented for " << type->ToString(); \
return Status::NotImplemented(ss.str()); \
#define CHECK_IMPLEMENTED(KERNEL, FUNCNAME, TYPE) \
if (!KERNEL) { \
return Status::NotImplemented(FUNCNAME, " not implemented for ", type->ToString()); \
}

// ----------------------------------------------------------------------
Expand Down

0 comments on commit ce9c6e3

Please sign in to comment.