westonpace commented on code in PR #14024:
URL: https://github.com/apache/arrow/pull/14024#discussion_r963799075
##########
cpp/src/arrow/engine/substrait/serde_test.cc:
##########
@@ -1000,15 +1000,17 @@ TEST(Substrait, GetRecordBatchReader) {
GTEST_SKIP() << "ARROW-16392: Substrait File URI not supported for Windows";
#else
ASSERT_OK_AND_ASSIGN(std::string substrait_json, GetSubstraitJSON());
- test_with_registries([&substrait_json](ExtensionIdRegistry* ext_id_reg,
- compute::FunctionRegistry*
func_registry) {
- ASSERT_OK_AND_ASSIGN(auto buf, SerializeJsonPlan(substrait_json));
- ASSERT_OK_AND_ASSIGN(auto reader, ExecuteSerializedPlan(*buf));
- ASSERT_OK_AND_ASSIGN(auto table,
Table::FromRecordBatchReader(reader.get()));
- // Note: assuming the binary.parquet file contains fixed amount of records
- // in case of a test failure, re-evalaute the content in the file
- EXPECT_EQ(table->num_rows(), 12);
- });
+ PythonTableProvider table_provider;
+ test_with_registries(
+ [&substrait_json, table_provider](ExtensionIdRegistry* ext_id_reg,
+ compute::FunctionRegistry*
func_registry) {
+ ASSERT_OK_AND_ASSIGN(auto buf, SerializeJsonPlan(substrait_json));
+ ASSERT_OK_AND_ASSIGN(auto reader, ExecuteSerializedPlan(*buf,
table_provider));
+ ASSERT_OK_AND_ASSIGN(auto table,
Table::FromRecordBatchReader(reader.get()));
+ // Note: assuming the binary.parquet file contains fixed amount of
records
+ // in case of a test failure, re-evalaute the content in the file
+ EXPECT_EQ(table->num_rows(), 12);
+ });
Review Comment:
```suggestion
test_with_registries(
[&substrait_json](ExtensionIdRegistry* ext_id_reg,
compute::FunctionRegistry* func_registry) {
ASSERT_OK_AND_ASSIGN(auto buf, SerializeJsonPlan(substrait_json));
ASSERT_OK_AND_ASSIGN(auto reader, ExecuteSerializedPlan(*buf, {}));
ASSERT_OK_AND_ASSIGN(auto table,
Table::FromRecordBatchReader(reader.get()));
// Note: assuming the binary.parquet file contains fixed amount of
records
// in case of a test failure, re-evalaute the content in the file
EXPECT_EQ(table->num_rows(), 12);
});
```
Although...I don't think this is the best suggestion. We should make
`table_provider` an optional argument to `ExecuteSerializedPlan`.
--
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]
