[GitHub] Jackie-Jiang commented on a change in pull request #3484: Fix the bug where time conversion is skipped when incoming and outgoing time column name are the same

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

Jackie-Jiang commented on a change in pull request #3484: Fix the bug where time conversion is skipped when incoming and outgoing time column name are the same
URL: https://github.com/apache/incubator-pinot/pull/3484#discussion_r234835978
 
 

 ##########
 File path: pinot-core/src/main/java/com/linkedin/pinot/core/data/recordtransformer/TimeTransformer.java
 ##########
 @@ -29,37 +29,64 @@
  * column for other record transformers (incoming time column can be ignored).
  */
 public class TimeTransformer implements RecordTransformer {
-  private final String _incomingTimeColumn;
-  private final String _outgoingTimeColumn;
-  private final TimeConverter _timeConverter;
+  private String _incomingTimeColumn;
+  private String _outgoingTimeColumn;
+  private TimeConverter _incomingTimeConverter;
+  private TimeConverter _outgoingTimeConverter;
+  private boolean _isFirstRecord = true;
 
   public TimeTransformer(Schema schema) {
     TimeFieldSpec timeFieldSpec = schema.getTimeFieldSpec();
     if (timeFieldSpec != null) {
       TimeGranularitySpec incomingGranularitySpec = timeFieldSpec.getIncomingGranularitySpec();
       TimeGranularitySpec outgoingGranularitySpec = timeFieldSpec.getOutgoingGranularitySpec();
+
+      // Perform time conversion only if incoming and outgoing granularity spec are different
       if (!incomingGranularitySpec.equals(outgoingGranularitySpec)) {
         _incomingTimeColumn = incomingGranularitySpec.getName();
         _outgoingTimeColumn = outgoingGranularitySpec.getName();
-        _timeConverter = TimeConverterProvider.getTimeConverter(incomingGranularitySpec, outgoingGranularitySpec);
-        return;
+        _incomingTimeConverter = new TimeConverter(incomingGranularitySpec);
+        _outgoingTimeConverter = new TimeConverter(outgoingGranularitySpec);
       }
     }
-    _incomingTimeColumn = null;
-    _outgoingTimeColumn = null;
-    _timeConverter = null;
   }
 
   @Override
   public GenericRow transform(GenericRow record) {
-    if (_timeConverter == null) {
+    if (_incomingTimeColumn == null) {
       return record;
     }
-    // Skip transformation if outgoing value already exist
-    // NOTE: outgoing value might already exist for OFFLINE data
-    if (record.getValue(_outgoingTimeColumn) == null) {
-      record.putField(_outgoingTimeColumn, _timeConverter.convert(record.getValue(_incomingTimeColumn)));
+
+    // Use the first record for sanity check and determine whether the conversion is needed
+    Object incomingTimeValue = record.getValue(_incomingTimeColumn);
+    if (_isFirstRecord) {
+      _isFirstRecord = false;
 
 Review comment:
   Make sense when use just ignore the exception and keep transforming. Will make the change, thanks.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services

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