[GitHub] [carbondata] kunal642 opened a new pull request #3697: [HOTFIX] Refactored hive related classes to use constants

[GitBox]

kunal642 opened a new pull request #3697: [HOTFIX] Refactored hive related classes to use constants
URL: https://github.com/apache/carbondata/pull/3697
 
 
    ### Why is this PR needed?
   Use hiveConstants instead of harcoding the values.
   
    ### What changes were proposed in this PR?
   Refactor code for better understanding
       
    ### 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]


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3697: [HOTFIX] Refactored hive related classes to use constants
URL: https://github.com/apache/carbondata/pull/3697#issuecomment-609678229
 
 
   Build Failed  with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/939/
   

----------------------------------------------------------------
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]


With regards,
Apache Git Services

[GitBox]

akashrn5 commented on a change in pull request #3697: [HOTFIX] Refactored hive related classes to use constants
URL: https://github.com/apache/carbondata/pull/3697#discussion_r403961085
 
 

 ##########
 File path: integration/hive/src/main/java/org/apache/carbondata/hive/MapredCarbonOutputFormat.java
 ##########
 @@ -61,24 +68,25 @@ public void checkOutputSpecs(FileSystem fileSystem, JobConf jobConf) throws IOEx
   public FileSinkOperator.RecordWriter getHiveRecordWriter(JobConf jc, Path finalOutPath,
       Class<? extends Writable> valueClass, boolean isCompressed, Properties tableProperties,
       Progressable progress) throws IOException {
+    ThreadLocalSessionInfo.setConfigurationToCurrentThread(jc);
     CarbonLoadModel carbonLoadModel = null;
-    String encodedString = jc.get(LOAD_MODEL);
+    // Take carbonLoadModel from container environment if set.
 
 Review comment:
   i think here first we should try to get directly from conf, if we dont get then we can try from systemEnv, it will serve all

----------------------------------------------------------------
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]


With regards,
Apache Git Services

[GitBox]

akashrn5 commented on a change in pull request #3697: [HOTFIX] Refactored hive related classes to use constants
URL: https://github.com/apache/carbondata/pull/3697#discussion_r403959294
 
 

 ##########
 File path: integration/hive/src/main/java/org/apache/carbondata/hive/MapredCarbonInputFormat.java
 ##########
 @@ -94,8 +95,9 @@ private static void populateCarbonTable(Configuration configuration, String path
       // persisted in the schema
       CarbonTable carbonTable;
       AbsoluteTableIdentifier absoluteTableIdentifier = AbsoluteTableIdentifier
-          .from(validInputPath, getDatabaseName(configuration), getTableName(configuration));
-      String schemaPath = CarbonTablePath.getSchemaFilePath(validInputPath);
+          .from(configuration.get(hive_metastoreConstants.META_TABLE_LOCATION),
+              getDatabaseName(configuration), getTableName(configuration));
+      String schemaPath = CarbonTablePath.getSchemaFilePath(absoluteTableIdentifier.getTablePath());
 
 Review comment:
   pass the configuration also to `getSchemaFilePath` method

----------------------------------------------------------------
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]


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3697: [HOTFIX] Refactored hive related classes to use constants
URL: https://github.com/apache/carbondata/pull/3697#issuecomment-609755799
 
 
   Build Failed  with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2650/
   

----------------------------------------------------------------
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]


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3697: [HOTFIX] Refactored hive related classes to use constants
URL: https://github.com/apache/carbondata/pull/3697#issuecomment-609763528
 
 
   Build Failed  with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/940/
   

----------------------------------------------------------------
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]


With regards,
Apache Git Services

[GitBox]

Indhumathi27 commented on a change in pull request #3697: [HOTFIX] Refactored hive related classes to use constants
URL: https://github.com/apache/carbondata/pull/3697#discussion_r404563047
 
 

 ##########
 File path: integration/hive/src/main/java/org/apache/carbondata/hive/MapredCarbonInputFormat.java
 ##########
 @@ -94,8 +95,10 @@ private static void populateCarbonTable(Configuration configuration, String path
       // persisted in the schema
       CarbonTable carbonTable;
       AbsoluteTableIdentifier absoluteTableIdentifier = AbsoluteTableIdentifier
-          .from(validInputPath, getDatabaseName(configuration), getTableName(configuration));
-      String schemaPath = CarbonTablePath.getSchemaFilePath(validInputPath);
+          .from(configuration.get(hive_metastoreConstants.META_TABLE_LOCATION),
 
 Review comment:
   Looks like `validInputPath` is never used. Please remove this variable

----------------------------------------------------------------
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]


With regards,
Apache Git Services

[GitBox]

Indhumathi27 commented on a change in pull request #3697: [HOTFIX] Refactored hive related classes to use constants
URL: https://github.com/apache/carbondata/pull/3697#discussion_r404561694
 
 

 ##########
 File path: integration/hive/src/main/java/org/apache/carbondata/hive/MapredCarbonOutputFormat.java
 ##########
 @@ -61,24 +68,32 @@ public void checkOutputSpecs(FileSystem fileSystem, JobConf jobConf) throws IOEx
   public FileSinkOperator.RecordWriter getHiveRecordWriter(JobConf jc, Path finalOutPath,
       Class<? extends Writable> valueClass, boolean isCompressed, Properties tableProperties,
       Progressable progress) throws IOException {
+    ThreadLocalSessionInfo.setConfigurationToCurrentThread(jc);
     CarbonLoadModel carbonLoadModel = null;
+    // Take carbonloadmodel from jobConf is it is set.
 
 Review comment:
   Please change the comment

----------------------------------------------------------------
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]


With regards,
Apache Git Services

[GitBox]

kunal642 commented on a change in pull request #3697: [HOTFIX] Refactored hive related classes to use constants
URL: https://github.com/apache/carbondata/pull/3697#discussion_r404567676
 
 

 ##########
 File path: integration/hive/src/main/java/org/apache/carbondata/hive/MapredCarbonInputFormat.java
 ##########
 @@ -94,8 +95,9 @@ private static void populateCarbonTable(Configuration configuration, String path
       // persisted in the schema
       CarbonTable carbonTable;
       AbsoluteTableIdentifier absoluteTableIdentifier = AbsoluteTableIdentifier
-          .from(validInputPath, getDatabaseName(configuration), getTableName(configuration));
-      String schemaPath = CarbonTablePath.getSchemaFilePath(validInputPath);
+          .from(configuration.get(hive_metastoreConstants.META_TABLE_LOCATION),
+              getDatabaseName(configuration), getTableName(configuration));
+      String schemaPath = CarbonTablePath.getSchemaFilePath(absoluteTableIdentifier.getTablePath());
 
 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]


With regards,
Apache Git Services

[GitBox]

kunal642 commented on a change in pull request #3697: [HOTFIX] Refactored hive related classes to use constants
URL: https://github.com/apache/carbondata/pull/3697#discussion_r404567692
 
 

 ##########
 File path: integration/hive/src/main/java/org/apache/carbondata/hive/MapredCarbonOutputFormat.java
 ##########
 @@ -61,24 +68,25 @@ public void checkOutputSpecs(FileSystem fileSystem, JobConf jobConf) throws IOEx
   public FileSinkOperator.RecordWriter getHiveRecordWriter(JobConf jc, Path finalOutPath,
       Class<? extends Writable> valueClass, boolean isCompressed, Properties tableProperties,
       Progressable progress) throws IOException {
+    ThreadLocalSessionInfo.setConfigurationToCurrentThread(jc);
     CarbonLoadModel carbonLoadModel = null;
-    String encodedString = jc.get(LOAD_MODEL);
+    // Take carbonLoadModel from container environment if set.
 
 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]


With regards,
Apache Git Services

[GitBox]

kunal642 commented on a change in pull request #3697: [HOTFIX] Refactored hive related classes to use constants
URL: https://github.com/apache/carbondata/pull/3697#discussion_r404567716
 
 

 ##########
 File path: integration/hive/src/main/java/org/apache/carbondata/hive/MapredCarbonOutputFormat.java
 ##########
 @@ -61,24 +68,32 @@ public void checkOutputSpecs(FileSystem fileSystem, JobConf jobConf) throws IOEx
   public FileSinkOperator.RecordWriter getHiveRecordWriter(JobConf jc, Path finalOutPath,
       Class<? extends Writable> valueClass, boolean isCompressed, Properties tableProperties,
       Progressable progress) throws IOException {
+    ThreadLocalSessionInfo.setConfigurationToCurrentThread(jc);
     CarbonLoadModel carbonLoadModel = null;
+    // Take carbonloadmodel from jobConf is it is set.
 
 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]


With regards,
Apache Git Services

[GitBox]

kunal642 commented on a change in pull request #3697: [HOTFIX] Refactored hive related classes to use constants
URL: https://github.com/apache/carbondata/pull/3697#discussion_r404567765
 
 

 ##########
 File path: integration/hive/src/main/java/org/apache/carbondata/hive/MapredCarbonInputFormat.java
 ##########
 @@ -94,8 +95,10 @@ private static void populateCarbonTable(Configuration configuration, String path
       // persisted in the schema
       CarbonTable carbonTable;
       AbsoluteTableIdentifier absoluteTableIdentifier = AbsoluteTableIdentifier
-          .from(validInputPath, getDatabaseName(configuration), getTableName(configuration));
-      String schemaPath = CarbonTablePath.getSchemaFilePath(validInputPath);
+          .from(configuration.get(hive_metastoreConstants.META_TABLE_LOCATION),
 
 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]


With regards,
Apache Git Services

[GitBox]

kunal642 commented on a change in pull request #3697: [HOTFIX] Refactored hive related classes to use constants
URL: https://github.com/apache/carbondata/pull/3697#discussion_r404567797
 
 

 ##########
 File path: integration/hive/src/main/java/org/apache/carbondata/hive/MapredCarbonOutputFormat.java
 ##########
 @@ -61,24 +68,32 @@ public void checkOutputSpecs(FileSystem fileSystem, JobConf jobConf) throws IOEx
   public FileSinkOperator.RecordWriter getHiveRecordWriter(JobConf jc, Path finalOutPath,
       Class<? extends Writable> valueClass, boolean isCompressed, Properties tableProperties,
       Progressable progress) throws IOException {
+    ThreadLocalSessionInfo.setConfigurationToCurrentThread(jc);
     CarbonLoadModel carbonLoadModel = null;
+    // Take carbonloadmodel from jobConf is it is set.
 
 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]


With regards,
Apache Git Services

[GitBox]

kunal642 commented on a change in pull request #3697: [HOTFIX] Refactored hive related classes to use constants
URL: https://github.com/apache/carbondata/pull/3697#discussion_r404567848
 
 

 ##########
 File path: integration/hive/src/main/java/org/apache/carbondata/hive/MapredCarbonInputFormat.java
 ##########
 @@ -94,8 +95,9 @@ private static void populateCarbonTable(Configuration configuration, String path
       // persisted in the schema
       CarbonTable carbonTable;
       AbsoluteTableIdentifier absoluteTableIdentifier = AbsoluteTableIdentifier
-          .from(validInputPath, getDatabaseName(configuration), getTableName(configuration));
-      String schemaPath = CarbonTablePath.getSchemaFilePath(validInputPath);
+          .from(configuration.get(hive_metastoreConstants.META_TABLE_LOCATION),
+              getDatabaseName(configuration), getTableName(configuration));
+      String schemaPath = CarbonTablePath.getSchemaFilePath(absoluteTableIdentifier.getTablePath());
 
 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]


With regards,
Apache Git Services

[GitBox]

kunal642 commented on a change in pull request #3697: [HOTFIX] Refactored hive related classes to use constants
URL: https://github.com/apache/carbondata/pull/3697#discussion_r404567876
 
 

 ##########
 File path: integration/hive/src/main/java/org/apache/carbondata/hive/MapredCarbonOutputFormat.java
 ##########
 @@ -61,24 +68,25 @@ public void checkOutputSpecs(FileSystem fileSystem, JobConf jobConf) throws IOEx
   public FileSinkOperator.RecordWriter getHiveRecordWriter(JobConf jc, Path finalOutPath,
       Class<? extends Writable> valueClass, boolean isCompressed, Properties tableProperties,
       Progressable progress) throws IOException {
+    ThreadLocalSessionInfo.setConfigurationToCurrentThread(jc);
     CarbonLoadModel carbonLoadModel = null;
-    String encodedString = jc.get(LOAD_MODEL);
+    // Take carbonLoadModel from container environment if set.
 
 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]


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3697: [HOTFIX] Refactored hive related classes to use constants
URL: https://github.com/apache/carbondata/pull/3697#issuecomment-610205708
 
 
   Build Failed  with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/950/
   

----------------------------------------------------------------
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]


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3697: [HOTFIX] Refactored hive related classes to use constants
URL: https://github.com/apache/carbondata/pull/3697#issuecomment-610206239
 
 
   Build Failed  with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2660/
   

----------------------------------------------------------------
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]


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3697: [HOTFIX] Refactored hive related classes to use constants
URL: https://github.com/apache/carbondata/pull/3697#issuecomment-610275632
 
 
   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/952/
   

----------------------------------------------------------------
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]


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3697: [HOTFIX] Refactored hive related classes to use constants
URL: https://github.com/apache/carbondata/pull/3697#issuecomment-610277405
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2662/
   

----------------------------------------------------------------
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]


With regards,
Apache Git Services

[GitBox]

ajantha-bhat commented on a change in pull request #3697: [HOTFIX] Refactored hive related classes to use constants
URL: https://github.com/apache/carbondata/pull/3697#discussion_r404704207
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/util/CarbonProperties.java
 ##########
 @@ -855,7 +855,11 @@ private void loadProperties() {
    * Return the store path
    */
   public static String getStorePath() {
-    return getInstance().getProperty(CarbonCommonConstants.STORE_LOCATION);
+    String storePath = getInstance().getProperty(CarbonCommonConstants.STORE_LOCATION);
+    if (storePath == null) {
 
 Review comment:
   what about `spark.warehouse.dir` ??
   
   and what if `hive.metastore.warehouse.dir` is not configured ?

----------------------------------------------------------------
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]


With regards,
Apache Git Services