[GitHub] [arrow] pitrou commented on a diff in pull request #15196: GH-15195: [C++][FlightRPC][Python] Add ToString/Equals for Flight types

[GitBox <gi...@apache.org>]

pitrou commented on code in PR #15196:
URL: https://github.com/apache/arrow/pull/15196#discussion_r1072484483


##########
cpp/src/arrow/flight/types.cc:
##########
@@ -638,6 +726,11 @@ arrow::Result<std::unique_ptr<Result>> SimpleResultStream::Next() {
   return std::make_unique<Result>(std::move(results_[position_++]));
 }
 
+std::string BasicAuth::ToString() const {
+  return arrow::util::StringBuilder("BasicAuth<username = '", username, "', password = '",
+                                    password, "'>");

Review Comment:
   It's dangerous to include the password here, as it could leak unwillingly through logging or other means.
   



##########
cpp/src/arrow/flight/flight_internals_test.cc:
##########
@@ -40,40 +42,250 @@ namespace pb = arrow::flight::protocol;
 // ----------------------------------------------------------------------
 // Core Flight types
 
-TEST(FlightTypes, FlightDescriptor) {
-  auto a = FlightDescriptor::Command("select * from table");
-  auto b = FlightDescriptor::Command("select * from table");
-  auto c = FlightDescriptor::Command("select foo from table");
-  auto d = FlightDescriptor::Path({"foo", "bar"});
-  auto e = FlightDescriptor::Path({"foo", "baz"});
-  auto f = FlightDescriptor::Path({"foo", "baz"});
-
-  ASSERT_EQ(a.ToString(), "FlightDescriptor<cmd = 'select * from table'>");
-  ASSERT_EQ(d.ToString(), "FlightDescriptor<path = 'foo/bar'>");
-  ASSERT_TRUE(a.Equals(b));
-  ASSERT_FALSE(a.Equals(c));
-  ASSERT_FALSE(a.Equals(d));
-  ASSERT_FALSE(d.Equals(e));
-  ASSERT_TRUE(e.Equals(f));
-}
+template <typename PbType, typename FlightType>
+void TestRoundtrip(const std::vector<FlightType>& values,
+                   const std::vector<std::string>& reprs) {
+  for (size_t i = 0; i < values.size(); i++) {
+    ARROW_SCOPED_TRACE("LHS = ", values[i].ToString());
+    for (size_t j = 0; j < values.size(); j++) {
+      ARROW_SCOPED_TRACE("RHS = ", values[j].ToString());
+      if (i == j) {
+        EXPECT_EQ(values[i], values[j]);
+        EXPECT_TRUE(values[i].Equals(values[j]));
+      } else {
+        EXPECT_NE(values[i], values[j]);
+        EXPECT_FALSE(values[i].Equals(values[j]));
+      }
+    }
+    EXPECT_EQ(values[i].ToString(), reprs[i]);
+
+    ASSERT_OK_AND_ASSIGN(std::string serialized, values[i].SerializeToString());
+    ASSERT_OK_AND_ASSIGN(auto deserialized, FlightType::Deserialize(serialized));
+    if constexpr (std::is_same_v<FlightType, FlightInfo>) {
+      EXPECT_EQ(values[i], *deserialized);
+    } else {
+      EXPECT_EQ(values[i], deserialized);
+    }
 
 // This tests the internal protobuf types which don't get exported in the Flight DLL.
 #ifndef _WIN32
-TEST(FlightTypes, FlightDescriptorToFromProto) {
-  FlightDescriptor descr_test;
-  pb::FlightDescriptor pb_descr;
-
-  FlightDescriptor descr1{FlightDescriptor::PATH, "", {"foo", "bar"}};
-  ASSERT_OK(internal::ToProto(descr1, &pb_descr));
-  ASSERT_OK(internal::FromProto(pb_descr, &descr_test));
-  ASSERT_EQ(descr1, descr_test);
-
-  FlightDescriptor descr2{FlightDescriptor::CMD, "command", {}};
-  ASSERT_OK(internal::ToProto(descr2, &pb_descr));
-  ASSERT_OK(internal::FromProto(pb_descr, &descr_test));
-  ASSERT_EQ(descr2, descr_test);
-}
+    PbType pb_value;
+    ASSERT_OK(internal::ToProto(values[i], &pb_value));

Review Comment:
   Unrelated to this PR: if we'd like to test this on Windows, I wonder if we could have wrapper APIs around ToProto that would return `std::any`.
   
   



-- 
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