[GitHub] [iceberg] kbendick commented on a change in pull request #1572: Refactor reusable map data in Spark and Flink parquet readers #1331

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

kbendick commented on a change in pull request #1572:
URL: https://github.com/apache/iceberg/pull/1572#discussion_r502841925



##########
File path: spark/src/main/java/org/apache/iceberg/spark/data/SparkParquetReaders.java
##########
@@ -655,6 +653,11 @@ public int numElements() {
       return numElements;
     }
 
+    @Override
+    public int getNumElements() {

Review comment:
       It looks like the function `numElements` is still there, just above this one

##########
File path: spark/src/main/java/org/apache/iceberg/spark/data/SparkParquetReaders.java
##########
@@ -655,6 +653,11 @@ public int numElements() {
       return numElements;
     }
 
+    @Override
+    public int getNumElements() {

Review comment:
       It looks like the function `numElements` is still there (it's part of the catalyst abstract class), in the lines just above these ones.
   
   But unless there's something going on with Spark, I agree with @rdblue.
   
   I looked to see if `getNumElements` were needed on the Flink side, as Flink highly favors POJOs for performance reasons and then for style reasons they also tend to use POJO-y like names, but the Flink `ArrayData` interface uses `size` instead, so keeping the iceberg ReusableArrayData interface to use `numElements` seems like it would kill two birds with one stone by also fulfilling the catalyst contract.




----------------------------------------------------------------
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



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