anuragmantri commented on code in PR #17504:
URL: https://github.com/apache/iceberg/pull/17504#discussion_r3833908909
##########
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:
The PR description says this is only for `sort` strategy. If you add it
here, this applies to both sort and zOrder. The issue with that is the actual
computation of overlap considers sort columns and not the zOrder columns. Can
we move this to sort strategy 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:
I'm not convinced of the value of this being a separate procedure. This is
different from `ComputePartitionStats` for example, that the result of the
computation is stored. There is not precedent for anything advisory like this.
This feels closer to the `dry_run` option on RemoveOrphanFiles. Can this be
modeled as a mode on the sort strategy itself instead of a separate action,
something like below that reports the overlap depths it would act on without
rewriting anything?
```java
rewrite_data_files(..., options => map('report-only', 'true'))
```
--
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]
