dramaticlly commented on code in PR #7363:
URL: https://github.com/apache/iceberg/pull/7363#discussion_r1169135730
##########
data/src/main/java/org/apache/iceberg/data/TableMigrationUtil.java:
##########
@@ -102,13 +104,19 @@ public static List<DataFile> listPartition(
Path partition = new Path(partitionUri);
FileSystem fs = partition.getFileSystem(conf);
- List<FileStatus> fileStatus =
- Arrays.stream(fs.listStatus(partition, HIDDEN_PATH_FILTER))
- .filter(FileStatus::isFile)
- .collect(Collectors.toList());
- DataFile[] datafiles = new DataFile[fileStatus.size()];
+ FileStatus[] fileStatuses = fs.listStatus(partition, HIDDEN_PATH_FILTER);
+ List<FileStatus> files = Lists.newArrayList();
Review Comment:
nit, maybe leave List with original name `fileStatuses` and get a new name
for array to avoid all those rename change below? I think Array is only used
once here
##########
spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/procedures/AddFilesProcedure.java:
##########
@@ -165,12 +168,16 @@ private void importFileTable(
boolean checkDuplicateFiles,
PartitionSpec spec) {
// List Partitions via Spark InMemory file search interface
+ long start = System.currentTimeMillis();
List<SparkPartition> partitions =
Spark3Util.getPartitions(spark(), tableLocation, format,
partitionFilter, spec);
+ LOG.info("found {} partitions in {}", partitions.size(),
Spark3Util.duration(start));
Review Comment:
I think such instrumentation is helpful when comparing performance, but do
you think it's needed for all imports?
##########
spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java:
##########
@@ -494,6 +506,20 @@ public static boolean extensionsEnabled(SparkSession
spark) {
return extensions.contains("IcebergSparkSessionExtensions");
}
+ public static boolean isPartitioned(SparkSession spark, Path tableLocation) {
+ try {
+ return Arrays.stream(
+
FileSystem.get(spark.sparkContext().hadoopConfiguration()).listStatus(tableLocation))
Review Comment:
checkstyle favor `spark.sessionState().newHadoopConf()` over `
spark.sparkContext().hadoopConfiguration()` as session config is not lost
##########
spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java:
##########
@@ -80,18 +86,24 @@
import org.apache.spark.sql.connector.expressions.NamedReference;
import org.apache.spark.sql.connector.expressions.Transform;
import org.apache.spark.sql.execution.datasources.FileStatusCache;
-import org.apache.spark.sql.execution.datasources.InMemoryFileIndex;
import org.apache.spark.sql.execution.datasources.PartitionDirectory;
+import org.apache.spark.sql.execution.datasources.PartitioningAwareFileIndex;
import org.apache.spark.sql.types.IntegerType;
import org.apache.spark.sql.types.LongType;
import org.apache.spark.sql.types.StructType;
import org.apache.spark.sql.util.CaseInsensitiveStringMap;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import scala.Option;
+import scala.Tuple2;
import scala.collection.JavaConverters;
+import scala.collection.immutable.Map$;
import scala.collection.immutable.Seq;
+import scala.collection.mutable.LinkedHashMap;
public class Spark3Util {
+ private static Logger log = LoggerFactory.getLogger(Spark3Util.class);
Review Comment:
nit: maybe logger?
##########
spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java:
##########
@@ -996,4 +1027,134 @@ public String unknown(
return String.format("%s(%s) %s %s", transform, sourceName, direction,
nullOrder);
}
}
+
+ /**
+ * Implement our own index in-memory index which will only list directories
to avoid unnecessary
+ * file listings. Should ONLY be used to get partition directory paths. Uses
table's schema to
+ * only visit partition dirs using number of partition columns depth
recursively. Does NOT return
+ * files within leaf dir.
+ */
+ private static class InMemoryLeafDirOnlyIndex extends
PartitioningAwareFileIndex {
Review Comment:
shall we add any test for this?
--
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]
