[GitHub] [carbondata] QiangCai opened a new pull request #3868: [CARBONDATA-3889] Cleanup code in carbondata-core module

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] QiangCai opened a new pull request #3868: [CARBONDATA-3889] Cleanup code in carbondata-core module

GitBox

QiangCai opened a new pull request #3868:
URL: https://github.com/apache/carbondata/pull/3868


    ### Why is this PR needed?
   1. Redundant 'if' statement
   2. Array access many times in for loop
   3. Repeated switch branch
   4. Use StringBuffer
   
    ### What changes were proposed in this PR?
   1. Change redundant 'if' statement
   2. Use enhanced for loop
   3. Merge switch branch
   4. Use StringBuilder instead of StringBuffer
       
    ### Does this PR introduce any user interface change?
    - No
    - Yes. (please explain the change and update document)
   
    ### Is any new testcase added?
    - No
    - Yes
   
       
   


----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] kevinjmh commented on a change in pull request #3868: [CARBONDATA-3889] Cleanup code in carbondata-core module

GitBox

kevinjmh commented on a change in pull request #3868:
URL: https://github.com/apache/carbondata/pull/3868#discussion_r461986247



##########
File path: core/src/main/java/org/apache/carbondata/core/datastore/page/SafeFixLengthColumnPage.java
##########
@@ -283,8 +283,8 @@ public BigDecimal getDecimal(int rowId) {
   @Override
   public byte[][] getByteArrayPage() {
     byte[][] data = new byte[arrayElementCount][];
-    for (int i = 0; i < arrayElementCount; i++) {
-      data[i] = fixedLengthData[i];
+    if (arrayElementCount >= 0) {

Review comment:
       ```suggestion
       if (arrayElementCount = 0) {
   ```

##########
File path: core/src/main/java/org/apache/carbondata/core/index/IndexMeta.java
##########
@@ -78,7 +78,7 @@ public String toString() {
     return new StringBuilder("IndexMeta{")
         .append("indexName='").append(indexName).append('\'')
         .append(", indexedColumns=[")
-        .append(StringUtils.join(getIndexedColumnNames(), ", ")).append("]\'")
+        .append(StringUtils.join(getIndexedColumnNames(), ", ")).append("]'")
         .append(", optimizedOperation=").append(optimizedOperation)
         .append('}')
         .toString();

Review comment:
       ```
       return new StringBuilder("IndexMeta{indexName='")
           .append(indexName)
           .append("', indexedColumns='[")
           .append(StringUtils.join(getIndexedColumnNames(), ", "))
           .append("]', optimizedOperation='")
           .append(optimizedOperation)
           .append("'}")
           .toString();
   ```
   keep same quotation marks

##########
File path: core/src/main/java/org/apache/carbondata/core/indexstore/Blocklet.java
##########
@@ -105,20 +106,18 @@ public boolean equals(Object o) {
 
     Blocklet blocklet = (Blocklet) o;
 
-    if (filePath != null ? !filePath.equals(blocklet.filePath) : blocklet.filePath != null) {
+    if (!Objects.equals(filePath, blocklet.filePath)) {
       return false;
     }
     if (!compareBlockletIdForObjectMatching) {
       return true;
     }
-    return blockletId != null ?
-        blockletId.equals(blocklet.blockletId) :
-        blocklet.blockletId == null;
+    return Objects.equals(blockletId, blocklet.blockletId);
   }
 
   @Override
   public String toString() {
-    final StringBuffer sb = new StringBuffer("Blocklet{");
+    final StringBuilder sb = new StringBuilder("Blocklet{");
     sb.append("filePath='").append(filePath).append('\'');
     sb.append(", blockletId='").append(blockletId).append('\'');
     sb.append('}');

Review comment:
       ditto

##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/datatype/Field.java
##########
@@ -193,6 +192,7 @@ public void setParent(String parent) {
   }
 
   public String getStoreType() {
+    String storeType = "columnar";

Review comment:
       Q1: why this change? why not mark the string as static instead?
   Q2: this method is never used

##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/CarbonTable.java
##########
@@ -722,15 +719,11 @@ public int getBlockletSizeInMB() {
   public String getBucketHashMethod() {
     String configuredMethod = tableInfo.getFactTable().getTableProperties()
         .get(CarbonCommonConstants.BUCKET_HASH_METHOD);
-    if (configuredMethod == null) {
-      return CarbonCommonConstants.BUCKET_HASH_METHOD_DEFAULT;
-    } else {
-      if (CarbonCommonConstants.BUCKET_HASH_METHOD_NATIVE.equals(configuredMethod)) {
-        return CarbonCommonConstants.BUCKET_HASH_METHOD_NATIVE;
-      }
-      // by default we use spark_hash_expression hash method
-      return CarbonCommonConstants.BUCKET_HASH_METHOD_DEFAULT;
+    if (CarbonCommonConstants.BUCKET_HASH_METHOD_NATIVE.equals(configuredMethod)) {

Review comment:
       should keep (not) null check




----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3868: [CARBONDATA-3889] Cleanup code in carbondata-core module

GitBox
In reply to this post by GitBox

CarbonDataQA1 commented on pull request #3868:
URL: https://github.com/apache/carbondata/pull/3868#issuecomment-665182981






----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] QiangCai commented on a change in pull request #3868: [CARBONDATA-3889] Cleanup code in carbondata-core module

GitBox
In reply to this post by GitBox

QiangCai commented on a change in pull request #3868:
URL: https://github.com/apache/carbondata/pull/3868#discussion_r462714824



##########
File path: core/src/main/java/org/apache/carbondata/core/datastore/page/SafeFixLengthColumnPage.java
##########
@@ -283,8 +283,8 @@ public BigDecimal getDecimal(int rowId) {
   @Override
   public byte[][] getByteArrayPage() {
     byte[][] data = new byte[arrayElementCount][];
-    for (int i = 0; i < arrayElementCount; i++) {
-      data[i] = fixedLengthData[i];
+    if (arrayElementCount >= 0) {

Review comment:
       change to "if (arrayElementCount = 0) {"

##########
File path: core/src/main/java/org/apache/carbondata/core/datastore/page/SafeFixLengthColumnPage.java
##########
@@ -283,8 +283,8 @@ public BigDecimal getDecimal(int rowId) {
   @Override
   public byte[][] getByteArrayPage() {
     byte[][] data = new byte[arrayElementCount][];
-    for (int i = 0; i < arrayElementCount; i++) {
-      data[i] = fixedLengthData[i];
+    if (arrayElementCount >= 0) {

Review comment:
       change to "if (arrayElementCount > 0) {"

##########
File path: core/src/main/java/org/apache/carbondata/core/index/IndexMeta.java
##########
@@ -78,7 +78,7 @@ public String toString() {
     return new StringBuilder("IndexMeta{")
         .append("indexName='").append(indexName).append('\'')
         .append(", indexedColumns=[")
-        .append(StringUtils.join(getIndexedColumnNames(), ", ")).append("]\'")
+        .append(StringUtils.join(getIndexedColumnNames(), ", ")).append("]'")
         .append(", optimizedOperation=").append(optimizedOperation)
         .append('}')
         .toString();

Review comment:
       done

##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/datatype/Field.java
##########
@@ -193,6 +192,7 @@ public void setParent(String parent) {
   }
 
   public String getStoreType() {
+    String storeType = "columnar";

Review comment:
       removed this method

##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/CarbonTable.java
##########
@@ -722,15 +719,11 @@ public int getBlockletSizeInMB() {
   public String getBucketHashMethod() {
     String configuredMethod = tableInfo.getFactTable().getTableProperties()
         .get(CarbonCommonConstants.BUCKET_HASH_METHOD);
-    if (configuredMethod == null) {
-      return CarbonCommonConstants.BUCKET_HASH_METHOD_DEFAULT;
-    } else {
-      if (CarbonCommonConstants.BUCKET_HASH_METHOD_NATIVE.equals(configuredMethod)) {
-        return CarbonCommonConstants.BUCKET_HASH_METHOD_NATIVE;
-      }
-      // by default we use spark_hash_expression hash method
-      return CarbonCommonConstants.BUCKET_HASH_METHOD_DEFAULT;
+    if (CarbonCommonConstants.BUCKET_HASH_METHOD_NATIVE.equals(configuredMethod)) {

Review comment:
       "==" and "instanceof" contain null check

##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/CarbonTable.java
##########
@@ -722,15 +719,11 @@ public int getBlockletSizeInMB() {
   public String getBucketHashMethod() {
     String configuredMethod = tableInfo.getFactTable().getTableProperties()
         .get(CarbonCommonConstants.BUCKET_HASH_METHOD);
-    if (configuredMethod == null) {
-      return CarbonCommonConstants.BUCKET_HASH_METHOD_DEFAULT;
-    } else {
-      if (CarbonCommonConstants.BUCKET_HASH_METHOD_NATIVE.equals(configuredMethod)) {
-        return CarbonCommonConstants.BUCKET_HASH_METHOD_NATIVE;
-      }
-      // by default we use spark_hash_expression hash method
-      return CarbonCommonConstants.BUCKET_HASH_METHOD_DEFAULT;
+    if (CarbonCommonConstants.BUCKET_HASH_METHOD_NATIVE.equals(configuredMethod)) {

Review comment:
       Both "==" and "instanceof" contain null check

##########
File path: core/src/main/java/org/apache/carbondata/core/indexstore/Blocklet.java
##########
@@ -105,20 +106,18 @@ public boolean equals(Object o) {
 
     Blocklet blocklet = (Blocklet) o;
 
-    if (filePath != null ? !filePath.equals(blocklet.filePath) : blocklet.filePath != null) {
+    if (!Objects.equals(filePath, blocklet.filePath)) {
       return false;
     }
     if (!compareBlockletIdForObjectMatching) {
       return true;
     }
-    return blockletId != null ?
-        blockletId.equals(blocklet.blockletId) :
-        blocklet.blockletId == null;
+    return Objects.equals(blockletId, blocklet.blockletId);
   }
 
   @Override
   public String toString() {
-    final StringBuffer sb = new StringBuffer("Blocklet{");
+    final StringBuilder sb = new StringBuilder("Blocklet{");
     sb.append("filePath='").append(filePath).append('\'');
     sb.append(", blockletId='").append(blockletId).append('\'');
     sb.append('}');

Review comment:
       done




----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] kevinjmh commented on a change in pull request #3868: [CARBONDATA-3889] Cleanup code in carbondata-core module

GitBox
In reply to this post by GitBox

kevinjmh commented on a change in pull request #3868:
URL: https://github.com/apache/carbondata/pull/3868#discussion_r462763715



##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/CarbonTable.java
##########
@@ -722,15 +719,11 @@ public int getBlockletSizeInMB() {
   public String getBucketHashMethod() {
     String configuredMethod = tableInfo.getFactTable().getTableProperties()
         .get(CarbonCommonConstants.BUCKET_HASH_METHOD);
-    if (configuredMethod == null) {
-      return CarbonCommonConstants.BUCKET_HASH_METHOD_DEFAULT;
-    } else {
-      if (CarbonCommonConstants.BUCKET_HASH_METHOD_NATIVE.equals(configuredMethod)) {
-        return CarbonCommonConstants.BUCKET_HASH_METHOD_NATIVE;
-      }
-      // by default we use spark_hash_expression hash method
-      return CarbonCommonConstants.BUCKET_HASH_METHOD_DEFAULT;
+    if (CarbonCommonConstants.BUCKET_HASH_METHOD_NATIVE.equals(configuredMethod)) {

Review comment:
       ok




----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3868: [CARBONDATA-3889] Cleanup code in carbondata-core module

GitBox
In reply to this post by GitBox

CarbonDataQA1 commented on pull request #3868:
URL: https://github.com/apache/carbondata/pull/3868#issuecomment-666183239






----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] kevinjmh commented on pull request #3868: [CARBONDATA-3889] Cleanup code in carbondata-core module

GitBox
In reply to this post by GitBox

kevinjmh commented on pull request #3868:
URL: https://github.com/apache/carbondata/pull/3868#issuecomment-666331953


   LGTM


----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] asfgit closed pull request #3868: [CARBONDATA-3889] Cleanup code in carbondata-core module

GitBox
In reply to this post by GitBox

asfgit closed pull request #3868:
URL: https://github.com/apache/carbondata/pull/3868


   


----------------------------------------------------------------
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:
[hidden email]