[GitHub] [incubator-iceberg] rdblue commented on a change in pull request #786: replace SparkDataFile with DataFile

[GitBox <gi...@apache.org>]

rdblue commented on a change in pull request #786: replace SparkDataFile with DataFile
URL: https://github.com/apache/incubator-iceberg/pull/786#discussion_r380320714
 
 

 ##########
 File path: spark/src/main/scala/org/apache/iceberg/spark/SparkTableUtil.scala
 ##########
 @@ -329,70 +222,74 @@ object SparkTableUtil {
   private def listAvroPartition(
       partitionPath: Map[String, String],
       partitionUri: String,
-      conf: Configuration): Seq[SparkDataFile] = {
+      spec: PartitionSpec,
+      conf: Configuration): Seq[DataFile] = {
     val partition = new Path(partitionUri)
     val fs = partition.getFileSystem(conf)
 
     fs.listStatus(partition, HiddenPathFilter).filter(_.isFile).map { stat =>
-      SparkDataFile(
-        stat.getPath.toString,
-        partitionPath, "avro", stat.getLen,
-        stat.getBlockSize,
-        -1,
-        null,
-        null,
-        null,
-        null,
-        null)
+      val metrics = new Metrics(-1L, arrayToMap(null), arrayToMap(null), arrayToMap(null))
 
 Review comment:
   Good catch, @rdsr. This is definitely a problem. Right now, the [`InclusiveMetricsEvaluator`](https://github.com/apache/incubator-iceberg/blob/master/api/src/main/java/org/apache/iceberg/expressions/InclusiveMetricsEvaluator.java#L89) will remove files with negative or 0 row counts.
   
   I don't think that the solution is to use a positive number here. The reason why this was required is that we want good stats for job planning. Setting this to -1 causes a correctness bug, but setting it to some other constant will introduce bad behavior when using the stats that are provided by Iceberg. I think we should either count the number of records, use a heuristic (file size / est. row size?), or remove support for importing Avro tables. I'm leaning toward counting the number of records.
   
   We should also change the check in `InclusiveMetricsEvaluator` to check for files with 0 rows and allow files with -1 rows through to fix the correctness bug for existing tables that used this path to import Avro data.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org