You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2020/07/29 07:43:28 UTC

[GitHub] [iceberg] rdsr commented on a change in pull request #1239: Add incremental scan for iceberg generics scan builder

rdsr commented on a change in pull request #1239:
URL: https://github.com/apache/iceberg/pull/1239#discussion_r461400750



##########
File path: data/src/test/java/org/apache/iceberg/data/TestLocalScan.java
##########
@@ -174,6 +174,64 @@ private void overwriteExistingData() throws IOException {
         .commit();
   }
 
+  private void appendData() throws IOException {

Review comment:
       I generally found the tests in TestLocalScan to be a little hard to read. It seems that the sharedTable being used in each test can have a different state depending upon the order of execution of the tests as it is initialized only once before all tests run. Also,  the data records `file1FirstSnapshotRecords` etc are being initialized by each test on their own, maybe we could make them constants. I don't think we need to take action on this though, if u all agree we can take it up as a separate task.

##########
File path: data/src/test/java/org/apache/iceberg/data/TestLocalScan.java
##########
@@ -174,6 +174,64 @@ private void overwriteExistingData() throws IOException {
         .commit();
   }
 
+  private void appendData() throws IOException {

Review comment:
       nit: I generally found the tests in TestLocalScan to be a little hard to read. It seems that the sharedTable being used in each test can have a different state depending upon the order of execution of the tests as it is initialized only once before all tests run. Also,  the data records `file1FirstSnapshotRecords` etc are being initialized by each test on their own, maybe we could make them constants. I don't think we need to take action on this though, if u all agree we can take it up as a separate task.




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