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 2022/03/31 02:26:39 UTC
[GitHub] [iceberg] felixYyu opened a new pull request #4438: Removing empty java docs, fixing java docs
felixYyu opened a new pull request #4438:
URL: https://github.com/apache/iceberg/pull/4438
fix #4376 cc @RussellSpitzer
--
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-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] rdblue commented on a change in pull request #4438: Removing empty java docs, fixing java docs
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4438:
URL: https://github.com/apache/iceberg/pull/4438#discussion_r839934491
##########
File path: 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:
Why is this change needed?
--
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-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] rdblue commented on a change in pull request #4438: Static Analysis: Removing empty java docs, fixing java docs
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4438:
URL: https://github.com/apache/iceberg/pull/4438#discussion_r841246873
##########
File path: 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:
I think the right fix is to document the method, not to just move the `@return`. Can you fix the documentation here instead? Javadoc should start with a short (one sentence) description and then go into more detail if needed after a paragraph break.
--
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-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] felixYyu commented on a change in pull request #4438: Removing empty java docs, fixing java docs
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4438:
URL: https://github.com/apache/iceberg/pull/4438#discussion_r840180392
##########
File path: 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:
removing redundant e.g.
--
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-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] rdblue commented on a change in pull request #4438: Removing empty java docs, fixing java docs
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4438:
URL: https://github.com/apache/iceberg/pull/4438#discussion_r839934802
##########
File path: 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:
I think this can be improved, but moving this line to the end doesn't help readability. Can you rewrite this doc instead?
--
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-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] felixYyu commented on a change in pull request #4438: Removing empty java docs, fixing java docs
Posted by GitBox <gi...@apache.org>.
felixYyu commented on a change in pull request #4438:
URL: https://github.com/apache/iceberg/pull/4438#discussion_r840153023
##########
File path: 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:
the static check warning log ，so I think should need change:
```
[MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.
* @return list of manifest files to explore for this files metadata table scan
^
(see https://google.github.io/styleguide/javaguide.html#s7.2-summary-fragment)
Did you mean '*Returns list of manifest files to explore for this files metadata table scan.'?
```
--
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-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] rdblue commented on a change in pull request #4438: Static Analysis: Removing empty java docs, fixing java docs
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4438:
URL: https://github.com/apache/iceberg/pull/4438#discussion_r841246766
##########
File path: 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 the correct fix is to add javadoc, not remove the placeholder.
--
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-unsubscribe@iceberg.apache.org
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