[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7269: Null handling for time column

[GitBox <gi...@apache.org>]

Jackie-Jiang commented on a change in pull request #7269:
URL: https://github.com/apache/pinot/pull/7269#discussion_r686308777



##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/NullValueTransformer.java
##########
@@ -41,6 +45,14 @@ public NullValueTransformer(Schema schema) {
         }
       }
     }
+
+    if (tableConfig.getValidationConfig().isNullTimeColumnHandlingEnabled() && schema.getTimeFieldSpec() != null && schema.getTimeFieldSpec().getIncomingGranularitySpec() != null) {

Review comment:
       Use `schema.getSpecForTimeColumn(timeColumn)` to read the field spec

##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/NullValueTransformer.java
##########
@@ -20,16 +20,20 @@
 
 import java.util.HashMap;
 import java.util.Map;
+import org.apache.pinot.spi.config.table.TableConfig;
 import org.apache.pinot.spi.data.FieldSpec;
 import org.apache.pinot.spi.data.FieldSpec.FieldType;
 import org.apache.pinot.spi.data.Schema;
 import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.utils.TimeConverter;
 
 
 public class NullValueTransformer implements RecordTransformer {
   private final Map<String, Object> _defaultNullValues = new HashMap<>();
+  private final TimeConverter _timeConverter;
+  private final String _timeColumnName;
 
-  public NullValueTransformer(Schema schema) {
+  public NullValueTransformer(TableConfig tableConfig, Schema schema) {
     for (FieldSpec fieldSpec : schema.getAllFieldSpecs()) {
       if (!fieldSpec.isVirtualColumn() && fieldSpec.getFieldType() != FieldType.TIME) {

Review comment:
       The time field can be of type `TIME` or `DATE_TIME`, so we might want to check the field name instead

##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/NullValueTransformer.java
##########
@@ -52,6 +64,12 @@ public GenericRow transform(GenericRow record) {
         record.putDefaultNullValue(fieldName, entry.getValue());
       }
     }
+
+    // handle null value in time column
+    if (_timeConverter != null && record.getValue(_timeColumnName) == null) {
+      record.putDefaultNullValue(_timeColumnName, _timeConverter.fromMillisSinceEpoch(System.currentTimeMillis()));

Review comment:
       The value should be converted into the correct data type. One way to achieve that is to put the `NullValueTransformer` after the `DataTypeTransformer` in the `CompositeTransformer`

##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
##########
@@ -192,6 +193,10 @@ private static void validateValidationConfig(TableConfig tableConfig, @Nullable
       }
     }
 
+    if (validationConfig.isNullTimeColumnHandlingEnabled()) {

Review comment:
       There can be multiple date time fields. Do we want to handle the null time value for only the time column or all the date time fields?

##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentsValidationAndRetentionConfig.java
##########
@@ -38,6 +38,8 @@
   private String _schemaName;
   private String _timeColumnName;
   private TimeUnit _timeType;
+  // Flag to indicate if null value in time column should be handled.
+  private boolean _nullTimeColumnHandlingEnabled;

Review comment:
       Suggest renaming to `_allowNullTimeValue`




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

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

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