rdblue commented on a change in pull request #1512:
URL: https://github.com/apache/iceberg/pull/1512#discussion_r496244628



##########
File path: 
spark3/src/main/java/org/apache/iceberg/spark/source/SparkBatchScan.java
##########
@@ -208,6 +213,33 @@ public Statistics estimateStatistics() {
     return new Stats(sizeInBytes, numRows);
   }
 
+  @Override
+  public boolean equals(Object o) {
+    if (this == o) {
+      return true;
+    }
+
+    if (o == null || getClass() != o.getClass()) {
+      return false;
+    }
+
+    SparkBatchScan that = (SparkBatchScan) o;
+    return table.toString().equals(that.table.toString()) &&
+        readSchema().equals(that.readSchema()) && // compare Spark schemas to 
ignore field ids
+        filterExpressions.toString().equals(that.filterExpressions.toString()) 
&&

Review comment:
       This is definitely something I think we should improve.
   
   Right now, we don't implement `equals` because we don't want people to use 
it to test semantic equivalence. Here, we actually want semantic equivalence, 
but with no utility to evaluate it the simplest way to check for identical 
structure is the string representation. A better first solution would be to 
build a comparison utility to test whether two expressions are structurally 
identical, and after that to introduce one for semantic equivalence.




----------------------------------------------------------------
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:
[email protected]



---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to