[GitHub] carbondata pull request #1418: Support db location

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1418
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/905/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1418
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1520/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1418
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/910/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1418
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1525/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1418
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/918/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1418
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/920/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1418
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/921/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1418
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/925/



---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1418#discussion_r149966044
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -1377,6 +1377,10 @@
     
       public static final String CARBON_USE_BLOCKLET_DISTRIBUTION_DEFAULT = "true";
     
    +  public static final String CARBON_UPDATE_SYNC_FOLDER = "carbon.update.sync.folder";
    --- End diff --
   
    Add comment here for usage.


---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1418#discussion_r149966152
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -1377,6 +1377,10 @@
     
       public static final String CARBON_USE_BLOCKLET_DISTRIBUTION_DEFAULT = "true";
     
    +  public static final String CARBON_UPDATE_SYNC_FOLDER = "carbon.update.sync.folder";
    +
    +  public static final String CARBON_UPDATE_SYNC_FOLDER_DEFAULT = "/tmp/carbondata";
    --- End diff --
   
    Add annotation


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1418
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1533/



---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1418#discussion_r149971236
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/locks/HdfsFileLock.java ---
    @@ -47,26 +44,12 @@
     
       private static String tmpPath;
     
    -  static {
    --- End diff --
   
    Use the mdtfile path to use systemlocking


---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1418#discussion_r149971910
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/locks/LocalFileLock.java ---
    @@ -90,8 +90,9 @@ public LocalFileLock(String lockFileLocation, String lockFile) {
        * @param tableIdentifier
        * @param lockFile
        */
    -  public LocalFileLock(CarbonTableIdentifier tableIdentifier, String lockFile) {
    -    this(tableIdentifier.getDatabaseName() + CarbonCommonConstants.FILE_SEPARATOR + tableIdentifier
    +  public LocalFileLock(AbsoluteTableIdentifier tableIdentifier, String lockFile) {
    +    this(tableIdentifier.getCarbonTableIdentifier().getDatabaseName()
    --- End diff --
   
    Don't use the storelocation  to create path, use tablepath


---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1418#discussion_r149977104
 
    --- Diff: hadoop/src/main/java/org/apache/carbondata/hadoop/api/CarbonTableInputFormat.java ---
    @@ -885,4 +882,24 @@ public static DataTypeConverter getDataTypeConverter(Configuration configuration
         }
         return (DataTypeConverter) ObjectSerializationUtil.convertStringToObject(converter);
       }
    +
    +  public static void setDatabaseName(Configuration configuration, String databaseName) {
    +    if (null != databaseName) {
    +      configuration.set(DATABASE_NAME, databaseName);
    +    }
    +  }
    +
    +  public static String getDatabaseName(Configuration configuration) {
    +    return configuration.get(DATABASE_NAME);
    +  }
    +
    +  public static void setTableName(Configuration configuration, String tableName) {
    +    if (null != tableName) {
    +      configuration.set(TABLE_NAME, tableName);
    +    }
    +  }
    +
    +  public static String getTableName(Configuration configuration) {
    +    return configuration.get(TABLE_NAME);
    --- End diff --
   
    Throw exception if not set  for dbname and tablename


---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1418#discussion_r149979447
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/impl/CarbonTableReader.java ---
    @@ -271,14 +271,20 @@ private CarbonTable parseCarbonMetadata(SchemaTableName table) {
           // If table is not previously cached, then:
     
           // Step 1: get store path of the table and cache it.
    -      String storePath = config.getStorePath();
           // create table identifier. the table id is randomly generated.
           cache.carbonTableIdentifier =
               new CarbonTableIdentifier(table.getSchemaName(), table.getTableName(),
                   UUID.randomUUID().toString());
    +      String storePath = config.getStorePath();
    --- End diff --
   
    Add TODO saying that developer should pass table path not store path


---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1418#discussion_r149979665
 
    --- Diff: integration/presto/src/test/scala/org/apache/carbondata/presto/integrationtest/PrestoAllDataTypeTest.scala ---
    @@ -31,7 +31,7 @@ class PrestoAllDataTypeTest extends FunSuiteLike with BeforeAndAfterAll {
         .getLogService(classOf[PrestoAllDataTypeTest].getCanonicalName)
     
       private val rootPath = new File(this.getClass.getResource("/").getPath
    -                                  + "../../../..").getCanonicalPath
    +                                  + "../../../..").getCanonicalPath.replaceAll("\\\\", "/")
    --- End diff --
   
    please remove if not required


---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1418#discussion_r149981553
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/carbondata/spark/rdd/CarbonMergerRDD.scala ---
    @@ -167,7 +167,7 @@ class CarbonMergerRDD[K, V](
             val dataFileMetadataSegMapping: java.util.Map[String, List[DataFileFooter]] =
               CarbonCompactionUtil.createDataFileFooterMappingForSegments(tableBlockInfoList)
     
    -        carbonLoadModel.setStorePath(hdfsStoreLocation)
    +        carbonLoadModel.setTablePath(hdfsStoreLocation)
    --- End diff --
   
    change name to hdfstablepath


---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1418#discussion_r149982091
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/carbondata/spark/rdd/CarbonMergerRDD.scala ---
    @@ -167,7 +167,7 @@ class CarbonMergerRDD[K, V](
             val dataFileMetadataSegMapping: java.util.Map[String, List[DataFileFooter]] =
    --- End diff --
   
    remove storepath from CarbonMergerMapping if nobody is using


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1418
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1535/



---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1418#discussion_r149993636
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/carbondata/spark/util/CommonUtil.scala ---
    @@ -839,14 +841,56 @@ object CommonUtil {
        */
       def mergeIndexFiles(sparkContext: SparkContext,
           segmentIds: Seq[String],
    -      storePath: String,
    +      tablePath: String,
           carbonTable: CarbonTable): Unit = {
         if (CarbonProperties.getInstance().getProperty(
           CarbonCommonConstants.CARBON_MERGE_INDEX_IN_SEGMENT,
           CarbonCommonConstants.CARBON_MERGE_INDEX_IN_SEGMENT_DEFAULT).toBoolean) {
    -      new CarbonMergeFilesRDD(sparkContext, AbsoluteTableIdentifier.from(storePath,
    +      new CarbonMergeFilesRDD(sparkContext, AbsoluteTableIdentifier.from(tablePath,
             carbonTable.getDatabaseName, carbonTable.getFactTableName).getTablePath,
             segmentIds).collect()
         }
       }
    +
    +  /**
    +   * The default database and databases ending with .db will point to the
    +   * either either "carbon.storeLocation" or "spark.sql.warehouse.dir".
    +   *
    +   * @param storeLocation
    +   * @param databaseName
    +   * @param fixedStorePath
    +   * @return
    +   */
    +  def getValidStorePath(storeLocation: String, databaseName: String,
    +      fixedStorePath: String): String = {
    --- End diff --
   
    Use ends with .db to decide whether to use dblocation or storepath


---