RussellSpitzer commented on code in PR #4438:
URL: https://github.com/apache/iceberg/pull/4438#discussion_r841829735
##########
core/src/main/java/org/apache/iceberg/BaseFilesTable.java:
##########
@@ -108,7 +108,8 @@ public TableScan appendsAfter(long fromSnapshotId) {
}
/**
- * @return list of manifest files to explore for this files metadata table
scan
+ * Returns list of {@link ManifestFile} files to explore for this file's
metadata table scan
+ * @return list of manifest files
Review Comment:
Yeah in this case like the warning suggests the right thing to do is
```java
Returns list of {@link ManifestFile} files to explore for this file's
metadata table scan
```
Because when JavaDoc builds a page this single line will be present in the
summary section. In cases like this you don't need a separate @return line.
The error from static analysis is to avoid an empty entry in the summary
table.
##########
core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java:
##########
@@ -27,9 +27,6 @@
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
import org.apache.iceberg.rest.RESTResponse;
-/**
- *
- */
Review Comment:
I think this can probably be improved, for example see ListTablesResponse
```java
/**
* A list of table identifiers in a given namespace.
*/
```
Here I think it would be something like
"Metadata and config for a given table" ?
That's probably not great either, but the description should help us know
what this class is for, in this case it's really just a container so we should
describe it's contents imho.
##########
hive3/src/main/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -191,12 +191,12 @@ public boolean hasFooter() {
}
/**
- * @return {@code true} if file schema doesn't have Acid metadata columns
* Such file may be in a delta_x_y/ or base_x due to being added via
* "load data" command. It could be at partition|table root due to table
having
* been converted from non-acid to acid table. It could even be something
like
* "warehouse/t/HIVE_UNION_SUBDIR_15/000000_0" if it was written by an
* "insert into t select ... from A union all select ... from B"
+ * @return {@code true} if file schema doesn't have Acid metadata columns
Review Comment:
The description added here is a bit confusing. Maybe something like
```java
A check for whether or not this file was originally created in a Hive ACID
table.
A file may be in a delta_x_y/ or base_x due to being added via
"load data" command. It could be located in a partition|table root due to
table having
been converted from non-acid to acid table. It could also even have paths
containing
"warehouse/t/HIVE_UNION_SUBDIR_15/000000_0" if it was written by an
"insert into t select ... from A union all select ... from B"
@return true if the file was written originally by a Hive ACID table
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
