GH-15195: [C++][FlightRPC][Python] Add ToString/Equals for Flight types #15196
Conversation
lidavidm
commented
Jan 4, 2023
•
lidavidm
commented
- Closes: [C++][FlightRPC] Flight types should all implement ToString, Equals, etc. #15195
lidavidm
force-pushed
the
gh-15195-flight-refactor
branch
from
88963ee
to
57d4f8a
Compare
cpp/src/arrow/flight/types.cc
Outdated
| @@ -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, "'>"); | |||
There was a problem hiding this comment.
It's dangerous to include the password here, as it could leak unwillingly through logging or other means.
| ASSERT_EQ(descr2, descr_test); | ||
| } | ||
| PbType pb_value; | ||
| ASSERT_OK(internal::ToProto(values[i], &pb_value)); |
There was a problem hiding this 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.
|
@jorisvandenbossche Do you want to take a look at the Python changes? |
lidavidm
force-pushed
the
gh-15195-flight-refactor
branch
from
57d4f8a
to
fefbe9e
Compare
lidavidm
force-pushed
the
gh-15195-flight-refactor
branch
from
fefbe9e
to
31d868f
Compare
python/pyarrow/_flight.pyx
Outdated
| return "<FlightDescriptor command: {!r}>".format(self.command) | ||
| else: | ||
| return "<FlightDescriptor type: {!r}>".format(self.descriptor_type) | ||
| return frombytes(self.descriptor.ToString()) |
There was a problem hiding this comment.
I don't know how consistent we want to be on this front, but so in another recent PR that improved some reprs for Buffer et al objects (#13921), we did keep the <...> around it.
We could easily do that here as well, but the output of ToString already uses that inside the text repr, so that would give double >>'s ?
There was a problem hiding this comment.
We also added "pyarrow." in front of the class names. For those flight specific classes, that might be less needed (in comparison to the generic "Buffer", for which it is useful to know it is a pyarrow Buffer), and would also get lengthier ("pyarrow.flight.Action ..")?
There was a problem hiding this comment.
Sorry, I've been distracted as of late. I can rework the Python parts of this so that we have custom Pythonic __repr__ implementations. I originally did the Python changes here just because I figured I'd reduce the duplication while I was already rooting around here, but it's be better to give a good experience for Python users.
lidavidm
force-pushed
the
gh-15195-flight-refactor
branch
from
31d868f
to
509d292
Compare
|
|
Thanks, those reprs look good! |
|
Benchmark runs are scheduled for baseline = 67fa416 and contender = 6a0fb7a. 6a0fb7a is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
…ht types (apache#15196) * Closes: apache#15195 Authored-by: David Li <li.davidm96@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
