Validate format pattern in `dateTimeFieldSpecs` upon schema creation

[mapshen]

The following error was encountered today

2021/11/03 14:21:49.226 INFO [LLRealtimeSegmentDataManager_table_realtime__0__0__20211101T1431Z] [table_realtime__0__0__20211101T1431Z] Waiting to acquire semaphore for building segment
2021/11/03 14:21:49.227 INFO [LLRealtimeSegmentDataManager_table_realtime__0__0__20211101T1431Z] [table_realtime__0__0__20211101T1431Z] Trying to build segment
2021/11/03 14:21:49.227 ERROR [LLRealtimeSegmentDataManager_table_realtime__0__0__20211101T1431Z] [table_realtime__0__0__20211101T1431Z] Could not build segment
java.lang.IllegalArgumentException: Illegal pattern component: T
        at org.joda.time.format.DateTimeFormat.parsePatternTo(DateTimeFormat.java:566) ~[pinot-all-0.8.0-jar-with-dependencies.jar:0.8.0-c4ceff06d21fc1c1b88469a8dbae742a4b609808]
        at org.joda.time.format.DateTimeFormat.createFormatterForPattern(DateTimeFormat.java:687) ~[pinot-all-0.8.0-jar-with-dependencies.jar:0.8.0-c4ceff06d21fc1c1b88469a8dbae742a4b609808]
        at org.joda.time.format.DateTimeFormat.forPattern(DateTimeFormat.java:177) ~[pinot-all-0.8.0-jar-with-dependencies.jar:0.8.0-c4ceff06d21fc1c1b88469a8dbae742a4b609808]
        at org.apache.pinot.spi.data.DateTimeFormatPatternSpec.<init>(DateTimeFormatPatternSpec.java:57) ~[pinot-all-0.8.0-jar-with-dependencies.jar:0.8.0-c4ceff06d21fc1c1b88469a8dbae742a4b609808]
        at org.apache.pinot.spi.data.DateTimeFormatSpec.<init>(DateTimeFormatSpec.java:60) ~[pinot-all-0.8.0-jar-with-dependencies.jar:0.8.0-c4ceff06d21fc1c1b88469a8dbae742a4b609808]
        at org.apache.pinot.segment.spi.creator.SegmentGeneratorConfig.setTime(SegmentGeneratorConfig.java:214) ~[pinot-all-0.8.0-jar-with-dependencies.jar:0.8.0-c4ceff06d21fc1c1b88469a8dbae742a4b609808]
        at org.apache.pinot.segment.spi.creator.SegmentGeneratorConfig.<init>(SegmentGeneratorConfig.java:140) ~[pinot-all-0.8.0-jar-with-dependencies.jar:0.8.0-c4ceff06d21fc1c1b88469a8dbae742a4b609808]
        at org.apache.pinot.segment.local.realtime.converter.RealtimeSegmentConverter.build(RealtimeSegmentConverter.java:83) ~[pinot-all-0.8.0-jar-with-dependencies.jar:0.8.0-c4ceff06d21fc1c1b88469a8dbae742a4b609808]
        at org.apache.pinot.core.data.manager.realtime.LLRealtimeSegmentDataManager.buildSegmentInternal(LLRealtimeSegmentDataManager.java:794) [pinot-all-0.8.0-jar-with-dependencies.jar:0.8.0-c4ceff06d21fc1c1b88469a8dbae742a4b609808
]

The relevant config was

    "dateTimeFieldSpecs": [
      {
        "name": "DATETIME_F",
        "dataType": "STRING",
        "format": "1:SECONDS:SIMPLE_DATE_FORMAT:yyyy-MM-ddTHH:mm:ss",
        "granularity": "1:SECONDS"
      }
    ],

You can tell yyyy-MM-ddTHH:mm:ss should have been yyyy-MM-dd'T'HH:mm:ss instead. However, the error was not caught till we found the table had stopped consuming messages. Even further, and this format pattern is only validated when a segment is built, no before or during consumption.

Therefore, it would be benefit the users if we can fail fast and throw the error when creating the schema in the first place.

[Jackie-Jiang]

+1. We should validate the time format when getting the first event from the upstreaming. It might not be possible to validate it during the table creation because we haven't got the event yet.

[mapshen]

Oh, I am saying the format config itself should be validated upon creation. You don't need an event to tell that yyyy-MM-ddTHH:mm:ss is not valid.

Also I agree with you that events should be validated against dateTimeFieldSpecs as well.

[Jackie-Jiang]

Oh, I see. Good point.

So basically we should perform 2 validations:

1. Validate the format itself when creating the table
2. Validate if the value follows the format when getting the first event

[mapshen]

2. Validate if the value follows the format when getting the first event

Should Pinot continue to validate all the following events? I am not quite clear on how helpful it is if it only validated the first one. On the other hand, if it does validate all events, we need to figure out how much it will impact the performance.

Probably we should limit the of scope of this issue to "1. Validate the format itself when creating the table".

[Jackie-Jiang]

Validating all might be too much. Or we can add a flag to allow turning the validation on/off

[ksnijjer]

@Jackie-Jiang saw this issue again with another user, segment build failing because of wrong format(missing millis) specified for time column:
2021/12/10 08:42:35.713 ERROR [LLRealtimeSegmentDataManager_dark_store_projection_stage__0__0__20211210T0838Z] [dark_store_projection_stage__0__0__20211210T0838Z] Could not build segment java.lang.IllegalArgumentException: Invalid format: "2021-12-07 15:39:49.684" is malformed at ".684" at org.joda.time.format.DateTimeParserBucket.doParseMillis(DateTimeParserBucket.java:187) ~[startree-pinot-all-0.9.0-ST.7-jar-with-dependencies.jar:0.9.0-ST.7-dbffa0a2688be756084876afd01079b22c7bc9f4] at org.joda.time.format.DateTimeFormatter.parseMillis(DateTimeFormatter.java:826) ~[startree-pinot-all-0.9.0-ST.7-jar-with-dependencies.jar:0.9.0-ST.7-dbffa0a2688be756084876afd01079b22c7bc9f4]

Can we expedite a fix ? cc @mayankshriv

[Jackie-Jiang]

#7804 Added validation on SDF when adding/updating the schema, and ensure it is in lexicographic order. This is required for range query to work properly.

To solve the problem raised by @ksnijjer, we need to validate the input value (the second validation mentioned above)

[mapshen]

#7804 Added validation on SDF when adding/updating the schema, and ensure it is in lexicographic order. This is required for range query to work properly.

Thanks @Jackie-Jiang - it would be great to update the docs to reflect this as well!

[walterddr]

Yes I think #7804 should address the issue by validating during schema change, and the error message should also be improved to include the entire SDF pattern passed in. Let me also change the docs as well