pitrou commented on code in PR #47391:
URL: https://github.com/apache/arrow/pull/47391#discussion_r3850976660
##########
cpp/src/arrow/acero/pivot_longer_node.cc:
##########
@@ -103,11 +105,28 @@ class PivotLongerNode : public ExecNode, public
TracedNode {
" measurement columns but one of the row templates only had ",
row_template.measurement_values.size(), " field references");
}
+
+ for (std::size_t i = 0; i < row_template.feature_values.size(); i++) {
+ if (feature_types[i]) {
+ if (!feature_types[i]->Equals(row_template.feature_values[i]->type))
{
+ return Status::Invalid(
Review Comment:
Perhaps we want to make this `TypeError`?
##########
cpp/src/arrow/acero/options.h:
##########
@@ -780,15 +780,15 @@ class ARROW_ACERO_EXPORT TableSinkNodeOptions : public
ExecNodeOptions {
/// \brief a row template that describes one row that will be generated for
each input row
struct ARROW_ACERO_EXPORT PivotLongerRowTemplate {
- PivotLongerRowTemplate(std::vector<std::string> feature_values,
+ PivotLongerRowTemplate(std::vector<std::shared_ptr<Scalar>> feature_values,
std::vector<std::optional<FieldRef>>
measurement_values)
: feature_values(std::move(feature_values)),
measurement_values(std::move(measurement_values)) {}
Review Comment:
Can we also keep the legacy constructor to avoid breaking the API?
##########
cpp/src/arrow/acero/pivot_longer_node_test.cc:
##########
@@ -43,9 +43,15 @@ TEST(PivotLongerNode, Basic) {
->Table(kRowsPerBatch, kNumBatches);
PivotLongerNodeOptions options;
- options.feature_field_names = {"feature1", "feature2"};
+ options.feature_field_names = {"feature1", "feature2", "feature3"};
options.measurement_field_names = {"meas1", "meas2"};
- options.row_templates = {{{"a", "x"}, {{1}, {3}}}, {{"b", "y"}, {{2},
std::nullopt}}};
+ options.row_templates = {
+ {{std::make_shared<StringScalar>("a"),
std::make_shared<StringScalar>("x"),
+ std::make_shared<UInt32Scalar>(12)},
+ {{1}, {3}}},
+ {{std::make_shared<StringScalar>("b"),
std::make_shared<StringScalar>("y"),
+ std::make_shared<UInt32Scalar>(13)},
+ {{2}, std::nullopt}}};
Review Comment:
It would be better if the unchanged spelling would still work.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
