ARROW-6508: [C++] Add Tensor and SparseTensor factory function with validations

[mrkn]

I'd like to add Make factory functions in Tensor and SparseTensor. These functions validate the given parameters.

The following validations run in Tensor::Make:

• Data type (non-null and is_tensor_supported)
• data pointer
• the size of shape
• elements of shape are all positive
• the consistency of shape and strides
• the consistency of strides and the size of data
• the consistency of the size of dim_names and shape

The following validations run in SparseTensor::Make:

• Data type
• the consistency of the sparse index and shape
• the consistency of the size of dim_names and shape

---

TODO:

• Use Result<T> as return types.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-6508

[wesm]

A couple of high level comments before I review the details:

• I might suggest calling these MakeSafe or similar since the Make constructors on other classes cannot fail (see Table::Make) (cc @fsaintjacques @pitrou for thoughts)
• Since you're adding new APIs, are these good candidates to use Result<T> instead of Status?

[mrkn]

I might suggest calling these MakeSafe or similar since the Make constructors on other classes cannot fail (see Table::Make)

OK, I understand. Renaming to MakeSafe is good for me, but I'm concerning that this name is a new name in C++ API.

Since you're adding new APIs, are these good candidates to use Result<T> instead of Status?

Oh, I forgot to rewrite this to use Result<T>. I will do it.

Thanks!

[mrkn]

I finished rewriting by using Result<T>, renaming to MakeSafe, and rebasing it onto the latest master branch.

I search uses of Result<T> and I found that StructArray::Make performs validations and returns Result<T>.
It may be better to rename it to StructArray::MakeSafe likewise.

[pitrou]

I would rather have all those methods named Make personally. The fact that they return a Result hint that there may be some validation going on.

[fsaintjacques]

The other (unsafe) Make functions also return Status and use an output parameter. The difference is whether they will logically validate (via Validate) the inputs.

We had this discussion previously, not sure if on the ML or in a PR review: Make|MakeUnsafe versus Make|MakeSafe.

[mrkn]

I guess it is enough to see the return type to distinguish whether a Make function may perform validations and can be failed, too.

But we have to use the different names if we want to introduce two factory functions, which have the same argument types and the different return types, and the one performs validations and the other does not.

[pitrou]

Well, why do we need the unsafe APIs here? You're only checking the shape and strides, so it's gonna be fast (unless you have 1000-dimensional tensors?).

[mrkn]

We don't need unsafe APIs in tensor and sparse tensor.

I just mentioned my anxiety about the potential issue of the naming convention that uses only Make. For example, we can consider adding a safe version of Table::Make:

Result<std::shared_ptr<Table>> Make(
    const std::shared_ptr<Schema>& schema,
    const std::vector<std::shared_ptr<ChunkedArray>>& columns,
    int64_t num_rows = -1
);

But we cannot add this because this function differs only the return type from the existing Table::Make.

[pitrou]

If we want to add a safe version of Table::Make, we can rename the unsafe version to Table::MakeUnsafe.

[mrkn]

OK, I'll revert the commit to rename Make to MakeSafe.

I found the previous discussion thread in ML. The discussion hasn't been settled yet.

[pitrou]

Thanks for doing this! There's some potential for simplification in some places. Also, the tests would deserve developing a bit.

[mrkn]

@pitrou I finished fixes for all of your comments. Could you please review this again?

[pitrou]

+1. Will merge when green, thank you!