You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/12/09 04:35:10 UTC
[GitHub] [arrow] supunkamburugamuve opened a new pull request #11910: ARROW-13762: [CPP] Make BinaryBuilder preserves the type parameter
supunkamburugamuve opened a new pull request #11910:
URL: https://github.com/apache/arrow/pull/11910
This pull request preserves the passed-in type parameter to the BinaryBuilders.
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #11910: ARROW-13762: [CPP] Make BinaryBuilder preserves the type parameter
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11910:
URL: https://github.com/apache/arrow/pull/11910#issuecomment-989503637
https://issues.apache.org/jira/browse/ARROW-13762
--
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
[GitHub] [arrow] supunkamburugamuve commented on a change in pull request #11910: ARROW-13672: [C++] Make BinaryBuilder preserve the type parameter
Posted by GitBox <gi...@apache.org>.
supunkamburugamuve commented on a change in pull request #11910:
URL: https://github.com/apache/arrow/pull/11910#discussion_r765886962
##########
File path: cpp/src/arrow/array/builder_binary.h
##########
@@ -396,52 +401,62 @@ class BaseBinaryBuilder : public ArrayBuilder {
/// \brief Builder class for variable-length binary data
class ARROW_EXPORT BinaryBuilder : public BaseBinaryBuilder<BinaryType> {
public:
- using BaseBinaryBuilder::BaseBinaryBuilder;
+ explicit BinaryBuilder(MemoryPool* pool = default_memory_pool())
+ : BaseBinaryBuilder(binary(), pool) {}
+
+ BinaryBuilder(const std::shared_ptr<DataType> &type, MemoryPool *pool)
+ : BaseBinaryBuilder(type, pool) {}
Review comment:
This constructor is used in other places of the code with templates. It seems all the builders are expected to have this constructor.
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #11910: ARROW-13672: [C++] Make BinaryBuilder preserve the type parameter
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11910:
URL: https://github.com/apache/arrow/pull/11910#issuecomment-989902032
https://issues.apache.org/jira/browse/ARROW-13672
--
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
[GitHub] [arrow] bkietz commented on a change in pull request #11910: ARROW-13672: [C++] Make BinaryBuilder preserve the type parameter
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #11910:
URL: https://github.com/apache/arrow/pull/11910#discussion_r765845229
##########
File path: cpp/src/arrow/array/builder_binary.h
##########
@@ -51,10 +51,12 @@ class BaseBinaryBuilder : public ArrayBuilder {
using offset_type = typename TypeClass::offset_type;
explicit BaseBinaryBuilder(MemoryPool* pool = default_memory_pool())
- : ArrayBuilder(pool), offsets_builder_(pool), value_data_builder_(pool) {}
+ : ArrayBuilder(pool), offsets_builder_(pool),
+ value_data_builder_(pool), type_(binary()) {}
BaseBinaryBuilder(const std::shared_ptr<DataType>& type, MemoryPool* pool)
- : BaseBinaryBuilder(pool) {}
+ : ArrayBuilder(pool), offsets_builder_(pool),
+ value_data_builder_(pool), type_(type) {}
Review comment:
Please make this constructor `protected`; this will make it clearer that BaseBinaryBuilder is intended for use by subclasses only
##########
File path: cpp/src/arrow/array/builder_binary.h
##########
@@ -396,52 +401,62 @@ class BaseBinaryBuilder : public ArrayBuilder {
/// \brief Builder class for variable-length binary data
class ARROW_EXPORT BinaryBuilder : public BaseBinaryBuilder<BinaryType> {
public:
- using BaseBinaryBuilder::BaseBinaryBuilder;
+ explicit BinaryBuilder(MemoryPool* pool = default_memory_pool())
+ : BaseBinaryBuilder(binary(), pool) {}
+
+ BinaryBuilder(const std::shared_ptr<DataType> &type, MemoryPool *pool)
+ : BaseBinaryBuilder(type, pool) {}
Review comment:
For this constructor and the other constructors below which accept an explicit type, please do one of:
- delete them
- make them protected as well
- rewrite them to `BinaryBuilder(const std::shared_ptr<ExtensionType> &type, MemoryPool *pool = default_memory_pool())`, since the only valid case I can see for allowing public construction of a `BinaryBuilder` with an output type other than `binary()` is building an extension array (which AFAICT is not used anywhere nor is it tested so my preference would be deletion for now)
##########
File path: cpp/src/arrow/array/builder_binary.h
##########
@@ -51,10 +51,12 @@ class BaseBinaryBuilder : public ArrayBuilder {
using offset_type = typename TypeClass::offset_type;
explicit BaseBinaryBuilder(MemoryPool* pool = default_memory_pool())
- : ArrayBuilder(pool), offsets_builder_(pool), value_data_builder_(pool) {}
+ : ArrayBuilder(pool), offsets_builder_(pool),
+ value_data_builder_(pool), type_(binary()) {}
Review comment:
Please delete this constructor; I think it's more understandable if all subclasses of BaseBinaryBuilder are required to pass the resulting type
##########
File path: cpp/src/arrow/compute/kernels/scalar_cast_string.cc
##########
@@ -58,7 +58,7 @@ struct NumericToStringCastFunctor {
static Status Convert(KernelContext* ctx, const ArrayData& input, ArrayData* output) {
FormatterType formatter(input.type);
- BuilderType builder(input.type, ctx->memory_pool());
+ BuilderType builder(utf8(), ctx->memory_pool());
Review comment:
IIUC, this will cause the output to be typed `utf8` even if we're casting to `large_utf8`. I think it's better to rely on the default constructor (which sets the correct output type without the need to be explicit):
```suggestion
BuilderType builder(ctx->memory_pool());
```
--
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
[GitHub] [arrow] supunkamburugamuve commented on a change in pull request #11910: ARROW-13672: [C++] Make BinaryBuilder preserve the type parameter
Posted by GitBox <gi...@apache.org>.
supunkamburugamuve commented on a change in pull request #11910:
URL: https://github.com/apache/arrow/pull/11910#discussion_r767308827
##########
File path: cpp/src/arrow/array/builder_binary.h
##########
@@ -51,10 +51,12 @@ class BaseBinaryBuilder : public ArrayBuilder {
using offset_type = typename TypeClass::offset_type;
explicit BaseBinaryBuilder(MemoryPool* pool = default_memory_pool())
- : ArrayBuilder(pool), offsets_builder_(pool), value_data_builder_(pool) {}
+ : ArrayBuilder(pool), offsets_builder_(pool),
+ value_data_builder_(pool), type_(binary()) {}
Review comment:
Done
##########
File path: cpp/src/arrow/array/builder_binary.h
##########
@@ -51,10 +51,12 @@ class BaseBinaryBuilder : public ArrayBuilder {
using offset_type = typename TypeClass::offset_type;
explicit BaseBinaryBuilder(MemoryPool* pool = default_memory_pool())
- : ArrayBuilder(pool), offsets_builder_(pool), value_data_builder_(pool) {}
+ : ArrayBuilder(pool), offsets_builder_(pool),
+ value_data_builder_(pool), type_(binary()) {}
BaseBinaryBuilder(const std::shared_ptr<DataType>& type, MemoryPool* pool)
- : BaseBinaryBuilder(pool) {}
+ : ArrayBuilder(pool), offsets_builder_(pool),
+ value_data_builder_(pool), type_(type) {}
Review comment:
Done
##########
File path: cpp/src/arrow/compute/kernels/scalar_cast_string.cc
##########
@@ -58,7 +58,7 @@ struct NumericToStringCastFunctor {
static Status Convert(KernelContext* ctx, const ArrayData& input, ArrayData* output) {
FormatterType formatter(input.type);
- BuilderType builder(input.type, ctx->memory_pool());
+ BuilderType builder(utf8(), ctx->memory_pool());
Review comment:
Done
--
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
[GitHub] [arrow] github-actions[bot] removed a comment on pull request #11910: ARROW-13672: [C++] Make BinaryBuilder preserve the type parameter
Posted by GitBox <gi...@apache.org>.
github-actions[bot] removed a comment on pull request #11910:
URL: https://github.com/apache/arrow/pull/11910#issuecomment-989503637
https://issues.apache.org/jira/browse/ARROW-13762
--
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
[GitHub] [arrow] bkmgit commented on pull request #11910: ARROW-13762: [CPP] Make BinaryBuilder preserves the type parameter
Posted by GitBox <gi...@apache.org>.
bkmgit commented on pull request #11910:
URL: https://github.com/apache/arrow/pull/11910#issuecomment-989546789
Thanks for your pull request. Please change the title to:
- ARROW-13762: [C++] Make BinaryBuilder preserve the type parameter
--
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