KKcorps commented on code in PR #8779: URL: https://github.com/apache/pinot/pull/8779#discussion_r882631465
########## pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeFormatSpec.java: ########## @@ -47,28 +50,75 @@ public class DateTimeFormatSpec { public static final int MIN_FORMAT_TOKENS = 3; public static final int MAX_FORMAT_TOKENS = 4; + public static final int FORMAT_TIMEFORMAT_POSITION_PIPE = 0; + public static final int FORMAT_PATTERN_POSITION_PIPE = 1; Review Comment: Better to divide these static variables into 2 groups one for EPOCH format and other for SDF format. These looks a bit odd clumped together. You can also create class `EpochDateTimeFormat` and `SDFDateTimeFormat` that handle the processing part for this. It will also make it easier to add or change a specific format in the future without needing to change the variables ########## pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeFormatSpec.java: ########## @@ -36,7 +37,9 @@ public class DateTimeFormatSpec { public static final String NUMBER_REGEX = "[1-9][0-9]*"; + public static final String COLON_REGEX = "([0-9])(:)(\\w+)(:)(\\w+)(:?)(.+)"; Review Comment: Can we name this variable to a better one. Also, add a brief comment on why this is needed (mention the backward compatibility) -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org