westonpace commented on code in PR #15097:
URL: https://github.com/apache/arrow/pull/15097#discussion_r1061142323
##########
cpp/src/arrow/engine/substrait/serde_test.cc:
##########
@@ -216,6 +216,20 @@ void CheckRoundTripResult(const std::shared_ptr<Table>
expected_table,
compute::AssertTablesEqualIgnoringOrder(merged_expected, output_table);
}
+void countProjectNodeOptionsInDeclarations(const compute::Declaration& input,
+ int& counter) {
Review Comment:
Can you change this to just return an `int` instead of taking an out
parameter?
##########
cpp/src/arrow/engine/substrait/serde_test.cc:
##########
@@ -216,6 +216,20 @@ void CheckRoundTripResult(const std::shared_ptr<Table>
expected_table,
compute::AssertTablesEqualIgnoringOrder(merged_expected, output_table);
}
+void countProjectNodeOptionsInDeclarations(const compute::Declaration& input,
+ int& counter) {
+ const auto& options = input.options;
+ const auto& inputs = input.inputs;
+ const auto* maybe_project_option =
+ dynamic_cast<const compute::ProjectNodeOptions*>(options.get());
+ if (maybe_project_option != NULL) {
+ counter++;
+ }
+ for (const auto& in : inputs) {
+ countProjectNodeOptionsInDeclarations(std::get<compute::Declaration>(in),
counter);
Review Comment:
```suggestion
CountProjectNodeOptionsInDeclarations(std::get<compute::Declaration>(in),
counter);
```
##########
cpp/src/arrow/engine/substrait/serde_test.cc:
##########
@@ -2648,21 +2744,32 @@ TEST(SubstraitRoundTrip,
ProjectRelOnFunctionWithAllEmit) {
ASSERT_OK_AND_ASSIGN(auto buf,
internal::SubstraitFromJSON("Plan", substrait_json,
/*ignore_unknown_fields=*/false));
- auto output_schema =
- schema({field("A", int32()), field("B", int32()), field("equal",
boolean())});
+ auto output_schema = schema({field("A", int32()), field("B", int32()),
+ field("eq1", boolean()), field("eq2",
boolean())});
auto expected_table = TableFromJSON(output_schema, {R"([
- [1, 1, true],
- [3, 5, false],
- [4, 1, false],
- [2, 1, false],
- [5, 5, true],
- [2, 2, true]
+ [1, 1, true, true],
+ [3, 5, false, false],
+ [4, 1, false, false],
+ [2, 1, false, false],
+ [5, 5, true, true],
+ [2, 2, true, true]
])"});
NamedTableProvider table_provider =
AlwaysProvideSameTable(std::move(input_table));
ConversionOptions conversion_options;
conversion_options.named_table_provider = std::move(table_provider);
+ // evaluate the plan
+ std::shared_ptr<ExtensionIdRegistry> sp_ext_id_reg =
MakeExtensionIdRegistry();
+ ExtensionIdRegistry* ext_id_reg = sp_ext_id_reg.get();
+ ExtensionSet ext_set(ext_id_reg);
+ ASSERT_OK_AND_ASSIGN(auto sink_decls, DeserializePlans(
+ *buf, [] { return kNullConsumer; },
+ ext_id_reg, &ext_set,
conversion_options));
+ auto& other_declrs = std::get<compute::Declaration>(sink_decls[0].inputs[0]);
+ int num_projections = 0;
+ countProjectNodeOptionsInDeclarations(other_declrs, num_projections);
+ ASSERT_EQ(num_projections, 2);
Review Comment:
```suggestion
int num_projections = CountProjectNodeOptionsInDeclarations(other_declrs);
ASSERT_EQ(num_projections, 2);
```
##########
cpp/src/arrow/engine/substrait/serde_test.cc:
##########
@@ -216,6 +216,20 @@ void CheckRoundTripResult(const std::shared_ptr<Table>
expected_table,
compute::AssertTablesEqualIgnoringOrder(merged_expected, output_table);
}
+void countProjectNodeOptionsInDeclarations(const compute::Declaration& input,
Review Comment:
```suggestion
void CountProjectNodeOptionsInDeclarations(const compute::Declaration& input,
```
##########
cpp/src/arrow/engine/substrait/serde_test.cc:
##########
@@ -216,6 +216,20 @@ void CheckRoundTripResult(const std::shared_ptr<Table>
expected_table,
compute::AssertTablesEqualIgnoringOrder(merged_expected, output_table);
}
+void countProjectNodeOptionsInDeclarations(const compute::Declaration& input,
+ int& counter) {
+ const auto& options = input.options;
+ const auto& inputs = input.inputs;
+ const auto* maybe_project_option =
+ dynamic_cast<const compute::ProjectNodeOptions*>(options.get());
+ if (maybe_project_option != NULL) {
+ counter++;
+ }
Review Comment:
```suggestion
if (input.factory_name == "project") {
counter++;
}
const auto& inputs = input.inputs;
```
Isn't it easier to just look at the name?
--
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]
