You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "jsjtxietian (via GitHub)" <gi...@apache.org> on 2023/07/27 16:17:06 UTC
[GitHub] [arrow] jsjtxietian opened a new pull request, #36915: GH-36867: [C++] Add a struct_ and schema overload taking a vector of (name, type) pairs
jsjtxietian opened a new pull request, #36915:
URL: https://github.com/apache/arrow/pull/36915
### Rationale for this change
Mostly for convenience. It would be nice to be able to write:
```struct_({{"a", int8()}, {"b", utf8()}});```
instead of:
```struct_({field("a", int8()), field("b", utf8())});```
Same with the schema factory.
### What changes are included in this PR?
Add a struct_ overload and two schema overload taking a vector of (name, type) pairs to construct a vector of fields.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes. Add three ARROW_EXPORTfunctions.
* closes #36867
--
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] conbench-apache-arrow[bot] commented on pull request #36915: GH-36867: [C++] Add a struct_ and schema overload taking a vector of (name, type) pairs
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #36915:
URL: https://github.com/apache/arrow/pull/36915#issuecomment-1675998892
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit d3ccc833a61b70a988090cd8065d3e38d7c29a89.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/15843519450) has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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] jsjtxietian commented on a diff in pull request #36915: GH-36867: [C++] Add a struct_ and schema overload taking a vector of (name, type) pairs
Posted by "jsjtxietian (via GitHub)" <gi...@apache.org>.
jsjtxietian commented on code in PR #36915:
URL: https://github.com/apache/arrow/pull/36915#discussion_r1277151668
##########
cpp/src/arrow/type.cc:
##########
@@ -2090,17 +2090,39 @@ Status SchemaBuilder::AreCompatible(const std::vector<std::shared_ptr<Schema>>&
return Merge(schemas, policy).status();
}
+std::vector<std::shared_ptr<Field>> make_fields(
+ const std::initializer_list<std::pair<std::string, std::shared_ptr<DataType>>>& init_list) {
+ std::vector<std::shared_ptr<Field>> fields;
+ fields.reserve(init_list.size());
+ for (const auto& pair : init_list) {
+ fields.push_back(std::make_shared<Field>(pair.first, pair.second));
+ }
+ return fields;
+}
+
std::shared_ptr<Schema> schema(std::vector<std::shared_ptr<Field>> fields,
std::shared_ptr<const KeyValueMetadata> metadata) {
return std::make_shared<Schema>(std::move(fields), std::move(metadata));
}
+std::shared_ptr<Schema> schema(const std::initializer_list<std::pair<std::string, std::shared_ptr<DataType>>>& fields,
+ std::shared_ptr<const KeyValueMetadata> metadata) {
+ return std::make_shared<Schema>(std::move(make_fields(fields)), std::move(metadata));
Review Comment:
indeed, thank you
--
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] jsjtxietian commented on pull request #36915: GH-36867: [C++] Add a struct_ and schema overload taking a vector of (name, type) pairs
Posted by "jsjtxietian (via GitHub)" <gi...@apache.org>.
jsjtxietian commented on PR #36915:
URL: https://github.com/apache/arrow/pull/36915#issuecomment-1671019244
Thank you very much for all your help!
--
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 #36915: GH-36867: [C++] Add a struct_ and schema overload taking a vector of (name, type) pairs
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36915:
URL: https://github.com/apache/arrow/pull/36915#issuecomment-1653936189
:warning: GitHub issue #36867 **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
[GitHub] [arrow] pitrou commented on a diff in pull request #36915: GH-36867: [C++] Add a struct_ and schema overload taking a vector of (name, type) pairs
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #36915:
URL: https://github.com/apache/arrow/pull/36915#discussion_r1287279072
##########
cpp/src/arrow/type_fwd.h:
##########
@@ -560,6 +560,10 @@ ARROW_EXPORT std::shared_ptr<DataType> time64(TimeUnit::type unit);
ARROW_EXPORT std::shared_ptr<DataType> struct_(
const std::vector<std::shared_ptr<Field>>& fields);
+/// \brief Create a StructType instance by initializer_list
Review Comment:
```suggestion
/// \brief Create a StructType instance from (name, type) pairs
///
/// The struct's fields will all be nullable with no associated metadata.
```
##########
cpp/src/arrow/type_fwd.h:
##########
@@ -640,6 +654,17 @@ std::shared_ptr<Schema> schema(
std::vector<std::shared_ptr<Field>> fields, Endianness endianness,
std::shared_ptr<const KeyValueMetadata> metadata = NULLPTR);
+/// \brief Create a Schema instance
+///
+/// \param fields the schema's fields in the form of initializer_list
Review Comment:
```suggestion
/// \param fields (name, type) pairs of the schema's fields
```
##########
cpp/src/arrow/type_fwd.h:
##########
@@ -629,6 +633,16 @@ std::shared_ptr<Schema> schema(
std::vector<std::shared_ptr<Field>> fields,
std::shared_ptr<const KeyValueMetadata> metadata = NULLPTR);
+/// \brief Create a Schema instance
+///
+/// \param fields the schema's fields in the form of initializer_list
Review Comment:
```suggestion
/// \param fields (name, type) pairs of the schema's fields
```
##########
cpp/src/arrow/type.cc:
##########
@@ -2090,17 +2090,39 @@ Status SchemaBuilder::AreCompatible(const std::vector<std::shared_ptr<Schema>>&
return Merge(schemas, policy).status();
}
+std::vector<std::shared_ptr<Field>> make_fields(
+ std::initializer_list<std::pair<std::string, std::shared_ptr<DataType>>> init_list) {
Review Comment:
Can you please put this helper function in the anonymous namespace, and also make the function name CamelCase (i.e. `MakeFields`)?
##########
cpp/src/arrow/type.cc:
##########
@@ -2090,17 +2090,39 @@ Status SchemaBuilder::AreCompatible(const std::vector<std::shared_ptr<Schema>>&
return Merge(schemas, policy).status();
}
+std::vector<std::shared_ptr<Field>> make_fields(
+ std::initializer_list<std::pair<std::string, std::shared_ptr<DataType>>> init_list) {
+ std::vector<std::shared_ptr<Field>> fields;
+ fields.reserve(init_list.size());
+ for (const auto& [name, type] : init_list) {
+ fields.push_back(field(name, type));
+ }
Review Comment:
Not sure this will work, but worth trying:
```suggestion
for (auto&& [name, type] : init_list) {
fields.push_back(field(std::move(name), std::move(type)));
}
```
@bkietz Thoughts?
##########
cpp/src/arrow/type_fwd.h:
##########
@@ -640,6 +654,17 @@ std::shared_ptr<Schema> schema(
std::vector<std::shared_ptr<Field>> fields, Endianness endianness,
std::shared_ptr<const KeyValueMetadata> metadata = NULLPTR);
+/// \brief Create a Schema instance
+///
Review Comment:
```suggestion
/// \brief Create a Schema instance from (name, type) pairs
///
/// The schema's fields will all be nullable with no associated metadata.
///
```
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -839,7 +839,8 @@ std::function<std::shared_ptr<::arrow::DataType>(FieldVector)> GetNestedFactory(
switch (inferred_type.id()) {
case ::arrow::Type::STRUCT:
if (origin_type.id() == ::arrow::Type::STRUCT) {
- return ::arrow::struct_;
+ return static_cast<std::shared_ptr<::arrow::DataType> (*)(const FieldVector&)>(
+ &::arrow::struct_);
Review Comment:
Or can use a lambda for clarity:
```suggestion
return [](FieldVector fields) {
return ::arrow::struct_(std::move(fields));
};
```
##########
cpp/src/arrow/type_fwd.h:
##########
@@ -629,6 +633,16 @@ std::shared_ptr<Schema> schema(
std::vector<std::shared_ptr<Field>> fields,
std::shared_ptr<const KeyValueMetadata> metadata = NULLPTR);
+/// \brief Create a Schema instance
+///
Review Comment:
```suggestion
/// \brief Create a Schema instance from (name, type) pairs
///
/// The schema's fields will all be nullable with no associated metadata.
///
```
--
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] jsjtxietian commented on a diff in pull request #36915: GH-36867: [C++] Add a struct_ and schema overload taking a vector of (name, type) pairs
Posted by "jsjtxietian (via GitHub)" <gi...@apache.org>.
jsjtxietian commented on code in PR #36915:
URL: https://github.com/apache/arrow/pull/36915#discussion_r1287316244
##########
cpp/src/arrow/type.cc:
##########
@@ -2090,17 +2090,39 @@ Status SchemaBuilder::AreCompatible(const std::vector<std::shared_ptr<Schema>>&
return Merge(schemas, policy).status();
}
+std::vector<std::shared_ptr<Field>> make_fields(
+ std::initializer_list<std::pair<std::string, std::shared_ptr<DataType>>> init_list) {
+ std::vector<std::shared_ptr<Field>> fields;
+ fields.reserve(init_list.size());
+ for (const auto& [name, type] : init_list) {
+ fields.push_back(field(name, type));
+ }
Review Comment:
I will take a look which one is better using a similar demo at compiler explorer tomorrow.
--
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] jsjtxietian commented on pull request #36915: GH-36867: [C++] Add a struct_ and schema overload taking a vector of (name, type) pairs
Posted by "jsjtxietian (via GitHub)" <gi...@apache.org>.
jsjtxietian commented on PR #36915:
URL: https://github.com/apache/arrow/pull/36915#issuecomment-1655067076
ok, will take a look
--
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] mapleFU commented on a diff in pull request #36915: GH-36867: [C++] Add a struct_ and schema overload taking a vector of (name, type) pairs
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #36915:
URL: https://github.com/apache/arrow/pull/36915#discussion_r1277085700
##########
cpp/src/arrow/type.cc:
##########
@@ -2090,17 +2090,39 @@ Status SchemaBuilder::AreCompatible(const std::vector<std::shared_ptr<Schema>>&
return Merge(schemas, policy).status();
}
+std::vector<std::shared_ptr<Field>> make_fields(
+ const std::initializer_list<std::pair<std::string, std::shared_ptr<DataType>>>& init_list) {
+ std::vector<std::shared_ptr<Field>> fields;
+ fields.reserve(init_list.size());
+ for (const auto& pair : init_list) {
+ fields.push_back(std::make_shared<Field>(pair.first, pair.second));
+ }
+ return fields;
+}
+
std::shared_ptr<Schema> schema(std::vector<std::shared_ptr<Field>> fields,
std::shared_ptr<const KeyValueMetadata> metadata) {
return std::make_shared<Schema>(std::move(fields), std::move(metadata));
}
+std::shared_ptr<Schema> schema(const std::initializer_list<std::pair<std::string, std::shared_ptr<DataType>>>& fields,
+ std::shared_ptr<const KeyValueMetadata> metadata) {
+ return std::make_shared<Schema>(std::move(make_fields(fields)), std::move(metadata));
Review Comment:
`make_fields()` return a prvalue, not need to be moved.
##########
cpp/src/arrow/type.cc:
##########
@@ -2090,17 +2090,39 @@ Status SchemaBuilder::AreCompatible(const std::vector<std::shared_ptr<Schema>>&
return Merge(schemas, policy).status();
}
+std::vector<std::shared_ptr<Field>> make_fields(
+ const std::initializer_list<std::pair<std::string, std::shared_ptr<DataType>>>& init_list) {
+ std::vector<std::shared_ptr<Field>> fields;
+ fields.reserve(init_list.size());
+ for (const auto& pair : init_list) {
+ fields.push_back(std::make_shared<Field>(pair.first, pair.second));
+ }
+ return fields;
+}
+
std::shared_ptr<Schema> schema(std::vector<std::shared_ptr<Field>> fields,
std::shared_ptr<const KeyValueMetadata> metadata) {
return std::make_shared<Schema>(std::move(fields), std::move(metadata));
}
+std::shared_ptr<Schema> schema(const std::initializer_list<std::pair<std::string, std::shared_ptr<DataType>>>& fields,
+ std::shared_ptr<const KeyValueMetadata> metadata) {
+ return std::make_shared<Schema>(std::move(make_fields(fields)), std::move(metadata));
+}
+
+
std::shared_ptr<Schema> schema(std::vector<std::shared_ptr<Field>> fields,
Endianness endianness,
std::shared_ptr<const KeyValueMetadata> metadata) {
return std::make_shared<Schema>(std::move(fields), endianness, std::move(metadata));
}
+std::shared_ptr<Schema> schema(const std::initializer_list<std::pair<std::string, std::shared_ptr<DataType>>>& fields,
+ Endianness endianness,
+ std::shared_ptr<const KeyValueMetadata> metadata) {
+ return std::make_shared<Schema>(std::move(make_fields(fields)), endianness, std::move(metadata));
Review Comment:
ditto
--
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 diff in pull request #36915: GH-36867: [C++] Add a struct_ and schema overload taking a vector of (name, type) pairs
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #36915:
URL: https://github.com/apache/arrow/pull/36915#discussion_r1283282577
##########
cpp/src/arrow/type_fwd.h:
##########
@@ -640,6 +654,18 @@ std::shared_ptr<Schema> schema(
std::vector<std::shared_ptr<Field>> fields, Endianness endianness,
std::shared_ptr<const KeyValueMetadata> metadata = NULLPTR);
+/// \brief Create a Schema instance
+///
+/// \param fields the schema's fields in the form of initializer_list
+/// \param endianness the endianness of the data
+/// \param metadata any custom key-value metadata, default null
+/// \return schema shared_ptr to Schema
+ARROW_EXPORT
+std::shared_ptr<Schema> schema(
+ const std::initializer_list<std::pair<std::string, std::shared_ptr<DataType>>>&
Review Comment:
One more
--
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 diff in pull request #36915: GH-36867: [C++] Add a struct_ and schema overload taking a vector of (name, type) pairs
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #36915:
URL: https://github.com/apache/arrow/pull/36915#discussion_r1287323046
##########
cpp/src/arrow/type.cc:
##########
@@ -2090,17 +2090,39 @@ Status SchemaBuilder::AreCompatible(const std::vector<std::shared_ptr<Schema>>&
return Merge(schemas, policy).status();
}
+std::vector<std::shared_ptr<Field>> make_fields(
+ std::initializer_list<std::pair<std::string, std::shared_ptr<DataType>>> init_list) {
+ std::vector<std::shared_ptr<Field>> fields;
+ fields.reserve(init_list.size());
+ for (const auto& [name, type] : init_list) {
+ fields.push_back(field(name, type));
+ }
Review Comment:
The elements of an initializer list are always const, so calling move() on them won't accomplish anything. We could elide the copy by making the initializer lists accept string_view instead of string, though:
```
std::vector<std::shared_ptr<Field>> MakeFields(
std::initializer_list<std::pair<std::string_view, std::shared_ptr<DataType>>> init_list) {
for (const auto& [name, type] : init_list) {
fields.push_back(field(std::string{name}, type));
}
}
```
However I've seldom seen a field name longer than 22 chars, so they'd be inlined in most STL impls.
From a perf golf standpoint, the larger annoyance is the type, which must be copied (since we don't have a nice implicitly constructible non-owning DataType holder) and which hits atomics when copied. If you wanted to get around *that* you'd need something like:
```
struct DataTypeView {
// NOLINTNEXTLINE
DataTypeView(const std::shared_ptr<DataType>& type) : type{type} {}
// NOLINTNEXTLINE
operator std::shared_ptr<DataType>() const { return type; }
const std::shared_ptr<DataType>& type;
};
std::vector<std::shared_ptr<Field>> MakeFields(
std::initializer_list<std::pair<std::string_view, DataTypeView>> init_list) {
for (const auto& [name, type] : init_list) {
fields.push_back(field(std::string{name}, type));
}
}
```
--
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] jsjtxietian commented on a diff in pull request #36915: GH-36867: [C++] Add a struct_ and schema overload taking a vector of (name, type) pairs
Posted by "jsjtxietian (via GitHub)" <gi...@apache.org>.
jsjtxietian commented on code in PR #36915:
URL: https://github.com/apache/arrow/pull/36915#discussion_r1283931184
##########
cpp/src/arrow/type_fwd.h:
##########
@@ -640,6 +654,18 @@ std::shared_ptr<Schema> schema(
std::vector<std::shared_ptr<Field>> fields, Endianness endianness,
std::shared_ptr<const KeyValueMetadata> metadata = NULLPTR);
+/// \brief Create a Schema instance
+///
+/// \param fields the schema's fields in the form of initializer_list
+/// \param endianness the endianness of the data
+/// \param metadata any custom key-value metadata, default null
+/// \return schema shared_ptr to Schema
+ARROW_EXPORT
+std::shared_ptr<Schema> schema(
+ const std::initializer_list<std::pair<std::string, std::shared_ptr<DataType>>>&
Review Comment:
Thank you very mych for your suggestion! Sorry I missed that one. A big help.
--
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] jsjtxietian commented on a diff in pull request #36915: GH-36867: [C++] Add a struct_ and schema overload taking a vector of (name, type) pairs
Posted by "jsjtxietian (via GitHub)" <gi...@apache.org>.
jsjtxietian commented on code in PR #36915:
URL: https://github.com/apache/arrow/pull/36915#discussion_r1287314457
##########
cpp/src/arrow/type.cc:
##########
@@ -2090,17 +2090,39 @@ Status SchemaBuilder::AreCompatible(const std::vector<std::shared_ptr<Schema>>&
return Merge(schemas, policy).status();
}
+std::vector<std::shared_ptr<Field>> make_fields(
+ std::initializer_list<std::pair<std::string, std::shared_ptr<DataType>>> init_list) {
Review Comment:
absolutely!
--
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] mapleFU commented on pull request #36915: GH-36867: [C++] Add a struct_ and schema overload taking a vector of (name, type) pairs
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #36915:
URL: https://github.com/apache/arrow/pull/36915#issuecomment-1655000063
```
C:\projects\arrow\cpp\build\src\arrow\engine\CMakeFiles\arrow_substrait_shared.dir\Unity\unity_1_cxx.cxx
C:/projects/arrow/cpp/src/arrow/engine/substrait/type_internal.cc(80): error C2672: 'arrow::Result<arrow::FieldVector>::Map': no matching overloaded function found
C:/projects/arrow/cpp/src/arrow/engine/substrait/type_internal.cc(164): note: see reference to function template instantiation 'arrow::Result<arrow::FieldVector> arrow::engine::`anonymous-namespace'::FieldsFromProto<google::protobuf::RepeatedPtrField<substrait::Type>,arrow::engine::FromProto::<lambda_998f5b636b6257cbec583f504695a06c>>(int,const Types &,const NextName &,const arrow::engine::ExtensionSet &,const arrow::engine::ConversionOptions &)' being compiled
with
[
Types=google::protobuf::RepeatedPtrField<substrait::Type>,
NextName=arrow::engine::FromProto::<lambda_998f5b636b6257cbec583f504695a06c>
]
C:/projects/arrow/cpp/src/arrow/engine/substrait/type_internal.cc(80): error C2783: 'EnsureResult<unknown-type>::type arrow::Result<arrow::FieldVector>::Map(M &&) const &': could not deduce template argument for 'M'
C:\projects\arrow\cpp\src\arrow/result.h(392): note: see declaration of 'arrow::Result<arrow::FieldVector>::Map'
C:/projects/arrow/cpp/src/arrow/engine/substrait/type_internal.cc(80): error C2783: 'EnsureResult<unknown-type>::type arrow::Result<arrow::FieldVector>::Map(M &&) &&': could not deduce template argument for 'M'
C:\projects\arrow\cpp\src\arrow/result.h(380): note: see declaration of 'arrow::Result<arrow::FieldVector>::Map'
C:/projects/arrow/cpp/src/arrow/engine/substrait/type_internal.cc(80): error C2530: '_error_or_value20': references must be initialized
C:/projects/arrow/cpp/src/arrow/engine/substrait/type_internal.cc(80): error C3536: '_error_or_value20': cannot be used before it is initialized
```
Would you mind fix lint first?
--
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] pitrou commented on pull request #36915: GH-36867: [C++] Add a struct_ and schema overload taking a vector of (name, type) pairs
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36915:
URL: https://github.com/apache/arrow/pull/36915#issuecomment-1671157942
CI failures are unrelated, will merge.
--
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] pitrou merged pull request #36915: GH-36867: [C++] Add a struct_ and schema overload taking a vector of (name, type) pairs
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #36915:
URL: https://github.com/apache/arrow/pull/36915
--
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] jsjtxietian commented on pull request #36915: GH-36867: [C++] Add a struct_ and schema overload taking a vector of (name, type) pairs
Posted by "jsjtxietian (via GitHub)" <gi...@apache.org>.
jsjtxietian commented on PR #36915:
URL: https://github.com/apache/arrow/pull/36915#issuecomment-1656015254
> ```
> C:\projects\arrow\cpp\build\src\arrow\engine\CMakeFiles\arrow_substrait_shared.dir\Unity\unity_1_cxx.cxx
> C:/projects/arrow/cpp/src/arrow/engine/substrait/type_internal.cc(80): error C2672: 'arrow::Result<arrow::FieldVector>::Map': no matching overloaded function found
> C:/projects/arrow/cpp/src/arrow/engine/substrait/type_internal.cc(164): note: see reference to function template instantiation 'arrow::Result<arrow::FieldVector> arrow::engine::`anonymous-namespace'::FieldsFromProto<google::protobuf::RepeatedPtrField<substrait::Type>,arrow::engine::FromProto::<lambda_998f5b636b6257cbec583f504695a06c>>(int,const Types &,const NextName &,const arrow::engine::ExtensionSet &,const arrow::engine::ConversionOptions &)' being compiled
> with
> [
> Types=google::protobuf::RepeatedPtrField<substrait::Type>,
> NextName=arrow::engine::FromProto::<lambda_998f5b636b6257cbec583f504695a06c>
> ]
> C:/projects/arrow/cpp/src/arrow/engine/substrait/type_internal.cc(80): error C2783: 'EnsureResult<unknown-type>::type arrow::Result<arrow::FieldVector>::Map(M &&) const &': could not deduce template argument for 'M'
> C:\projects\arrow\cpp\src\arrow/result.h(392): note: see declaration of 'arrow::Result<arrow::FieldVector>::Map'
> C:/projects/arrow/cpp/src/arrow/engine/substrait/type_internal.cc(80): error C2783: 'EnsureResult<unknown-type>::type arrow::Result<arrow::FieldVector>::Map(M &&) &&': could not deduce template argument for 'M'
> C:\projects\arrow\cpp\src\arrow/result.h(380): note: see declaration of 'arrow::Result<arrow::FieldVector>::Map'
> C:/projects/arrow/cpp/src/arrow/engine/substrait/type_internal.cc(80): error C2530: '_error_or_value20': references must be initialized
> C:/projects/arrow/cpp/src/arrow/engine/substrait/type_internal.cc(80): error C3536: '_error_or_value20': cannot be used before it is initialized
> ```
>
@mapleFU The error here doesn't seem related to my commit, need more investigation.
--
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] pitrou commented on a diff in pull request #36915: GH-36867: [C++] Add a struct_ and schema overload taking a vector of (name, type) pairs
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #36915:
URL: https://github.com/apache/arrow/pull/36915#discussion_r1287364052
##########
cpp/src/arrow/type.cc:
##########
@@ -2090,17 +2090,39 @@ Status SchemaBuilder::AreCompatible(const std::vector<std::shared_ptr<Schema>>&
return Merge(schemas, policy).status();
}
+std::vector<std::shared_ptr<Field>> make_fields(
+ std::initializer_list<std::pair<std::string, std::shared_ptr<DataType>>> init_list) {
+ std::vector<std::shared_ptr<Field>> fields;
+ fields.reserve(init_list.size());
+ for (const auto& [name, type] : init_list) {
+ fields.push_back(field(name, type));
+ }
Review Comment:
I see, thank you @bkietz !
Well, let's not overdo this and keep the const-ref instead.
--
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