Copilot commented on code in PR #2601:
URL: https://github.com/apache/orc/pull/2601#discussion_r3117482616
##########
java/tools/src/java/org/apache/orc/tools/MergeFiles.java:
##########
@@ -100,11 +137,88 @@ public static void main(Configuration conf, String[]
args) throws Exception {
}
}
+ /**
+ * Multi-output behavior when --maxSize is set.
+ * Input files are grouped by cumulative raw file size; each group is merged
into
+ * a separate part file (part-00000.orc, part-00001.orc, ...) under
outputDir.
+ * A single file whose size already exceeds maxSizeBytes is placed in its
own part.
+ */
+ private static void mergeIntoMultipleFiles(Configuration conf,
+ OrcFile.WriterOptions
writerOptions,
+ List<LocatedFileStatus>
inputStatuses,
+ List<Path> inputFiles,
+ Path outputDir,
+ long maxSizeBytes) throws
Exception {
+ FileSystem outFs = outputDir.getFileSystem(conf);
+ if (outFs.exists(outputDir)) {
+ if (!outFs.getFileStatus(outputDir).isDirectory()) {
+ throw new IllegalArgumentException(
+ "Output path already exists and is not a directory: " + outputDir);
+ }
+ if (outFs.listStatus(outputDir).length > 0) {
+ throw new IllegalArgumentException(
+ "Output directory must be empty for multi-file merge: " +
outputDir);
+ }
+ } else if (!outFs.mkdirs(outputDir)) {
+ throw new IllegalStateException("Failed to create output directory: " +
outputDir);
+ }
+
+ // Group input files into batches where each batch's total size <=
maxSizeBytes.
+ List<List<Path>> batches = new ArrayList<>();
+ List<Path> currentBatch = new ArrayList<>();
+ long currentBatchSize = 0;
+
+ for (LocatedFileStatus status : inputStatuses) {
+ long fileSize = status.getLen();
+ if (!currentBatch.isEmpty() && currentBatchSize + fileSize >
maxSizeBytes) {
+ batches.add(currentBatch);
+ currentBatch = new ArrayList<>();
+ currentBatchSize = 0;
+ }
+ currentBatch.add(status.getPath());
+ currentBatchSize += fileSize;
+ }
+ if (!currentBatch.isEmpty()) {
+ batches.add(currentBatch);
+ }
+
+ int totalMerged = 0;
+ List<Path> allUnmerged = new ArrayList<>();
+
+ for (int i = 0; i < batches.size(); i++) {
+ List<Path> batch = batches.get(i);
+ Path partOutput = new Path(outputDir, String.format(PART_FILE_FORMAT,
i));
+ List<Path> merged = OrcFile.mergeFiles(partOutput,
writerOptions.clone(), batch);
+ totalMerged += merged.size();
+
+ if (merged.size() != batch.size()) {
+ Set<Path> mergedSet = new HashSet<>(merged);
+ for (Path p : batch) {
+ if (!mergedSet.contains(p)) {
+ allUnmerged.add(p);
+ }
+ }
+ }
+ }
+
Review Comment:
In multi-output mode, each batch is merged independently and the first file
in each batch becomes the schema/reference for that part. That means a
mixed-schema input set could be split into batches such that every batch is
internally compatible, leading to a successful run that produces part files
with different schemas—contradicting the tool/docs claim that inputs share the
same schema. Consider validating that all input files share the same schema (at
minimum) before batching, and failing (or listing as unmergeable) any files
that differ, so behavior matches single-output mode expectations.
##########
java/tools/src/java/org/apache/orc/tools/MergeFiles.java:
##########
@@ -100,11 +137,88 @@ public static void main(Configuration conf, String[]
args) throws Exception {
}
}
+ /**
+ * Multi-output behavior when --maxSize is set.
+ * Input files are grouped by cumulative raw file size; each group is merged
into
+ * a separate part file (part-00000.orc, part-00001.orc, ...) under
outputDir.
+ * A single file whose size already exceeds maxSizeBytes is placed in its
own part.
+ */
+ private static void mergeIntoMultipleFiles(Configuration conf,
+ OrcFile.WriterOptions
writerOptions,
+ List<LocatedFileStatus>
inputStatuses,
+ List<Path> inputFiles,
+ Path outputDir,
+ long maxSizeBytes) throws
Exception {
+ FileSystem outFs = outputDir.getFileSystem(conf);
+ if (outFs.exists(outputDir)) {
+ if (!outFs.getFileStatus(outputDir).isDirectory()) {
+ throw new IllegalArgumentException(
+ "Output path already exists and is not a directory: " + outputDir);
+ }
+ if (outFs.listStatus(outputDir).length > 0) {
+ throw new IllegalArgumentException(
+ "Output directory must be empty for multi-file merge: " +
outputDir);
+ }
+ } else if (!outFs.mkdirs(outputDir)) {
+ throw new IllegalStateException("Failed to create output directory: " +
outputDir);
+ }
Review Comment:
mergeIntoMultipleFiles throws IllegalArgumentException/IllegalStateException
for common CLI misuse (output path exists and is not a directory, output dir
non-empty, mkdirs failure). Since MergeFiles is a CLI tool, these will surface
as stack traces rather than a clean one-line error and exit code. Consider
catching these conditions in main and printing a user-friendly message (and
optionally help) before exiting non-zero, consistent with the rest of the
tool’s error handling.
##########
java/tools/src/java/org/apache/orc/tools/MergeFiles.java:
##########
@@ -56,27 +60,60 @@ public static void main(Configuration conf, String[] args)
throws Exception {
}
boolean ignoreExtension = cli.hasOption("ignoreExtension");
- List<Path> inputFiles = new ArrayList<>();
- OrcFile.WriterOptions writerOptions = OrcFile.writerOptions(conf);
+ long maxSizeBytes = 0;
+ if (cli.hasOption("maxSize")) {
+ try {
+ maxSizeBytes = Long.parseLong(cli.getOptionValue("maxSize"));
+ } catch (NumberFormatException e) {
+ System.err.println("--maxSize requires a numeric value in bytes.");
+ System.exit(1);
+ }
+ if (maxSizeBytes <= 0) {
+ System.err.println("--maxSize must be a positive number of bytes.");
+ System.exit(1);
+ }
+ }
+ List<LocatedFileStatus> inputStatuses = new ArrayList<>();
String[] files = cli.getArgs();
for (String root : files) {
Path rootPath = new Path(root);
FileSystem fs = rootPath.getFileSystem(conf);
for (RemoteIterator<LocatedFileStatus> itr = fs.listFiles(rootPath,
true); itr.hasNext(); ) {
LocatedFileStatus status = itr.next();
if (status.isFile() && (ignoreExtension ||
status.getPath().getName().endsWith(".orc"))) {
- inputFiles.add(status.getPath());
+ inputStatuses.add(status);
}
}
}
Review Comment:
Batching depends on the iteration order returned by FileSystem#listFiles,
which is not guaranteed to be stable. With --maxSize this can change which
inputs land in which part file across runs/filesystems, making outputs
non-deterministic. Consider sorting inputStatuses (e.g., by path, or by path
then length) before creating batches to make part grouping reproducible.
##########
site/_docs/java-tools.md:
##########
@@ -356,13 +356,39 @@
______________________________________________________________________
## Java Merge
-The merge command can merge multiple ORC files that all have the same schema
into a single ORC file.
+The merge command can merge multiple ORC files that all have the same schema.
By default
+it writes a single output file. If `--maxSize` is set, `--output` is treated
as a directory
+and the tool writes multiple part files (`part-00000.orc`, `part-00001.orc`,
…) under it.
+Input files are grouped using their on-disk sizes so that each part’s total
input size
+does not exceed the given threshold (a single input file larger than the
threshold is still
+merged into its own part).
+
+`-h,--help`
+ : Print help
+
+`-i,--ignoreExtension`
+ : Include files that do not end in `.orc`
+
+`-m,--maxSize <bytes>`
+ : Maximum size in bytes for each output part; enables multi-file output
under `--output`
+
+`-o,--output <path>`
+ : Output ORC filename (single-file mode) or output directory (when
`--maxSize` is set)
+
Review Comment:
The implementation requires the multi-output `--output` directory to already
be empty (it errors if it exists and has any contents). The docs describe
`--output` as a directory when `--maxSize` is set, but don’t mention the
emptiness requirement, which will surprise users. Please document this
constraint (or relax it in code by writing into a subdirectory / allowing
overwrite semantics).
--
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]
