[GitHub] [drill] vdiravka commented on a change in pull request #2310: LGTM Code Cleanup-Fixed Javadoc param tags and type conversions

[GitBox <gi...@apache.org>]

vdiravka commented on a change in pull request #2310:
URL: https://github.com/apache/drill/pull/2310#discussion_r702143780



##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/base/filter/FilterPushDownListener.java
##########
@@ -126,13 +122,7 @@
      * to leave in the query. Those terms can be the ones passed in, or
      * new terms to handle special needs.
      *
-     * @param groupScan the scan node
-     * @param andTerms a list of the CNF (AND) terms, in which each is given
-     * by the Calcite AND node and the derived RelOp expression.
-     * @param orTerm the DNF (OR) term, if any, that includes the Calcite
-     * node for that term and the set of OR terms. Only provided if the OR
-     * term represents a simple list of values (all OR clauses are on the
-     * same column). The OR term itself is AND'ed with the CNF terms.
+     * @param expr

Review comment:
       Could you describe all newly added empty params? We usually don't add empty param docs
   Please refer to the doc of `AndNode` to accomplish that

##########
File path: contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/json/JsonTableGroupScan.java
##########
@@ -377,7 +377,11 @@ private ScanStats indexScanStats() {
     int totalColNum = STAR_COLS;
     PluginCost pluginCostModel = formatPlugin.getPluginCostModel();
     final int avgColumnSize = pluginCostModel.getAverageColumnSize(this);
-    boolean filterPushed = (scanSpec.getSerializedFilter() != null);
+    boolean filterPushed;
+    if (scanSpec == null) {

Review comment:
       Looks like `scanSpec` is required field for this class and it is used everywhere in the class, but never checked for the `null` value. So it looks like a redundant check.

##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/base/filter/FilterPushDownListener.java
##########
@@ -100,10 +100,6 @@
      * If so, return an equivalent RelOp with the value normalized to what
      * the plugin needs. The returned value may be the same as the original
      * one if the value is already normalized.
-     *
-     * @param groupScan the scan element. Use {@code scan.getGroupScan()}
-     * to get the group scan
-     * @param relOp the description of the relational operator expression

Review comment:
       Need to add javadoc for `conjunct` param instead of the removed ones

##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/base/filter/FilterPushDownListener.java
##########
@@ -43,7 +43,7 @@
  * <dl>
  * <p>
  * In both cases, the conditions are in the form of a
- * {@link ColRelOpConst} in which one side refers to a column in the scan
+ * {@link  } in which one side refers to a column in the scan

Review comment:
       What is `_`?

##########
File path: common/src/main/java/org/apache/drill/common/HistoricalLog.java
##########
@@ -119,16 +119,10 @@ public void buildHistory(final StringBuilder sb, boolean includeStackTrace) {
    * events with their stack traces.
    *
    * @param sb {@link StringBuilder} to write to
-   * @param additional an extra string that will be written between the identifying
-   *     information and the history; often used for a current piece of state
-   */
-
-  /**
-   *
-   * @param sb
-   * @param indexLevel
+   * @param indent
    * @param includeStackTrace
    */
+

Review comment:
       redundant space




-- 
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: dev-unsubscribe@drill.apache.org

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