nastra commented on a change in pull request #2826:
URL: https://github.com/apache/iceberg/pull/2826#discussion_r690999266



##########
File path: core/src/test/java/org/apache/iceberg/TestTableMetadata.java
##########
@@ -90,11 +90,11 @@ public void testJsonConversion() throws Exception {
     long previousSnapshotId = System.currentTimeMillis() - new 
Random(1234).nextInt(3600);
     Snapshot previousSnapshot = new BaseSnapshot(
         ops.io(), previousSnapshotId, null, previousSnapshotId, null, null, 
null, ImmutableList.of(
-        new GenericManifestFile(localInput("file:/tmp/manfiest.1.avro"), 
SPEC_5.specId())));
+          new GenericManifestFile(localInput("file:/tmp/manfiest.1.avro"), 
SPEC_5.specId())));

Review comment:
       yes these are required

##########
File path: 
hive3/src/main/java/org/apache/iceberg/mr/hive/vector/CompatibilityHiveVectorUtils.java
##########
@@ -59,7 +59,7 @@ private CompatibilityHiveVectorUtils() {
    * Returns serialized mapwork instance from a job conf - ported from Hive 
source code LlapHiveUtils#findMapWork
    *
    * @param job JobConf instance
-   * @return
+   * @return A serialized {@link MapWork} based on the given job conf

Review comment:
       I searched across the codebase and it seems that it's a bit inconsistent 
in terms of upper/lowercase. I generally think that something like this would 
be better if addressed by Checkstyle (but not sure if checkstyle supports such 
a check). Otherwise stuff like this will essentially pop up in lots of PR 
reviews

##########
File path: mr/src/test/java/org/apache/iceberg/mr/hive/TestTables.java
##########
@@ -115,7 +115,7 @@ public String catalogName() {
   /**
    * The table properties string needed for the CREATE TABLE ... commands,
    * like "TBLPROPERTIES('iceberg.catalog'='mycatalog')
-   * @return
+   * @return The tables properties string, such as 
"TBLPROPERTIES('iceberg.catalog'='mycatalog')

Review comment:
       ah this is just a blind copy/paste from the javadoc description a few 
lines above without actually me checking whether it had missing pieces..will 
update




-- 
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: issues-unsubscr...@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



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

Reply via email to