[GitHub] [carbondata] Indhumathi27 opened a new pull request #3688: [WIP] Refactor Index Metadata

[GitBox]

CarbonDataQA1 commented on issue #3688: [CARBONDATA-3765] Refactor Index Metadata for CG and FG Indexes
URL: https://github.com/apache/carbondata/pull/3688#issuecomment-608550918
 
 
   Build Failed  with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2632/
   

----------------------------------------------------------------
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 #3688: [CARBONDATA-3765] Refactor Index Metadata for CG and FG Indexes
URL: https://github.com/apache/carbondata/pull/3688#issuecomment-608574551
 
 
   Build Failed  with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/923/
   

----------------------------------------------------------------
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 #3688: [CARBONDATA-3765] Refactor Index Metadata for CG and FG Indexes
URL: https://github.com/apache/carbondata/pull/3688#issuecomment-608621910
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2636/
   

----------------------------------------------------------------
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 #3688: [CARBONDATA-3765] Refactor Index Metadata for CG and FG Indexes
URL: https://github.com/apache/carbondata/pull/3688#issuecomment-608625942
 
 
   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/927/
   

----------------------------------------------------------------
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 #3688: [CARBONDATA-3765] Refactor Index Metadata for CG and FG Indexes
URL: https://github.com/apache/carbondata/pull/3688#discussion_r405272670
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/index/IndexChooser.java
 ##########
 @@ -68,15 +67,14 @@
   public IndexChooser(CarbonTable carbonTable) throws IOException {
     this.carbonTable = carbonTable;
     // read all indexes for this table and populate CG and FG index list
-    List<TableIndex> visibleIndexes =
-        DataMapStoreManager.getInstance().getAllVisibleIndexes(carbonTable);
-    Map<String, DataMapStatusDetail> map = DataMapStatusManager.readDataMapStatusMap();
+    List<TableIndex> visibleIndexes = carbonTable.getAllVisibleIndexes();
     cgIndexes = new ArrayList<>(visibleIndexes.size());
     fgIndexes = new ArrayList<>(visibleIndexes.size());
     for (TableIndex visibleIndex : visibleIndexes) {
-      DataMapStatusDetail status = map.get(visibleIndex.getDataMapSchema().getDataMapName());
-      if (status != null && status.isEnabled()) {
-        IndexLevel level = visibleIndex.getIndexFactory().getDataMapLevel();
+      if (visibleIndex.getIndexSchema().getProperties().get(CarbonCommonConstants.INDEX_STATUS)
 
 Review comment:
   How do you handle the compatibility during upgrade ? 1.6 already wrote in system folder. Once upgraded. Table will not have these properties.

----------------------------------------------------------------
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 #3688: [CARBONDATA-3765] Refactor Index Metadata for CG and FG Indexes
URL: https://github.com/apache/carbondata/pull/3688#discussion_r405268051
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java
 ##########
 @@ -1536,12 +1536,12 @@ private CarbonCommonConstants() {
   //////////////////////////////////////////////////////////////////////////////////////////
 
   /**
-   * key prefix for set command. 'carbon.datamap.visible.dbName.tableName.dmName = false' means
+   * key prefix for set command. 'carbon.index.visible.dbName.tableName.dmName = false' means
    * that the query on 'dbName.table' will not use the datamap 'dmName'
 
 Review comment:
   ```suggestion
      * that the query on 'dbName.table' will not use the index 'dmName'
   ```

----------------------------------------------------------------
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 #3688: [CARBONDATA-3765] Refactor Index Metadata for CG and FG Indexes
URL: https://github.com/apache/carbondata/pull/3688#discussion_r405279772
 
 

 ##########
 File path: format/src/main/thrift/schema.thrift
 ##########
 @@ -203,9 +203,9 @@ struct ParentColumnTableRelation {
    3: required string columnName
 }
 
-struct DataMapSchema  {
+struct IndexSchema  {
 
 Review comment:
   @niuge01 , @QiangCai : please check in previous merged PR also for compatibility issues. better to test old store once.

----------------------------------------------------------------
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 #3688: [CARBONDATA-3765] Refactor Index Metadata for CG and FG Indexes
URL: https://github.com/apache/carbondata/pull/3688#discussion_r405274752
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/index/IndexStoreManager.java
 ##########
 @@ -820,8 +715,11 @@ public synchronized void clearInvalidIndex(CarbonTable carbonTable, List<String>
   }
 
   private boolean hasCGIndex(CarbonTable carbonTable) throws IOException {
-    for (TableIndex tableIndex : getAllVisibleIndexes(carbonTable)) {
-      if (tableIndex.getIndexFactory().getDataMapLevel().equals(IndexLevel.CG)) {
+    if (null == carbonTable) {
 
 Review comment:
   Table cannot be null here

----------------------------------------------------------------
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 #3688: [CARBONDATA-3765] Refactor Index Metadata for CG and FG Indexes
URL: https://github.com/apache/carbondata/pull/3688#discussion_r405273233
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java
 ##########
 @@ -1536,12 +1536,12 @@ private CarbonCommonConstants() {
   //////////////////////////////////////////////////////////////////////////////////////////
 
   /**
-   * key prefix for set command. 'carbon.datamap.visible.dbName.tableName.dmName = false' means
+   * key prefix for set command. 'carbon.index.visible.dbName.tableName.dmName = false' means
    * that the query on 'dbName.table' will not use the datamap 'dmName'
 
 Review comment:
   should have not combined refactoring and functionality PR together. It is hard to review now.

----------------------------------------------------------------
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 #3688: [CARBONDATA-3765] Refactor Index Metadata for CG and FG Indexes
URL: https://github.com/apache/carbondata/pull/3688#discussion_r405278856
 
 

 ##########
 File path: format/src/main/thrift/schema.thrift
 ##########
 @@ -203,9 +203,9 @@ struct ParentColumnTableRelation {
    3: required string columnName
 }
 
-struct DataMapSchema  {
+struct IndexSchema  {
 
 Review comment:
   Changing thrift member name will leads to compatibility issue. Old store will have as object of DataMapSchema, type cast may fail. Have tried reading old store that has datamap schema ?

----------------------------------------------------------------
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 issue #3688: [CARBONDATA-3765] Refactor Index Metadata for CG and FG Indexes
URL: https://github.com/apache/carbondata/pull/3688#issuecomment-610773012
 
 
   @Indhumathi27 : Just checked few files on high level. Couldn't focus on functionality as there is attracting 300 file rename changes. Suggest to separate rename and functionality(multi-tenant) PR

----------------------------------------------------------------
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 edited a comment on issue #3688: [CARBONDATA-3765] Refactor Index Metadata for CG and FG Indexes
URL: https://github.com/apache/carbondata/pull/3688#issuecomment-610773012
 
 
   @Indhumathi27 : Just checked few files on high level. Couldn't focus on functionality as there is distracting 300 file rename changes. Suggest to separate rename and functionality(multi-tenant) PR

----------------------------------------------------------------
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 issue #3688: [CARBONDATA-3765] Refactor Index Metadata for CG and FG Indexes
URL: https://github.com/apache/carbondata/pull/3688#issuecomment-610775682
 
 
   @ajantha-bhat there are seperate commits for support muti-tenant and datamap refactor in this PR. If still difficult to review, i can raise one more PR

----------------------------------------------------------------
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 #3688: [CARBONDATA-3765] Refactor Index Metadata for CG and FG Indexes
URL: https://github.com/apache/carbondata/pull/3688#discussion_r405286075
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/index/IndexChooser.java
 ##########
 @@ -68,15 +67,14 @@
   public IndexChooser(CarbonTable carbonTable) throws IOException {
     this.carbonTable = carbonTable;
     // read all indexes for this table and populate CG and FG index list
-    List<TableIndex> visibleIndexes =
-        DataMapStoreManager.getInstance().getAllVisibleIndexes(carbonTable);
-    Map<String, DataMapStatusDetail> map = DataMapStatusManager.readDataMapStatusMap();
+    List<TableIndex> visibleIndexes = carbonTable.getAllVisibleIndexes();
     cgIndexes = new ArrayList<>(visibleIndexes.size());
     fgIndexes = new ArrayList<>(visibleIndexes.size());
     for (TableIndex visibleIndex : visibleIndexes) {
-      DataMapStatusDetail status = map.get(visibleIndex.getDataMapSchema().getDataMapName());
-      if (status != null && status.isEnabled()) {
-        IndexLevel level = visibleIndex.getIndexFactory().getDataMapLevel();
+      if (visibleIndex.getIndexSchema().getProperties().get(CarbonCommonConstants.INDEX_STATUS)
 
 Review comment:
   DatamapSchemas from old store will be not taken/considered with current code

----------------------------------------------------------------
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 #3688: [CARBONDATA-3765] Refactor Index Metadata for CG and FG Indexes
URL: https://github.com/apache/carbondata/pull/3688#discussion_r405294912
 
 

 ##########
 File path: format/src/main/thrift/schema.thrift
 ##########
 @@ -203,9 +203,9 @@ struct ParentColumnTableRelation {
    3: required string columnName
 }
 
-struct DataMapSchema  {
+struct IndexSchema  {
 
 Review comment:
   These changes added in `schema.thrift` are to support pre-aggregate. I will remove those changes from `schema.thrift` file

----------------------------------------------------------------
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 #3688: [CARBONDATA-3765] Refactor Index Metadata for CG and FG Indexes
URL: https://github.com/apache/carbondata/pull/3688#issuecomment-610808714
 
 
   Build Failed  with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2677/
   

----------------------------------------------------------------
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 #3688: [CARBONDATA-3765] Refactor Index Metadata for CG and FG Indexes
URL: https://github.com/apache/carbondata/pull/3688#issuecomment-610813477
 
 
   Build Failed  with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/965/
   

----------------------------------------------------------------
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 #3688: [CARBONDATA-3765] Refactor Index Metadata for CG and FG Indexes
URL: https://github.com/apache/carbondata/pull/3688#issuecomment-610887402
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2681/
   

----------------------------------------------------------------
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 #3688: [CARBONDATA-3765] Refactor Index Metadata for CG and FG Indexes
URL: https://github.com/apache/carbondata/pull/3688#issuecomment-610904927
 
 
   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/969/
   

----------------------------------------------------------------
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 #3688: [CARBONDATA-3765] Refactor Index Metadata for CG and FG Indexes
URL: https://github.com/apache/carbondata/pull/3688#discussion_r405971778
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/datamap/DataMapStoreManager.java
 ##########
 @@ -101,49 +100,29 @@ private DataMapStoreManager() {
   }
 
   /**
-   * It only gives the visible datamaps
-   */
-  List<TableIndex> getAllVisibleIndexes(CarbonTable carbonTable) throws IOException {
-    CarbonSessionInfo sessionInfo = ThreadLocalSessionInfo.getCarbonSessionInfo();
-    List<TableIndex> allDataMaps = getAllIndexes(carbonTable);
-    Iterator<TableIndex> dataMapIterator = allDataMaps.iterator();
-    while (dataMapIterator.hasNext()) {
-      TableIndex dataMap = dataMapIterator.next();
-      String dbName = carbonTable.getDatabaseName();
-      String tableName = carbonTable.getTableName();
-      String dmName = dataMap.getDataMapSchema().getDataMapName();
-      // TODO: need support get the visible status of datamap without sessionInfo in the future
-      if (sessionInfo != null) {
-        boolean isDmVisible = sessionInfo.getSessionParams().getProperty(
-            String.format("%s%s.%s.%s", CarbonCommonConstants.CARBON_DATAMAP_VISIBLE,
-                dbName, tableName, dmName), "true").trim().equalsIgnoreCase("true");
-        if (!isDmVisible) {
-          LOGGER.warn(String.format("Ignore invisible datamap %s on table %s.%s",
-              dmName, dbName, tableName));
-          dataMapIterator.remove();
-        }
-      } else {
-        String message = "Carbon session info is null";
-        LOGGER.info(message);
-      }
-    }
-    return allDataMaps;
-  }
-
-  /**
-   * It gives all indexes except the default index.
+   * It gives all indexes except the default index and secondary index.
+   * Collect's Coarse grain and Fine grain indexes on a table
    *
    * @return
    */
   public List<TableIndex> getAllIndexes(CarbonTable carbonTable) throws IOException {
-    List<DataMapSchema> dataMapSchemas = getDataMapSchemasOfTable(carbonTable);
+    String indexMeta = carbonTable.getTableInfo().getFactTable().getTableProperties()
+        .get(carbonTable.getCarbonTableIdentifier().getTableId());
 
 Review comment:
   why use table id for key for this table property ? why not call key as "index_meta" ?

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