[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5378: The last of the getTimeFieldSpec calls

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

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



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/realtime/converter/RealtimeSegmentConverter.java
##########
@@ -156,20 +156,8 @@ public void build(@Nullable SegmentVersion segmentVersion, ServerMetrics serverM
   @VisibleForTesting
   public Schema getUpdatedSchema(Schema original) {

Review comment:
       We need to remove the time transform here because the time has already been transformed.
   Please also update the javadoc

##########
File path: pinot-core/src/test/java/org/apache/pinot/realtime/converter/RealtimeSegmentConverterTest.java
##########
@@ -46,28 +46,11 @@ public void testNoVirtualColumnsInSchema() {
     String segmentName = "segment1";
     VirtualColumnProviderFactory.addBuiltInVirtualColumnsToSegmentSchema(schema, segmentName);
     Assert.assertEquals(schema.getColumnNames().size(), 5);
-    Assert.assertEquals(schema.getTimeFieldSpec().getIncomingGranularitySpec().getTimeType(), TimeUnit.MILLISECONDS);
 
     RealtimeSegmentConverter converter =
         new RealtimeSegmentConverter(null, "", schema, "testTable", tableConfig, segmentName, "col1");
 
     Schema newSchema = converter.getUpdatedSchema(schema);
     Assert.assertEquals(newSchema.getColumnNames().size(), 2);
-    Assert.assertEquals(newSchema.getTimeFieldSpec().getIncomingGranularitySpec().getTimeType(), TimeUnit.DAYS);
-  }
-
-  @Test
-  public void testNoTimeColumnsInSchema() {
-    Schema schema = new Schema();

Review comment:
       Do we still want this support? (Realtime table without time column)

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/indexsegment/generator/SegmentGeneratorConfig.java
##########
@@ -182,42 +180,21 @@ public SegmentGeneratorConfig(TableConfig tableConfig, Schema schema) {
   }
 
   /**
-   * Set time column details using the given time column. If not found, use schema
+   * Set time column details using the given time column
    */
   public void setTime(String timeColumnName, Schema schema) {

Review comment:
       Annotate `timeColumnName` as nullable or check it on the caller side

##########
File path: pinot-core/src/test/java/org/apache/pinot/core/indexsegment/generator/SegmentGeneratorConfigTest.java
##########
@@ -48,14 +48,12 @@ public void testEpochTime() {
     assertNull(segmentGeneratorConfig.getSimpleDateFormat());
 
     // table config not provided

Review comment:
       Remove this line?

##########
File path: pinot-core/src/test/java/org/apache/pinot/core/indexsegment/generator/SegmentGeneratorConfigTest.java
##########
@@ -74,14 +72,12 @@ public void testSimpleDateFormat() {
     assertEquals(segmentGeneratorConfig.getSimpleDateFormat(), "yyyyMMdd");
 
     // Table config not provided

Review comment:
       Remove this line?

##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java
##########
@@ -582,10 +582,6 @@ public boolean equals(Object o) {
    */
 
   public boolean isBackwardCompatibleWith(Schema oldSchema) {
-    if (!EqualityUtils.isEqual(_timeFieldSpec, oldSchema.getTimeFieldSpec()) || !EqualityUtils
-        .isEqual(_dateTimeFieldSpecs, oldSchema.getDateTimeFieldSpecs())) {
-      return false;
-    }
     for (Map.Entry<String, FieldSpec> entry : oldSchema.getFieldSpecMap().entrySet()) {
       if (!getColumnNames().contains(entry.getKey())) {

Review comment:
       Move `getColumnNames()` out of the loop




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