[C++] BinaryBuilder doesn't preserve passed in DataType

[asfimport]

There is a constructor that takes a datatype for binary builder but it is discarded.  When constructing an Array the type is always the value returned from type() binary

If a consumer of the API wants to have an extension array this prevents them from passing the extension type though.

Reporter: Micah Kornfield / @emkornfield

Related issues:

• [C++] BinaryBuilder doesn't preserve passed in DataType (is duplicated by)

PRs and other links:

• GitHub Pull Request #11910

_{Note: This issue was originally created as ARROW-13672. Please see the migration documentation for further details.}

[asfimport]

Supun Kamburugamuva:
Should the solution be that we remove passing the type to the constructor?

[asfimport]

David Li / @lidavidm:
[~supun] probably the builder should store the given type and use it in FinishInternal (and presumably in type()).

[asfimport]

Supun Kamburugamuva:
The BaseBinaryBuilder has a constructor that accepts the type parameter but is not used. I think what the issue says is we should store this type and return it in the type() method. 

Someone using the BaseBinaryBuilder can override these two methods and achieve the same. So I'm thinking is there a value in implementing this? The FinishInternal is implemented correctly as it uses the type() method to get the type in a derived class. 

[asfimport]

David Li / @lidavidm:
Users probably wouldn't use BaseBinaryBuilder directly, it's referenced because it's the base implementation for BinaryBuilder/StringBuilder/etc. I don't think we should require users to override methods just to do something that the API suggests is already implemented. Also note NumericBuilder does store and use the given type.

As noted in the JIRA description, there is a use case for this: creating extension arrays (which have a custom logical type on top of an underlying physical type). Sure, the user could manually change the type after constructing the array or manually assemble the array, but that is more work.

[asfimport]

Todd Farmer / @toddfarmer:
This issue was last updated over 90 days ago, which may be an indication it is no longer being actively worked. To better reflect the current state, the issue is being unassigned. Please feel free to re-take assignment of the issue if it is being actively worked, or if you plan to start that work soon.

[shreyans800755]

Hi

I tried to work on this, but I think current implementation might be intentional. There are other builds which have the same interfaces. For ex:

arrow/cpp/src/arrow/array/builder_decimal.cc

Line 143 in 1114958

Decimal256Builder::Decimal256Builder(const std::shared_ptr<DataType>& type,

And we have places which deal with generic types and require interfaces to be compatible. For ex:

arrow/cpp/src/arrow/builder.cc

Line 199 in 1114958

out.reset(new typename TypeTraits<T>::BuilderType(type, pool));

We can put if constexpr or some metaprogramming here to construct builder per params. But that doesn't seem to be the cleanest approach either.

Please let me know what you think. @asfimport

[BITree2004]

I opened a PR addressing this issue: #50049

The PR preserves the DataType passed to BinaryBuilder/LargeBinaryBuilder and adds a regression test for a binary-backed extension type.

[pitrou]

I agree with @shreyans800755 here:

I tried to work on this, but I think current implementation might be intentional

If you want to build an extension array, you should first build the storage array and then wrap it explicitly with the extension type.

This issue is basically asking for all Builder classes to handle extension types, which is going to add a lot of logic for little benefit.