Copilot commented on code in PR #50061:
URL: https://github.com/apache/arrow/pull/50061#discussion_r3330232785
##########
cpp/src/arrow/io/interfaces.h:
##########
@@ -318,18 +318,21 @@ class ARROW_EXPORT RandomAccessFile : public InputStream,
public Seekable {
/// \param[in] position Where to read bytes from
/// \param[in] nbytes The number of bytes to read
/// \return A buffer containing the bytes read, or an error
+ ARROW_DEPRECATED("Deprecated in 17.0.0. Use signature with allow_short_read
instead")
virtual Result<std::shared_ptr<Buffer>> ReadAt(int64_t position, int64_t
nbytes);
/// EXPERIMENTAL: Read data asynchronously.
virtual Future<std::shared_ptr<Buffer>> ReadAsync(const IOContext&, int64_t
position,
int64_t nbytes,
bool allow_short_read);
+ ARROW_DEPRECATED("Deprecated in 17.0.0. Use signature with allow_short_read
instead")
virtual Future<std::shared_ptr<Buffer>> ReadAsync(const IOContext&, int64_t
position,
int64_t nbytes);
/// EXPERIMENTAL: Read data asynchronously, using the file's IOContext.
Future<std::shared_ptr<Buffer>> ReadAsync(int64_t position, int64_t nbytes,
bool allow_short_read);
+ ARROW_DEPRECATED("Deprecated in 17.0.0. Use signature with allow_short_read
instead")
Review Comment:
The deprecation message says "Deprecated in 17.0.0", but Arrow 17.0.0 was
released in July 2024 and the current Arrow version on `main` is
25.0.0-SNAPSHOT (see `cpp/CMakeLists.txt:99`). The PR description likewise
states "scheduling them for removal in version 17.0.0", which doesn't match the
issue's intent of giving callers a few releases to migrate. Other deprecations
in the codebase use the version in which the deprecation was introduced (e.g.
`cpp/src/arrow/config.h:101` "Deprecated in 24.0.0",
`cpp/src/arrow/flight/server_auth.h:90` "Deprecated in 13.0.0"). Please update
all four `ARROW_DEPRECATED` strings (and any matching `\deprecated` doc tag) to
the upcoming release (e.g. 25.0.0).
##########
cpp/src/arrow/ipc/message.cc:
##########
@@ -305,7 +305,7 @@ Status ReadFieldsSubset(int64_t offset, int32_t
metadata_length,
RETURN_NOT_OK(fields_loader(batch, &io_recorded_random_access_file));
const auto& read_ranges = io_recorded_random_access_file.GetReadRanges();
for (const auto& range : read_ranges) {
- auto read_result = file->ReadAt(offset + metadata_length + range.offset,
range.length,
+ auto read_result = file->ReadAt(offset + metadata_length + range.offset,
range.length,/*allow_short_read=*/true,
body->mutable_data() + range.offset);
Review Comment:
Whitespace around the inserted argument is malformed and will not pass
`clang-format`: missing space after the comma before
`/*allow_short_read=*/true` and a stray space before the comma after it. The
same issue is present in several other call sites updated in this PR
(`cpp/src/arrow/io/slow.cc:134` and `:146`,
`cpp/src/arrow/io/test_common.cc:136`, `cpp/src/arrow/io/interfaces.cc:284` and
`:293`, `cpp/src/arrow/buffer_test.cc:619`,
`cpp/src/arrow/adapters/orc/adapter.cc:116`). Please run `clang-format` on the
modified files.
##########
cpp/src/arrow/io/interfaces.h:
##########
@@ -295,6 +294,7 @@ class ARROW_EXPORT RandomAccessFile : public InputStream,
public Seekable {
/// \param[in] nbytes The number of bytes to read
/// \param[out] out The buffer to read bytes into
/// \return The number of bytes read, or an error
+ ARROW_DEPRECATED("Deprecated in 17.0.0. Use signature with allow_short_read
instead")
virtual Result<int64_t> ReadAt(int64_t position, int64_t nbytes, void* out);
Review Comment:
In addition to the existing comments above each deprecated method, the
codebase convention is to also add a `/// \deprecated Deprecated in <version>.
...` doxygen tag immediately above the `ARROW_DEPRECATED(...)` line so it is
rendered in the API docs (see e.g. `cpp/src/arrow/config.h:96-101`,
`cpp/src/arrow/flight/server_auth.h:88-90`,
`cpp/src/arrow/testing/util.h:115-118`). Consider adding `\deprecated` lines to
each of the four deprecated overloads here.
##########
cpp/src/arrow/io/interfaces.h:
##########
@@ -29,7 +29,6 @@
#include "arrow/util/macros.h"
#include "arrow/util/type_fwd.h"
#include "arrow/util/visibility.h"
-
namespace arrow {
Review Comment:
The blank line between the `#include` block and the `namespace arrow {`
declaration was removed. This is the conventional blank-line separator
throughout the codebase and clang-format will reinstate it; please restore the
blank line.
##########
cpp/src/arrow/io/slow.cc:
##########
@@ -131,7 +131,7 @@ Result<std::shared_ptr<Buffer>>
SlowRandomAccessFile::Read(int64_t nbytes) {
Result<int64_t> SlowRandomAccessFile::ReadAt(int64_t position, int64_t nbytes,
void* out) {
latencies_->Sleep();
- return stream_->ReadAt(position, nbytes, out);
+ return stream_->ReadAt(position, nbytes,/*allow_short_read=*/true ,out);
Review Comment:
Stray spaces before the commas: `nbytes , /*allow_short_read=*/true` does
not match the project's clang-format style. Please reformat.
##########
cpp/src/arrow/io/slow.cc:
##########
@@ -143,7 +143,7 @@ Result<int64_t> SlowRandomAccessFile::ReadAt(int64_t
position, int64_t nbytes,
Result<std::shared_ptr<Buffer>> SlowRandomAccessFile::ReadAt(int64_t position,
int64_t nbytes) {
latencies_->Sleep();
- return stream_->ReadAt(position, nbytes);
+ return stream_->ReadAt(position, nbytes , /*allow_short_read=*/true);
Review Comment:
Stray space before the comma. Please reformat with clang-format.
--
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]
