You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/07/16 18:14:02 UTC

[GitHub] [incubator-pinot] siddharthteotia opened a new pull request #5710: Cleanup the call to RealtimeSegmentConverter from HLCDataManager

siddharthteotia opened a new pull request #5710:
URL: https://github.com/apache/incubator-pinot/pull/5710


   After the removal of old star tree spec, the calls to constructor were not correct. 


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #5710: Cleanup the call to RealtimeSegmentConverter from HLCDataManager

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5710:
URL: https://github.com/apache/incubator-pinot/pull/5710#discussion_r456082248



##########
File path: pinot-core/src/test/java/org/apache/pinot/realtime/converter/RealtimeSegmentConverterTest.java
##########
@@ -46,11 +46,7 @@ public void testNoVirtualColumnsInSchema() {
     String segmentName = "segment1";

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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] siddharthteotia merged pull request #5710: Cleanup the call to RealtimeSegmentConverter from HLCDataManager

Posted by GitBox <gi...@apache.org>.
siddharthteotia merged pull request #5710:
URL: https://github.com/apache/incubator-pinot/pull/5710


   


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #5710: Cleanup the call to RealtimeSegmentConverter from HLCDataManager

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5710:
URL: https://github.com/apache/incubator-pinot/pull/5710#discussion_r456004770



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/HLRealtimeSegmentDataManager.java
##########
@@ -287,8 +287,7 @@ public void run() {
               new RealtimeSegmentConverter(_realtimeSegment, tempSegmentFolder.getAbsolutePath(), schema,
                   _tableNameWithType, tableConfig, realtimeSegmentZKMetadata.getSegmentName(), _sortedColumn,
                   HLRealtimeSegmentDataManager.this._invertedIndexColumns, _noDictionaryColumns,
-                  _varLengthDictionaryColumns, null/*StarTreeIndexSpec*/,
-                  indexingConfig.isNullHandlingEnabled()); // Star tree not supported for HLC.
+                  _varLengthDictionaryColumns, indexingConfig.isNullHandlingEnabled());

Review comment:
       It is there for LLC already. Will add for HLC as well
   




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] siddharthteotia commented on pull request #5710: Cleanup the call to RealtimeSegmentConverter from HLCDataManager

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on pull request #5710:
URL: https://github.com/apache/incubator-pinot/pull/5710#issuecomment-659611328


   @Jackie-Jiang , addressed the review comments. 


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #5710: Cleanup the call to RealtimeSegmentConverter from HLCDataManager

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5710:
URL: https://github.com/apache/incubator-pinot/pull/5710#discussion_r456009572



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/realtime/converter/RealtimeSegmentConverter.java
##########
@@ -82,14 +83,14 @@ public RealtimeSegmentConverter(MutableSegmentImpl realtimeSegment, String outpu
       List<String> invertedIndexColumns, List<String> noDictionaryColumns, List<String> varLengthDictionaryColumns,
       boolean nullHandlingEnabled) {
     this(realtimeSegment, outputPath, schema, tableName, tableConfig, segmentName, sortedColumn,
-        invertedIndexColumns, new ArrayList<>(), noDictionaryColumns, varLengthDictionaryColumns, nullHandlingEnabled);
+        invertedIndexColumns, Collections.emptyList(), noDictionaryColumns, varLengthDictionaryColumns, nullHandlingEnabled);
   }
 
   // Used in RealtimeSegmentConverterTest
   public RealtimeSegmentConverter(MutableSegmentImpl realtimeSegment, String outputPath, Schema schema,

Review comment:
       done

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/HLRealtimeSegmentDataManager.java
##########
@@ -287,8 +287,7 @@ public void run() {
               new RealtimeSegmentConverter(_realtimeSegment, tempSegmentFolder.getAbsolutePath(), schema,
                   _tableNameWithType, tableConfig, realtimeSegmentZKMetadata.getSegmentName(), _sortedColumn,
                   HLRealtimeSegmentDataManager.this._invertedIndexColumns, _noDictionaryColumns,

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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5710: Cleanup the call to RealtimeSegmentConverter from HLCDataManager

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5710:
URL: https://github.com/apache/incubator-pinot/pull/5710#discussion_r455998572



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/HLRealtimeSegmentDataManager.java
##########
@@ -287,8 +287,7 @@ public void run() {
               new RealtimeSegmentConverter(_realtimeSegment, tempSegmentFolder.getAbsolutePath(), schema,
                   _tableNameWithType, tableConfig, realtimeSegmentZKMetadata.getSegmentName(), _sortedColumn,
                   HLRealtimeSegmentDataManager.this._invertedIndexColumns, _noDictionaryColumns,

Review comment:
       (nit) Remove `HLRealtimeSegmentDataManager.this.`

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/realtime/converter/RealtimeSegmentConverter.java
##########
@@ -82,14 +83,14 @@ public RealtimeSegmentConverter(MutableSegmentImpl realtimeSegment, String outpu
       List<String> invertedIndexColumns, List<String> noDictionaryColumns, List<String> varLengthDictionaryColumns,
       boolean nullHandlingEnabled) {
     this(realtimeSegment, outputPath, schema, tableName, tableConfig, segmentName, sortedColumn,
-        invertedIndexColumns, new ArrayList<>(), noDictionaryColumns, varLengthDictionaryColumns, nullHandlingEnabled);
+        invertedIndexColumns, Collections.emptyList(), noDictionaryColumns, varLengthDictionaryColumns, nullHandlingEnabled);
   }
 
   // Used in RealtimeSegmentConverterTest
   public RealtimeSegmentConverter(MutableSegmentImpl realtimeSegment, String outputPath, Schema schema,

Review comment:
       Let's remove the extra constructors and just keep one (easier to use).
   The only usage of them is `RealtimeSegmentConverterTest.testNoVirtualColumnsInSchema()`, where you can make `RealtimeSegmentConverter.getUpdatedSchema()` static

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/HLRealtimeSegmentDataManager.java
##########
@@ -287,8 +287,7 @@ public void run() {
               new RealtimeSegmentConverter(_realtimeSegment, tempSegmentFolder.getAbsolutePath(), schema,
                   _tableNameWithType, tableConfig, realtimeSegmentZKMetadata.getSegmentName(), _sortedColumn,
                   HLRealtimeSegmentDataManager.this._invertedIndexColumns, _noDictionaryColumns,
-                  _varLengthDictionaryColumns, null/*StarTreeIndexSpec*/,
-                  indexingConfig.isNullHandlingEnabled()); // Star tree not supported for HLC.
+                  _varLengthDictionaryColumns, indexingConfig.isNullHandlingEnabled());

Review comment:
       We should support text index for HLC




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5710: Cleanup the call to RealtimeSegmentConverter from HLCDataManager

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5710:
URL: https://github.com/apache/incubator-pinot/pull/5710#discussion_r456075562



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/HLRealtimeSegmentDataManager.java
##########
@@ -286,9 +287,8 @@ public void run() {
           RealtimeSegmentConverter converter =
               new RealtimeSegmentConverter(_realtimeSegment, tempSegmentFolder.getAbsolutePath(), schema,
                   _tableNameWithType, tableConfig, realtimeSegmentZKMetadata.getSegmentName(), _sortedColumn,
-                  HLRealtimeSegmentDataManager.this._invertedIndexColumns, _noDictionaryColumns,
-                  _varLengthDictionaryColumns, null/*StarTreeIndexSpec*/,
-                  indexingConfig.isNullHandlingEnabled()); // Star tree not supported for HLC.
+                  _invertedIndexColumns, Collections.emptyList(), _noDictionaryColumns, _varLengthDictionaryColumns,

Review comment:
       Are you planning to add text index support for HLC in a separate PR?

##########
File path: pinot-core/src/test/java/org/apache/pinot/realtime/converter/RealtimeSegmentConverterTest.java
##########
@@ -46,11 +46,7 @@ public void testNoVirtualColumnsInSchema() {
     String segmentName = "segment1";

Review comment:
       Remove the unused table config




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org