You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "BITree2004 (via GitHub)" <gi...@apache.org> on 2026/05/27 10:01:36 UTC
[PR] GH-29309: [C++] Preserve BinaryBuilder data type [arrow]
BITree2004 opened a new pull request, #50049:
URL: https://github.com/apache/arrow/pull/50049
### Rationale for this change
BinaryBuilder accepted a DataType in its constructor but discarded it, so finishing through ArrayBuilder::Finish() always produced a plain binary array instead of preserving the supplied type, such as an extension type backed by binary storage.
### What changes are included in this PR?
- Store the supplied DataType in BaseBinaryBuilder.
- Use the stored type for BinaryBuilder and LargeBinaryBuilder output.
- Add a regression test covering BinaryBuilder with a binary-backed extension type.
### Are these changes tested?
Yes.
```bash
ninja -C cpp/build arrow-array-test
./cpp/build/release/arrow-array-test --gtest_filter=BinaryBuilder.PreservesDataType
./cpp/build/release/arrow-array-test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-29309: [C++] Preserve BinaryBuilder data type [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #50049:
URL: https://github.com/apache/arrow/pull/50049#issuecomment-4553599556
:warning: GitHub issue #29309 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-29309: [C++] Preserve BinaryBuilder data type [arrow]
Posted by "BITree2004 (via GitHub)" <gi...@apache.org>.
BITree2004 commented on PR #50049:
URL: https://github.com/apache/arrow/pull/50049#issuecomment-4565608156
Addressed the Copilot review comments in 6ad9de9:
- validate the stored binary/large-binary builder type, including extension storage type
- use the type singleton for default binary builders
- guard typed Finish(...) overloads so extension-typed builders return TypeError instead of casting to BinaryArray/LargeBinaryArray
- add regression coverage for BinaryBuilder and LargeBinaryBuilder
Local verification:
- cmake --build cpp/build-pr-50049 --target arrow-array-test
- ./cpp/build-pr-50049/release/arrow-array-test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-29309: [C++] Preserve BinaryBuilder data type [arrow]
Posted by "BITree2004 (via GitHub)" <gi...@apache.org>.
BITree2004 commented on PR #50049:
URL: https://github.com/apache/arrow/pull/50049#issuecomment-4582501981
@kou Please, rerun Copilot
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-29309: [C++] Preserve BinaryBuilder data type [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #50049:
URL: https://github.com/apache/arrow/pull/50049#issuecomment-4553510888
:warning: GitHub issue #29309 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-29309: [C++] Preserve BinaryBuilder data type [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #50049:
URL: https://github.com/apache/arrow/pull/50049#issuecomment-4553548236
:warning: GitHub issue #29309 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-29309: [C++] Preserve BinaryBuilder data type [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #50049:
URL: https://github.com/apache/arrow/pull/50049#discussion_r3315093613
##########
cpp/src/arrow/array/builder_binary.h:
##########
@@ -415,8 +422,6 @@ class ARROW_EXPORT BinaryBuilder : public BaseBinaryBuilder<BinaryType> {
/// \endcond
Status Finish(std::shared_ptr<BinaryArray>* out) { return FinishTyped(out); }
Review Comment:
`BinaryBuilder::Finish(std::shared_ptr<BinaryArray>* ...)` still calls `FinishTyped`, which goes through `ArrayBuilder::Finish()` and then `std::static_pointer_cast<BinaryArray>`. Now that `BinaryBuilder::type()` may return an extension type, `Finish()` can produce an `ExtensionArray`, making this cast undefined behavior. Consider overriding this `Finish(BinaryArray*)` overload to either (a) return a `TypeError` when `type()->id()` is not `Type::BINARY`, or (b) explicitly finish to a storage `BinaryArray` when the builder was constructed with an extension type.
##########
cpp/src/arrow/array/builder_binary.h:
##########
@@ -60,11 +60,15 @@ class BaseBinaryBuilder
explicit BaseBinaryBuilder(MemoryPool* pool = default_memory_pool(),
int64_t alignment = kDefaultBufferAlignment)
: ArrayBuilder(pool, alignment),
+ type_(std::make_shared<TypeClass>()),
offsets_builder_(pool, alignment),
value_data_builder_(pool, alignment) {}
BaseBinaryBuilder(const std::shared_ptr<DataType>& type, MemoryPool* pool)
- : BaseBinaryBuilder(pool) {}
+ : ArrayBuilder(pool),
+ type_(type),
+ offsets_builder_(pool),
+ value_data_builder_(pool) {}
Review Comment:
The `BaseBinaryBuilder(const std::shared_ptr<DataType>& type, ...)` constructor now stores and later uses the passed-in `type` to build `ArrayData`. Since this is a public constructor (inherited by `BinaryBuilder` / `LargeBinaryBuilder`), passing an incompatible type (wrong id / wrong offset width / wrong layout) would now yield an invalid array. It would be safer to DCHECK/ARROW_CHECK that `type` is either the expected base binary type or an extension type with a matching storage type, and also check `type` is non-null.
##########
cpp/src/arrow/array/builder_binary.h:
##########
@@ -445,8 +450,6 @@ class ARROW_EXPORT LargeBinaryBuilder : public BaseBinaryBuilder<LargeBinaryType
/// \endcond
Status Finish(std::shared_ptr<LargeBinaryArray>* out) { return FinishTyped(out); }
Review Comment:
`LargeBinaryBuilder::Finish(std::shared_ptr<LargeBinaryArray>* ...)` uses `FinishTyped`, which performs a `std::static_pointer_cast` after calling `ArrayBuilder::Finish()`. With this PR, `type()` can now be an extension type, so `Finish()` may return an `ExtensionArray` and the static cast becomes undefined behavior. Suggest guarding this overload (e.g., return `TypeError` unless `type()->id() == Type::LARGE_BINARY`) or finishing explicitly to a storage `LargeBinaryArray` when `type()` is an extension type.
##########
cpp/src/arrow/array/builder_binary.h:
##########
@@ -60,11 +60,15 @@ class BaseBinaryBuilder
explicit BaseBinaryBuilder(MemoryPool* pool = default_memory_pool(),
int64_t alignment = kDefaultBufferAlignment)
: ArrayBuilder(pool, alignment),
+ type_(std::make_shared<TypeClass>()),
Review Comment:
The default `BaseBinaryBuilder` constructor now does `type_(std::make_shared<TypeClass>())`, which allocates a new type instance per builder. For consistency with other builders and to avoid repeated allocations, consider initializing with the appropriate singleton (e.g., `TypeTraits<TypeClass>::type_singleton()` / `binary()` / `large_binary()`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-29309: [C++] Preserve BinaryBuilder data type [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #50049:
URL: https://github.com/apache/arrow/pull/50049#issuecomment-4560432511
:warning: GitHub issue #29309 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-29309: [C++] Preserve BinaryBuilder data type [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #50049:
URL: https://github.com/apache/arrow/pull/50049#discussion_r3328565610
##########
cpp/src/arrow/array/builder_binary.h:
##########
@@ -390,6 +400,32 @@ class BaseBinaryBuilder
}
protected:
+ template <typename LogicalType>
+ static std::shared_ptr<DataType> ValidateType(std::shared_ptr<DataType> type) {
+ ARROW_CHECK(type != nullptr) << "Cannot construct binary builder with null type";
+ const auto expected_type = TypeTraits<LogicalType>::type_singleton();
Review Comment:
ValidateType() uses ARROW_CHECK for user-supplied constructor inputs (both null type and mismatched logical/storage types). ARROW_CHECK will terminate the process in release builds; for public builders, prefer returning a Status error (e.g. validate in FinishInternal / Finish) or otherwise avoid a fatal check on invalid inputs.
##########
cpp/src/arrow/array/array_binary_test.cc:
##########
@@ -48,6 +50,35 @@ namespace arrow {
using internal::checked_cast;
+class BinaryExtensionType : public ExtensionType {
+ public:
+ BinaryExtensionType(std::shared_ptr<DataType> storage_type, std::string extension_name)
+ : ExtensionType(std::move(storage_type)),
+ extension_name_(std::move(extension_name)) {}
+
+ std::string extension_name() const override { return extension_name_; }
+
+ bool ExtensionEquals(const ExtensionType& other) const override {
+ return other.extension_name() == this->extension_name() &&
+ storage_type()->Equals(*other.storage_type());
+ }
+
+ std::shared_ptr<Array> MakeArray(std::shared_ptr<ArrayData> data) const override {
+ return std::make_shared<ExtensionArray>(std::move(data));
+ }
+
+ Result<std::shared_ptr<DataType>> Deserialize(
+ std::shared_ptr<DataType> storage_type,
+ const std::string& serialized) const override {
+ return Status::NotImplemented("BinaryExtensionType::Deserialize");
Review Comment:
BinaryExtensionType::Deserialize() doesn't use its parameters, which can trigger -Wunused-parameter warnings in some build configurations. Consider omitting the parameter names (or explicitly marking them unused) to keep tests warning-clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org