[GitHub] carbondata pull request #2113: [WIP][LUCENE_DATAMAP]load issue in lucene dat...

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2113
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/3777/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2113#discussion_r181394048
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -1642,6 +1642,16 @@
     
       public static final String CARBON_SEARCH_MODE_THREAD_DEFAULT = "3";
     
    +  /**
    +   * compression mode used by lucene for index writing
    +   */
    +  public static final String CARBON_LUCENE_COMPRESSION_MODE = "carbon.lucene.compression.mode";
    --- End diff --
   
    what are the options available for this property?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2113#discussion_r181396888
 
    --- Diff: datamap/lucene/pom.xml ---
    @@ -141,6 +141,34 @@
               </execution>
             </executions>
           </plugin>
    +      <plugin>
    --- End diff --
   
    I realize that in this pom, it should not depend on carbon-spark2, please modify the dependency in this pom


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2113#discussion_r181397988
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/CarbonEnv.scala ---
    @@ -173,6 +174,10 @@ object CarbonEnv {
           .addListener(classOf[AlterTableDropPartitionPostStatusEvent],
             AlterTableDropPartitionPostStatusListener)
           .addListener(classOf[AlterTableDropPartitionMetaEvent], AlterTableDropPartitionMetaListener)
    +      .addListener(classOf[AlterTableRenamePreEvent], LuceneRenameTablePreListener)
    --- End diff --
   
    Is this required? Ideally, lucene datamap is a separate module which should not have intrusive modification in other modules


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2113#discussion_r181399425
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/datamap/CarbonCreateDataMapCommand.scala ---
    @@ -69,11 +69,33 @@ case class CarbonCreateDataMapCommand(
         }
     
         dataMapSchema = new DataMapSchema(dataMapName, dmClassName)
    -    if (mainTable != null &&
    -        mainTable.isStreamingTable &&
    -        !(dataMapSchema.getProviderName.equalsIgnoreCase(DataMapClassProvider.PREAGGREGATE.toString)
    -          || dataMapSchema.getProviderName
    -            .equalsIgnoreCase(DataMapClassProvider.TIMESERIES.toString))) {
    +    if (dataMapSchema.getProviderName.equalsIgnoreCase(DataMapClassProvider.LUCENEFG.toString) ||
    --- End diff --
   
    I think we should abstract interface for it. We can not add if check for every new datamap added


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2113#discussion_r181437595
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -1642,6 +1642,16 @@
     
       public static final String CARBON_SEARCH_MODE_THREAD_DEFAULT = "3";
     
    +  /**
    +   * compression mode used by lucene for index writing
    +   */
    +  public static final String CARBON_LUCENE_COMPRESSION_MODE = "carbon.lucene.compression.mode";
    --- End diff --
   
    SPEED and COMPRESSION, by default the property value will be SPEED


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2113#discussion_r181438240
 
    --- Diff: datamap/lucene/pom.xml ---
    @@ -141,6 +141,34 @@
               </execution>
             </executions>
           </plugin>
    +      <plugin>
    --- End diff --
   
    this was added to include test suite in main CI


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2113#discussion_r181439768
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/CarbonEnv.scala ---
    @@ -173,6 +174,10 @@ object CarbonEnv {
           .addListener(classOf[AlterTableDropPartitionPostStatusEvent],
             AlterTableDropPartitionPostStatusListener)
           .addListener(classOf[AlterTableDropPartitionMetaEvent], AlterTableDropPartitionMetaListener)
    +      .addListener(classOf[AlterTableRenamePreEvent], LuceneRenameTablePreListener)
    --- End diff --
   
    this listener class is added to block alter operation on lucene datamap, if we are blocking alter operation for all the datamaps, then this may not be required.


---

[qiuchenjian-2]

Github user akashrn5 commented on the issue:

    https://github.com/apache/carbondata/pull/2113
 
    retest sdv please


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2113#discussion_r181629595
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datamap/DataMapStoreManager.java ---
    @@ -246,17 +247,24 @@ public TableDataMap getDataMap(CarbonTable table, DataMapSchema dataMapSchema) {
       public TableDataMap createAndRegisterDataMap(CarbonTable table,
           DataMapSchema dataMapSchema) throws MalformedDataMapCommandException, IOException {
         DataMapFactory dataMapFactory;
    -    try {
    -      // try to create datamap by reflection to test whether it is a valid DataMapFactory class
    -      Class<? extends DataMapFactory> factoryClass =
    -          (Class<? extends DataMapFactory>) Class.forName(dataMapSchema.getProviderName());
    -      dataMapFactory = factoryClass.newInstance();
    -    } catch (ClassNotFoundException e) {
    -      throw new MalformedDataMapCommandException(
    -          "DataMap '" + dataMapSchema.getProviderName() + "' not found");
    -    } catch (Throwable e) {
    -      throw new MetadataProcessException(
    -          "failed to create DataMap '" + dataMapSchema.getProviderName() + "'", e);
    +    if (dataMapSchema.getProviderName()
    +        .equalsIgnoreCase(DataMapClassProvider.LUCENEFG.getShortName()) || dataMapSchema
    +        .getProviderName().equalsIgnoreCase(DataMapClassProvider.LUCENECG.getShortName())) {
    --- End diff --
   
    Why is this special check? why can't it instantiated from old code?


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2113
 
    Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/3819/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2113#discussion_r181658482
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -1642,6 +1642,16 @@
     
       public static final String CARBON_SEARCH_MODE_THREAD_DEFAULT = "3";
     
    +  /**
    +   * compression mode used by lucene for index writing
    +   */
    +  public static final String CARBON_LUCENE_COMPRESSION_MODE = "carbon.lucene.compression.mode";
    --- End diff --
   
    mention it in the description in line 1646


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2113#discussion_r181659113
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java ---
    @@ -681,10 +681,10 @@ private static void deletePhysicalPartition(List<PartitionSpec> partitionSpecs,
               FileFactory.deleteAllCarbonFilesOfDir(FileFactory.getCarbonFile(location.toString()));
             }
           } else {
    -        // delete the segment folder if it is empty
    +        // delete the segment folder
             CarbonFile file = FileFactory.getCarbonFile(location.toString());
    --- End diff --
   
    change `file` to `segmentPath`


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2113#discussion_r181660289
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/CarbonTable.java ---
    @@ -856,6 +858,25 @@ public boolean hasAggregationDataMap() {
         return false;
       }
     
    +  /**
    +   * whether this table has Lucene DataMap or not
    +   */
    +  public boolean hasLuceneDataMap() {
    --- End diff --
   
    remove this now, we should add blocking feature in other PR


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2113#discussion_r181660460
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/DataMapSchema.java ---
    @@ -143,11 +142,12 @@ public void setChildSchema(TableSchema childSchema) {
     
       /**
        * Return true if this datamap is an Index DataMap
    +   *
    --- End diff --
   
    no need to modify


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2113#discussion_r181660484
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/DataMapSchema.java ---
    @@ -143,11 +142,12 @@ public void setChildSchema(TableSchema childSchema) {
     
       /**
        * Return true if this datamap is an Index DataMap
    +   *
        * @return
        */
       public boolean isIndexDataMap() {
    -    if (providerName.equalsIgnoreCase(DataMapClassProvider.PREAGGREGATE.getShortName()) ||
    -        providerName.equalsIgnoreCase(DataMapClassProvider.TIMESERIES.getShortName())) {
    +    if (providerName.equalsIgnoreCase(DataMapClassProvider.PREAGGREGATE.getShortName())
    +        || providerName.equalsIgnoreCase(DataMapClassProvider.TIMESERIES.getShortName())) {
    --- End diff --
   
    no need to modify


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2113#discussion_r181661362
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/DiskBasedDMSchemaStorageProvider.java ---
    @@ -129,9 +130,9 @@ public DiskBasedDMSchemaStorageProvider(String storePath) {
         return dataMapSchemas;
       }
     
    -  @Override public void dropSchema(String dataMapName) throws IOException {
    -    String schemaPath =
    -        storePath + CarbonCommonConstants.FILE_SEPARATOR + dataMapName + ".dmschema";
    +  @Override public void dropSchema(String dataMapName,String tableName) throws IOException {
    +    String schemaPath = storePath + CarbonCommonConstants.FILE_SEPARATOR + tableName
    +        + CarbonCommonConstants.UNDERSCORE + dataMapName + ".dmschema";
    --- End diff --
   
    I think it is better to record the datamap short name in the path also, so that we can know what kind of datamap it is by looking at the file name


---