This is an automated email from the ASF dual-hosted git repository.
yiguolei 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 88f22fd875e [refactor](outfile) Inline file name formatting and add
regression coverage (#69189)
88f22fd875e is described below
commit 88f22fd875e0be4dea2c58c53744f4074378a870
Author: Gabriel <[email protected]>
AuthorDate: Fri Oct 16 23:31:58 2026 +0800
[refactor](outfile) Inline file name formatting and add regression coverage
(#69189)
### What problem does this PR solve?
Follow-up to #68746. Remove `result_file_name.h` and inline its single
formatting expression at both call sites: the OUTFILE and TVF writers.
File naming behavior is unchanged.
Replace the helper-only unit test with an end-to-end regression covering
both writers, CSV/Parquet/ORC, default and explicit padding widths,
invalid values, custom OUTFILE suffixes, and rollover beyond index 9.
The test checks actual object names, verifies minimum-width semantics
and lexical ordering, reads the exported data back, and cleans up its
unique object prefix.
### Validation
- The new `test_result_file_name_padding` regression passed locally
against a master build containing #68746. A mutation that forces padding
to zero fails the filename assertion.
- Both inlined C++ formatting expressions passed the five original
boundary scenarios in a standalone harness.
- clang-format 16 and `git diff --check` passed.
- The modified full BE binary has not been rebuilt locally; the new
regression's local run used the existing master binary. Full build and
regression validation will run in CI.
### Release note
None; no behavior change.
### Check List (For Author)
- [x] Regression test
- [x] No behavior change
- [x] No documentation change needed
---
be/src/exec/sink/writer/result_file_name.h | 39 --------
be/src/exec/sink/writer/vfile_result_writer.cpp | 8 +-
be/src/exec/sink/writer/vtvf_table_writer.cpp | 6 +-
be/test/exec/sink/writer/result_file_name_test.cpp | 73 --------------
.../export_p0/test_result_file_name_padding.groovy | 111 +++++++++++++++++++++
5 files changed, 119 insertions(+), 118 deletions(-)
diff --git a/be/src/exec/sink/writer/result_file_name.h
b/be/src/exec/sink/writer/result_file_name.h
deleted file mode 100644
index 111061bb48e..00000000000
--- a/be/src/exec/sink/writer/result_file_name.h
+++ /dev/null
@@ -1,39 +0,0 @@
-// 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 <fmt/format.h>
-
-#include <string>
-
-namespace doris {
-
-// Builds an output file name of the form "{prefix}{id}_{idx}.{suffix}",
shared by
-// the SELECT INTO OUTFILE writer (VFileResultWriter) and the INSERT INTO
FUNCTION
-// TVF writer (VTVFTableWriter).
-//
-// When pad > 0 the index is zero-padded to `pad` digits so that the files
produced
-// by a single writer sort lexicographically (e.g. _00011 sorts after _00002
rather
-// than before _2). pad <= 0 leaves the index unpadded, which is byte-for-byte
the
-// historical name format.
-inline std::string build_result_file_name(const std::string& prefix, const
std::string& id, int idx,
- int pad, const std::string& suffix) {
- return fmt::format("{}{}_{:0{}}.{}", prefix, id, idx, pad < 0 ? 0 : pad,
suffix);
-}
-
-} // namespace doris
diff --git a/be/src/exec/sink/writer/vfile_result_writer.cpp
b/be/src/exec/sink/writer/vfile_result_writer.cpp
index 4528eb92360..9ccf2f2dcbf 100644
--- a/be/src/exec/sink/writer/vfile_result_writer.cpp
+++ b/be/src/exec/sink/writer/vfile_result_writer.cpp
@@ -38,7 +38,6 @@
#include "core/value/decimalv2_value.h"
#include "core/value/large_int_value.h"
#include "exec/operator/result_sink_operator.h"
-#include "exec/sink/writer/result_file_name.h"
#include "exec/sink/writer/vmysql_result_writer.h"
#include "exprs/function/cast/cast_to_string.h"
#include "exprs/vexpr.h"
@@ -163,8 +162,11 @@ Status VFileResultWriter::_create_file_writer(const
std::string& file_name) {
Status VFileResultWriter::_get_next_file_name(std::string* file_name) {
std::string suffix =
_file_opts->file_suffix.empty() ? _file_format_to_name() :
_file_opts->file_suffix;
- *file_name = build_result_file_name(_file_opts->file_path,
print_id(_fragment_instance_id),
- _file_idx++,
_file_opts->file_name_padding, suffix);
+ // Padding is a minimum width: larger indices must retain every digit.
+ const int padding = _file_opts->file_name_padding;
+ *file_name =
+ fmt::format("{}{}_{:0{}}.{}", _file_opts->file_path,
print_id(_fragment_instance_id),
+ _file_idx++, padding < 0 ? 0 : padding, suffix);
if (_storage_type == TStorageBackendType::LOCAL) {
// For local file writer, the file_path is a local dir.
// Here we do a simple security verification by checking whether the
file exists.
diff --git a/be/src/exec/sink/writer/vtvf_table_writer.cpp
b/be/src/exec/sink/writer/vtvf_table_writer.cpp
index 137279c8c32..70765f51d81 100644
--- a/be/src/exec/sink/writer/vtvf_table_writer.cpp
+++ b/be/src/exec/sink/writer/vtvf_table_writer.cpp
@@ -21,7 +21,6 @@
#include "common/status.h"
#include "core/block/block.h"
-#include "exec/sink/writer/result_file_name.h"
#include "exprs/vexpr.h"
#include "exprs/vexpr_context.h"
#include "io/file_factory.h"
@@ -166,8 +165,9 @@ Status VTVFTableWriter::_get_next_file_name(std::string*
file_name) {
// file_path is a prefix, generate: {prefix}{query_id}_{idx}.{ext}
std::string query_id_str = print_id(_state->query_id());
- *file_name =
- build_result_file_name(_file_path, query_id_str, _file_idx++,
_file_name_padding, ext);
+ // Keep the same minimum-width index format as SELECT INTO OUTFILE.
+ *file_name = fmt::format("{}{}_{:0{}}.{}", _file_path, query_id_str,
_file_idx++,
+ _file_name_padding < 0 ? 0 : _file_name_padding,
ext);
return Status::OK();
}
diff --git a/be/test/exec/sink/writer/result_file_name_test.cpp
b/be/test/exec/sink/writer/result_file_name_test.cpp
deleted file mode 100644
index 9bb533edf64..00000000000
--- a/be/test/exec/sink/writer/result_file_name_test.cpp
+++ /dev/null
@@ -1,73 +0,0 @@
-// 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.
-
-#include "exec/sink/writer/result_file_name.h"
-
-#include <gtest/gtest.h>
-
-#include <algorithm>
-#include <string>
-#include <vector>
-
-namespace doris {
-
-TEST(ResultFileNameTest, no_padding_preserves_legacy_format) {
- EXPECT_EQ("s3://bucket/exp_abc_0.csv",
- build_result_file_name("s3://bucket/exp_", "abc", 0, 0, "csv"));
- EXPECT_EQ("s3://bucket/exp_abc_11.csv",
- build_result_file_name("s3://bucket/exp_", "abc", 11, 0, "csv"));
-}
-
-TEST(ResultFileNameTest, negative_padding_is_treated_as_none) {
- EXPECT_EQ("s3://bucket/exp_abc_7.csv",
- build_result_file_name("s3://bucket/exp_", "abc", 7, -1, "csv"));
-}
-
-TEST(ResultFileNameTest, padding_zero_fills_to_width) {
- EXPECT_EQ("s3://bucket/exp_abc_00000.csv",
- build_result_file_name("s3://bucket/exp_", "abc", 0, 5, "csv"));
- EXPECT_EQ("s3://bucket/exp_abc_00011.csv",
- build_result_file_name("s3://bucket/exp_", "abc", 11, 5, "csv"));
-}
-
-TEST(ResultFileNameTest, padding_does_not_truncate_when_index_exceeds_width) {
- // Width is a minimum, never a cap: an index wider than `pad` is emitted
in full.
- EXPECT_EQ("s3://bucket/exp_abc_123456.csv",
- build_result_file_name("s3://bucket/exp_", "abc", 123456, 5,
"csv"));
-}
-
-TEST(ResultFileNameTest, padded_names_sort_lexicographically) {
- // The bug this feature fixes: without padding, _11 sorts before _2.
- std::vector<std::string> unpadded;
- std::vector<std::string> padded;
- for (int i : {0, 1, 2, 10, 11, 100}) {
- unpadded.push_back(build_result_file_name("exp_", "id", i, 0, "csv"));
- padded.push_back(build_result_file_name("exp_", "id", i, 5, "csv"));
- }
-
- std::vector<std::string> unpadded_sorted = unpadded;
- std::sort(unpadded_sorted.begin(), unpadded_sorted.end());
- // Lexicographic sort scrambles the numeric order for unpadded names.
- EXPECT_NE(unpadded, unpadded_sorted);
-
- std::vector<std::string> padded_sorted = padded;
- std::sort(padded_sorted.begin(), padded_sorted.end());
- // Padded names keep numeric order under a lexicographic sort.
- EXPECT_EQ(padded, padded_sorted);
-}
-
-} // namespace doris
diff --git
a/regression-test/suites/export_p0/test_result_file_name_padding.groovy
b/regression-test/suites/export_p0/test_result_file_name_padding.groovy
new file mode 100644
index 00000000000..7250c9f4662
--- /dev/null
+++ b/regression-test/suites/export_p0/test_result_file_name_padding.groovy
@@ -0,0 +1,111 @@
+// 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.
+
+suite("test_result_file_name_padding", "p0,external") {
+ def bucket = context.config.otherConfigs.get("s3BucketName")
+ def prefix = "regression/result_file_name_padding/${UUID.randomUUID()}/"
+ def client = getS3Client()
+ def storage = """
+ "s3.endpoint" = "${getS3Endpoint()}",
+ "s3.region" = "${getS3Region()}",
+ "s3.access_key" = "${getS3AK()}",
+ "s3.secret_key" = "${getS3SK()}"
+ """
+ def listKeys = { String path ->
+ def keys = []
+ def listing = client.listObjects(bucket, path)
+ while (true) {
+ keys.addAll(listing.objectSummaries.collect { it.key })
+ if (!listing.isTruncated()) {
+ return keys.sort()
+ }
+ listing = client.listNextBatchOfObjects(listing)
+ }
+ }
+ def writeQuery = { String writer, String path, String format, String
extra, String query ->
+ if (writer == "outfile") {
+ return """${query} INTO OUTFILE "s3://${bucket}/${path}" FORMAT AS
${format}
+ PROPERTIES (${storage} ${extra})"""
+ } else {
+ return """INSERT INTO s3("file_path" = "s3://${bucket}/${path}",
+ "format" = "${format}", ${storage} ${extra}) ${query}"""
+ }
+ }
+ def readFiles = { String path, String format, String projection ->
+ sql """SELECT ${projection} FROM s3("uri" = "s3://${bucket}/${path}",
+ "format" = "${format}", ${storage})"""
+ }
+
+ sql "set parallel_pipeline_task_num = 1"
+ sql "set enable_parallel_outfile = false"
+ sql "set batch_size = 512"
+ try {
+ ["outfile", "tvf"].each { writer ->
+ ["csv", "parquet", "orc"].each { format ->
+ [null, 0, 1, 5, 10].each { padding ->
+ def path = "${prefix}${writer}_${format}_${padding}/part_"
+ def extra = padding == null ? "" : ",
\"file_name_padding\" = \"${padding}\""
+ sql writeQuery(writer, path, format, extra, "SELECT 7 AS
id")
+ def keys = listKeys(path)
+ assertEquals(1, keys.size())
+ def index = "0".padLeft(padding == null ? 0 : padding, '0')
+ assertTrue(keys[0].endsWith("_${index}.${format}"),
keys[0])
+ assertEquals("7", readFiles(keys[0], format,
"*")[0][0].toString())
+ }
+ }
+ ["-1", "11", "abc", "2147483648"].each { padding ->
+ test {
+ sql writeQuery(writer, "${prefix}invalid/part_", "csv",
+ ", \"file_name_padding\" = \"${padding}\"", "SELECT 7
AS id")
+ exception "file_name_padding"
+ }
+ }
+ }
+
+ def suffixPath = "${prefix}suffix/part_"
+ sql writeQuery("outfile", suffixPath, "csv",
+ ', "file_name_padding" = "5", "file_suffix" = "txt"', "SELECT 7 AS
id")
+ def suffixKeys = listKeys(suffixPath)
+ assertEquals(1, suffixKeys.size())
+ assertTrue(suffixKeys[0].endsWith("_00000.txt"))
+ assertEquals("7", readFiles(suffixKeys[0], "csv",
"*")[0][0].toString())
+
+ // Cross index 9 using real file rotation: width is a minimum, never a
truncation limit.
+ ["outfile": 1, "tvf": 5].each { writer, padding ->
+ def path = "${prefix}${writer}_rotation/part_"
+ def maxFileSize = writer == "outfile" ? "5MB" : "5242880"
+ sql writeQuery(writer, path, "csv",
+ ", \"file_name_padding\" = \"${padding}\", \"max_file_size\" =
\"${maxFileSize}\"",
+ "SELECT number, repeat('x', 4096) FROM numbers(\"number\" =
\"18000\") ORDER BY number")
+ def keys = listKeys(path)
+ assertTrue(keys.size() > 10, "Expected rotation past index 9:
${keys}")
+ def indices = keys.collect { key ->
+ def digits = key.substring(key.lastIndexOf('_') + 1,
key.lastIndexOf('.'))
+ def index = digits.toInteger()
+ assertEquals(index.toString().padLeft(padding, '0'), digits)
+ index
+ }
+ assertEquals((0..<keys.size()).toList(), indices.sort(false))
+ if (padding == 5) {
+ assertEquals(indices.sort(false), indices)
+ }
+ assertEquals("18000", readFiles("${path}*", "csv",
"count(*)")[0][0].toString())
+ }
+ } finally {
+ listKeys(prefix).each { client.deleteObject(bucket, it) }
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
