This is an automated email from the ASF dual-hosted git repository.
gavinchou pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new 3a83360e4f8 [fix](cloud) Get packed slice location through the
FileWriter interface (#68106)
3a83360e4f8 is described below
commit 3a83360e4f829cf2910a89108ca57ee290daf87e
Author: Xin Liao <[email protected]>
AuthorDate: Thu Oct 15 20:48:43 2026 +0800
[fix](cloud) Get packed slice location through the FileWriter interface
(#68106)
#67347 changed `CloudRowsetWriter::_collect_packed_slice_location()` to
read the slice
location from the writer instead of `PackedFileManager`:
```cpp
if (!file_writer->is_in_packed_file()) {
return Status::OK();
}
...
static_cast<io::PackedFileWriter*>(file_writer)->get_packed_slice_location(&index);
```
`is_in_packed_file()` is a virtual method of `FileWriter`, and a writer
that wraps a
`PackedFileWriter` (for example an encryption writer layered on top of
`PackedFileSystem`)
has to forward it so that callers still see that the data went into a
packed file. For
such a writer the check passes but the object is not a
`PackedFileWriter`, so the
`static_cast` is undefined behavior:
```
cloud_rowset_writer.cpp:218:5: runtime error: downcast of address
0x7c8be5b61100 which does not point to an object of type 'io::PackedFileWriter'
note: object is of type 'doris::io::EncryptedFileWriter'
F packed_file_writer.cpp:236] Check failed: _state == State::CLOSED
```
Debug/ASAN builds abort on the `DCHECK`. Release builds read unrelated
fields as
`_is_direct_write` / `_packed_slice_handle`, which may crash or record a
bogus packed slice
location in the rowset meta.
### What is changed and how it works?
`get_packed_slice_location()` becomes a virtual method of `FileWriter`
and replaces
`is_in_packed_file()`:
- The two methods described the same fact and had the same "after close"
precondition, so a
wrapper had to forward both. Forwarding only one of them either failed
or, worse, made the
caller skip the file silently. Now there is a single method, and an
empty
`packed_file_path` means the file is not in a packed file.
- The default implementation returns an empty location.
`PackedFileWriter` overrides it with
its existing implementation (empty for direct write), and a wrapper
forwards it to its
inner writer.
- `CloudRowsetWriter` and `DeleteBitmapFileWriter` call it through
`FileWriter` instead of
downcasting. The lifetime fix of #67347 is kept: the location is still
taken from the
writer's own slice handle, not looked up by path.
- `PackedSliceLocation` moves to `io/fs/packed_slice_location.h` so
`file_writer.h` does not
need to include `packed_file_manager.h`.
---
be/src/cloud/cloud_rowset_writer.cpp | 11 ++-------
be/src/cloud/delete_bitmap_file_writer.cpp | 3 +--
be/src/io/fs/file_writer.h | 12 ++++++---
be/src/io/fs/packed_file_manager.h | 13 +---------
be/src/io/fs/packed_file_writer.h | 5 +---
be/src/io/fs/packed_slice_location.h | 39 ++++++++++++++++++++++++++++++
be/test/io/fs/packed_file_system_test.cpp | 13 +++++-----
be/test/io/fs/packed_file_writer_test.cpp | 23 ++++++++++++++++++
8 files changed, 83 insertions(+), 36 deletions(-)
diff --git a/be/src/cloud/cloud_rowset_writer.cpp
b/be/src/cloud/cloud_rowset_writer.cpp
index 20aa151a341..03fc3ca6682 100644
--- a/be/src/cloud/cloud_rowset_writer.cpp
+++ b/be/src/cloud/cloud_rowset_writer.cpp
@@ -21,7 +21,6 @@
#include "common/status.h"
#include "io/cache/block_file_cache_factory.h"
#include "io/fs/packed_file_manager.h"
-#include "io/fs/packed_file_writer.h"
#include "storage/rowset/rowset_factory.h"
namespace doris {
@@ -219,18 +218,12 @@ Status
CloudRowsetWriter::_collect_packed_slice_location(io::FileWriter* file_wr
return Status::OK();
}
- // Check if file is actually in packed file (not direct write for large
files)
- if (!file_writer->is_in_packed_file()) {
- return Status::OK();
- }
-
// Ask the writer, which holds a reference to its own slice location.
Looking it up by
// path in PackedFileManager would race with the retention based cleanup
of the index.
io::PackedSliceLocation index;
- RETURN_IF_ERROR(
-
static_cast<io::PackedFileWriter*>(file_writer)->get_packed_slice_location(&index));
+ RETURN_IF_ERROR(file_writer->get_packed_slice_location(&index));
if (index.packed_file_path.empty()) {
- return Status::OK(); // File not in packed file, skip
+ return Status::OK(); // File not in packed file (e.g. direct write for
large files), skip
}
rowset_meta->add_packed_slice_location(file_path, index.packed_file_path,
index.offset,
diff --git a/be/src/cloud/delete_bitmap_file_writer.cpp
b/be/src/cloud/delete_bitmap_file_writer.cpp
index d137da5efec..81714ce5559 100644
--- a/be/src/cloud/delete_bitmap_file_writer.cpp
+++ b/be/src/cloud/delete_bitmap_file_writer.cpp
@@ -97,9 +97,8 @@ Status DeleteBitmapFileWriter::close() {
// Check if file was written to packed file
if (_enable_packed_file) {
- auto* packed_writer =
static_cast<io::PackedFileWriter*>(_file_writer.get());
io::PackedSliceLocation loc;
- st = packed_writer->get_packed_slice_location(&loc);
+ st = _file_writer->get_packed_slice_location(&loc);
if (!st.ok()) {
LOG(WARNING) << "failed to get packed slice location for delete
bitmap file=" << _path
<< ", st=" << st.to_string();
diff --git a/be/src/io/fs/file_writer.h b/be/src/io/fs/file_writer.h
index 92e4475d637..1ab0d14745b 100644
--- a/be/src/io/fs/file_writer.h
+++ b/be/src/io/fs/file_writer.h
@@ -25,6 +25,7 @@
#include "io/cache/block_file_cache_factory.h"
#include "io/cache/file_cache_common.h"
#include "io/fs/file_reader_writer_fwd.h"
+#include "io/fs/packed_slice_location.h"
#include "io/fs/path.h"
#include "util/slice.h"
@@ -93,9 +94,14 @@ public:
virtual State state() const = 0;
- // Returns true if this file's data was written to a packed file.
- // Used to determine whether to collect packed slice location from
PackedFileManager.
- virtual bool is_in_packed_file() const { return false; }
+ // Gets the location of this file's slice in the packed file. Must be
called after the writer
+ // is closed. An empty packed_file_path means the file is not in a packed
file. Writers that
+ // wrap another writer must forward this call, so callers never need to
downcast to
+ // PackedFileWriter.
+ virtual Status get_packed_slice_location(PackedSliceLocation* location)
const {
+ *location = PackedSliceLocation {};
+ return Status::OK();
+ }
FileCacheAllocatorBuilder* cache_builder() const {
return _cache_builder == nullptr ? nullptr : _cache_builder.get();
diff --git a/be/src/io/fs/packed_file_manager.h
b/be/src/io/fs/packed_file_manager.h
index 9cc76eb803e..de9ebf89df7 100644
--- a/be/src/io/fs/packed_file_manager.h
+++ b/be/src/io/fs/packed_file_manager.h
@@ -36,23 +36,12 @@
#include "common/status.h"
#include "io/fs/file_system.h"
#include "io/fs/file_writer.h"
+#include "io/fs/packed_slice_location.h"
#include "io/fs/path.h"
#include "util/slice.h"
namespace doris::io {
-struct PackedSliceLocation {
- std::string packed_file_path;
- int64_t offset;
- int64_t size;
- int64_t create_time = 0;
- int64_t tablet_id = 0;
- std::string rowset_id;
- std::string resource_id;
- int64_t txn_id = 0;
- int64_t packed_file_size = -1; // Total size of the packed file, -1 means
not set
-};
-
// Upload state of the packed file a slice belongs to
enum class PackedSliceUploadState : uint8_t {
PENDING = 0,
diff --git a/be/src/io/fs/packed_file_writer.h
b/be/src/io/fs/packed_file_writer.h
index a53d412e667..f571b188acb 100644
--- a/be/src/io/fs/packed_file_writer.h
+++ b/be/src/io/fs/packed_file_writer.h
@@ -55,10 +55,7 @@ public:
// Get merge file segment index information
// This method should be called after close(true) to get the index
information
// Returns empty index if file is not in merge file
- Status get_packed_slice_location(PackedSliceLocation* location) const;
-
- // Returns true if this file's data was written to a packed file (not
direct write)
- bool is_in_packed_file() const override { return !_is_direct_write; }
+ Status get_packed_slice_location(PackedSliceLocation* location) const
override;
#ifdef BE_TEST
size_t buffer_capacity_for_test() const { return _buffer.capacity(); }
diff --git a/be/src/io/fs/packed_slice_location.h
b/be/src/io/fs/packed_slice_location.h
new file mode 100644
index 00000000000..a6d9e48a49e
--- /dev/null
+++ b/be/src/io/fs/packed_slice_location.h
@@ -0,0 +1,39 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstdint>
+#include <string>
+
+namespace doris::io {
+
+// Location of a small file's slice inside a packed file.
+// An empty packed_file_path means the file is not in a packed file.
+struct PackedSliceLocation {
+ std::string packed_file_path;
+ int64_t offset;
+ int64_t size;
+ int64_t create_time = 0;
+ int64_t tablet_id = 0;
+ std::string rowset_id;
+ std::string resource_id;
+ int64_t txn_id = 0;
+ int64_t packed_file_size = -1; // Total size of the packed file, -1 means
not set
+};
+
+} // namespace doris::io
diff --git a/be/test/io/fs/packed_file_system_test.cpp
b/be/test/io/fs/packed_file_system_test.cpp
index 3ead07b5610..b2007941ab7 100644
--- a/be/test/io/fs/packed_file_system_test.cpp
+++ b/be/test/io/fs/packed_file_system_test.cpp
@@ -30,6 +30,7 @@
#include "io/fs/file_reader.h"
#include "io/fs/file_system.h"
#include "io/fs/file_writer.h"
+#include "io/fs/packed_file_writer.h"
#include "io/fs/path.h"
#include "util/slice.h"
@@ -247,7 +248,7 @@ TEST_F(PackedFileSystemTest,
FirstSegmentDataFileUsesPackedWriter) {
ASSERT_NE(_inner_fs->last_writer(), nullptr);
EXPECT_EQ(_inner_fs->last_writer()->bytes_appended(), 0);
- EXPECT_TRUE(writer->is_in_packed_file());
+ EXPECT_NE(dynamic_cast<PackedFileWriter*>(writer.get()), nullptr);
}
TEST_F(PackedFileSystemTest, LaterSegmentDataFileUsesDirectWriter) {
@@ -264,7 +265,7 @@ TEST_F(PackedFileSystemTest,
LaterSegmentDataFileUsesDirectWriter) {
ASSERT_NE(_inner_fs->last_writer(), nullptr);
EXPECT_EQ(_inner_fs->last_writer()->bytes_appended(), data.size());
- EXPECT_FALSE(writer->is_in_packed_file());
+ EXPECT_EQ(dynamic_cast<PackedFileWriter*>(writer.get()), nullptr);
}
TEST_F(PackedFileSystemTest, LaterSegmentIndexFileUsesDirectWriter) {
@@ -281,7 +282,7 @@ TEST_F(PackedFileSystemTest,
LaterSegmentIndexFileUsesDirectWriter) {
ASSERT_NE(_inner_fs->last_writer(), nullptr);
EXPECT_EQ(_inner_fs->last_writer()->bytes_appended(), data.size());
- EXPECT_FALSE(writer->is_in_packed_file());
+ EXPECT_EQ(dynamic_cast<PackedFileWriter*>(writer.get()), nullptr);
}
TEST_F(PackedFileSystemTest, NonzeroFirstSegmentUsesPackedWriter) {
@@ -298,7 +299,7 @@ TEST_F(PackedFileSystemTest,
NonzeroFirstSegmentUsesPackedWriter) {
ASSERT_TRUE(first_segment_writer->appendv(&data_slice, 1).ok());
ASSERT_NE(_inner_fs->last_writer(), nullptr);
EXPECT_EQ(_inner_fs->last_writer()->bytes_appended(), 0);
- EXPECT_TRUE(first_segment_writer->is_in_packed_file());
+ EXPECT_NE(dynamic_cast<PackedFileWriter*>(first_segment_writer.get()),
nullptr);
Path segment_zero_path("rowset_1_0.dat");
FileWriterPtr segment_zero_writer;
@@ -307,7 +308,7 @@ TEST_F(PackedFileSystemTest,
NonzeroFirstSegmentUsesPackedWriter) {
ASSERT_TRUE(segment_zero_writer->appendv(&data_slice, 1).ok());
ASSERT_NE(_inner_fs->last_writer(), nullptr);
EXPECT_EQ(_inner_fs->last_writer()->bytes_appended(), data.size());
- EXPECT_FALSE(segment_zero_writer->is_in_packed_file());
+ EXPECT_EQ(dynamic_cast<PackedFileWriter*>(segment_zero_writer.get()),
nullptr);
Path first_segment_index_path("rowset_1_10.idx");
FileWriterPtr first_segment_index_writer;
@@ -320,7 +321,7 @@ TEST_F(PackedFileSystemTest,
NonzeroFirstSegmentUsesPackedWriter) {
ASSERT_TRUE(first_segment_index_writer->appendv(&index_slice, 1).ok());
ASSERT_NE(_inner_fs->last_writer(), nullptr);
EXPECT_EQ(_inner_fs->last_writer()->bytes_appended(), 0);
- EXPECT_TRUE(first_segment_index_writer->is_in_packed_file());
+
EXPECT_NE(dynamic_cast<PackedFileWriter*>(first_segment_index_writer.get()),
nullptr);
}
TEST_F(PackedFileSystemTest, OpenFileNotInMergeFile) {
diff --git a/be/test/io/fs/packed_file_writer_test.cpp
b/be/test/io/fs/packed_file_writer_test.cpp
index e26a5ec4743..d5b7dc2da79 100644
--- a/be/test/io/fs/packed_file_writer_test.cpp
+++ b/be/test/io/fs/packed_file_writer_test.cpp
@@ -275,4 +275,27 @@ TEST_F(PackedFileWriterTest, EmptyAppend) {
EXPECT_EQ(writer.bytes_appended(), 0);
}
+TEST_F(PackedFileWriterTest, GetPackedSliceLocationThroughFileWriterInterface)
{
+ // Callers only hold a FileWriter, which may be a wrapper around
PackedFileWriter,
+ // so the location must be reachable through the virtual interface.
+ MockFileWriterForMerge plain_writer("plain_file");
+ ASSERT_TRUE(plain_writer.close(false).ok());
+ FileWriter* plain = &plain_writer;
+ PackedSliceLocation location;
+ location.packed_file_path = "stale";
+ ASSERT_TRUE(plain->get_packed_slice_location(&location).ok());
+ EXPECT_TRUE(location.packed_file_path.empty());
+
+ PackedFileWriter packed_writer(std::move(_inner_writer),
Path("large_file"), _append_info);
+ FileWriter* writer = &packed_writer;
+ std::string data(150, 'x');
+ ASSERT_TRUE(writer->append(Slice(data)).ok());
+ ASSERT_TRUE(writer->close(false).ok());
+ ASSERT_EQ(writer->state(), FileWriter::State::CLOSED);
+
+ location.packed_file_path = "stale";
+ ASSERT_TRUE(writer->get_packed_slice_location(&location).ok());
+ EXPECT_TRUE(location.packed_file_path.empty());
+}
+
} // namespace doris::io
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
