[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4583: Support default value for Byte column

2019-09-17 Thread GitBox 
Jackie-Jiang commented on a change in pull request #4583: Support default value 
for Byte column
URL: https://github.com/apache/incubator-pinot/pull/4583#discussion_r325378744
 
 

 ##
 File path: 
pinot-core/src/main/java/org/apache/pinot/core/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java
 ##
 @@ -303,6 +304,10 @@ protected void createColumnV1Indices(String column)
 dictionaryElementSize = 
StringUtil.encodeUtf8(stringDefaultValue).length;
 
 Review comment:
   @kishoreg We always encode String using UTF-8. As long as the files are 
generated with Pinot library, the String will be encoded with UTF-8. Record 
reader should be able to decode other format if necessary.


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


With regards,
Apache Git Services

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

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4583: Support default value for Byte column

2019-09-17 Thread GitBox 
Jackie-Jiang commented on a change in pull request #4583: Support default value 
for Byte column
URL: https://github.com/apache/incubator-pinot/pull/4583#discussion_r325355324
 
 

 ##
 File path: 
pinot-core/src/main/java/org/apache/pinot/core/segment/creator/impl/SegmentColumnarIndexCreator.java
 ##
 @@ -574,8 +575,14 @@ public static void 
addColumnMetadataInfo(PropertiesConfiguration properties, Str
 if (defaultNullValue == null) {
   defaultNullValue = fieldSpec.getDefaultNullValue();
 }
-properties.setProperty(V1Constants.MetadataKeys.Column.getKeyFor(column, 
DEFAULT_NULL_VALUE),
-String.valueOf(defaultNullValue));
+if (fieldSpec.getDataType() == FieldSpec.DataType.BYTES && 
defaultNullValue instanceof byte[]) {
 
 Review comment:
   We just need the second check


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


With regards,
Apache Git Services

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

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4583: Support default value for Byte column

2019-09-17 Thread GitBox 
Jackie-Jiang commented on a change in pull request #4583: Support default value 
for Byte column
URL: https://github.com/apache/incubator-pinot/pull/4583#discussion_r325355088
 
 

 ##
 File path: 
pinot-core/src/main/java/org/apache/pinot/core/segment/creator/impl/SegmentColumnarIndexCreator.java
 ##
 @@ -574,8 +575,14 @@ public static void 
addColumnMetadataInfo(PropertiesConfiguration properties, Str
 if (defaultNullValue == null) {
 
 Review comment:
   I don't think defaultNullValue can ever be null


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


With regards,
Apache Git Services

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