gimgit commented on code in PR #17504:
URL: https://github.com/apache/iceberg/pull/17504#discussion_r3834652923
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/SparkShufflingDataRewritePlanner.java:
##########
@@ -46,7 +52,24 @@ class SparkShufflingDataRewritePlanner extends
BinPackRewriteFilePlanner {
public static final double COMPRESSION_FACTOR_DEFAULT = 1.0;
+ /**
+ * The overlap depth at which files are selected for rewrite regardless of
their size.
+ *
+ * <p>Size based selection cannot detect files that are large enough but
whose sort key ranges sit
+ * on top of each other, so a sorted rewrite may report success without
improving clustering. When
+ * this option is set, files that intersect a region of the table sort key
covered by at least
+ * this many files of the same partition are added to the rewrite, on top of
the files selected by
+ * size and by deletes.
+ *
+ * <p>The overlap is measured on the table sort order using data file bounds
only, as reported by
+ * the {@code compute_sort_order_stats} procedure. Because column bounds may
be truncated, the
+ * measured depth is an upper-bound estimate. Unset by default, which leaves
selection unchanged.
+ */
+ public static final String MIN_OVERLAP_DEPTH = "min-overlap-depth";
Review Comment:
Right — the planner is shared with zorder, so as written the option leaks
there while the computation only makes sense on the table sort order. I'll
reject `min-overlap-depth` when the runner is zorder and document it as
sort-only.
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/procedures/ComputeSortOrderStatsProcedure.java:
##########
@@ -0,0 +1,135 @@
+/*
+ * 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.
+ */
+package org.apache.iceberg.spark.procedures;
+
+import java.util.Iterator;
+import java.util.List;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.SortOrderStatsHandler;
+import org.apache.iceberg.SortOrderStatsHandler.PartitionOverlapStats;
+import org.apache.iceberg.spark.procedures.SparkProcedures.ProcedureBuilder;
+import org.apache.spark.sql.catalyst.InternalRow;
+import org.apache.spark.sql.connector.catalog.Identifier;
+import org.apache.spark.sql.connector.catalog.TableCatalog;
+import org.apache.spark.sql.connector.catalog.procedures.BoundProcedure;
+import org.apache.spark.sql.connector.catalog.procedures.ProcedureParameter;
+import org.apache.spark.sql.connector.read.Scan;
+import org.apache.spark.sql.types.DataTypes;
+import org.apache.spark.sql.types.Metadata;
+import org.apache.spark.sql.types.StructField;
+import org.apache.spark.sql.types.StructType;
+import org.apache.spark.unsafe.types.UTF8String;
+
+/**
+ * A procedure that computes per-partition file overlap statistics on the
table's sort order,
+ * reading only data file metadata (column bounds) — no data files are opened
and nothing is
+ * committed to the table.
+ *
+ * <p>The maximum overlap depth of a partition is the largest number of data
files whose sort-key
+ * ranges cover a single point. A value of 1 means the partition is perfectly
clustered on the first
+ * sort field; values close to the file count mean sorting has not
materialized in the file layout.
+ *
+ * @see SortOrderStatsHandler
+ */
+public class ComputeSortOrderStatsProcedure extends BaseProcedure {
Review Comment:
Agreed on dropping the separate procedure — `report-only` on the rewrite
keeps the surface smaller and the measurement path identical to what the
planner actually acts on, which is better than what I had. Net code reduction
too.
On output shape, two options I considered:
**(a)** Keep the single summary row and add aggregate columns (table-level
max/avg overlap depth). Minimal change, but it collapses the distribution — one
hot partition dominates the max while the average dilutes it, so there's no way
to pick a sensible `min-overlap-depth` from it. That defeats the advisory
purpose.
**(b)** Keep one fixed superset schema — the existing count columns plus
nullable `partition`, `max_overlap_depth`, `avg_overlap_depth`,
`candidate_file_count`, `candidate_bytes`, `missing_bounds_file_count` — and
vary only the row cardinality: normal mode returns the summary row as today
(new columns null), `report-only` returns one row per partition. This follows
the existing precedent for a mode option (`remove_orphan_files.dry_run` changes
behavior and row count, never the schema), and a fixed schema also keeps a 3.5
backport possible, where `outputType()` is static.
I'll go with (b) unless you see a problem with it. `report-only` will be
validated to never commit (tested), and an invalid option value fails fast.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
