Jackie-Jiang commented on code in PR #8779:
URL: https://github.com/apache/pinot/pull/8779#discussion_r885049733
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeFormatSpec.java:
##########
@@ -47,28 +49,69 @@ 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 MIN_FORMAT_TOKENS_PIPE = 1;
+ public static final int MAX_FORMAT_TOKENS_PIPE = 3;
+
+ //Applicable for SDF|<timeFormat>(|<timezone>)
Review Comment:
(minor) Also add a space between the comment and `//`
```suggestion
// Applicable for SIMPLE_DATE_FORMAT|<timeFormat>(|<timezone>)
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeFormatSpec.java:
##########
@@ -47,28 +49,69 @@ 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 MIN_FORMAT_TOKENS_PIPE = 1;
+ public static final int MAX_FORMAT_TOKENS_PIPE = 3;
+
+ //Applicable for SDF|<timeFormat>(|<timezone>)
+ public static final int SDF_PATTERN_POSITION = 1;
+ public static final int SDF_TIMEZONE_POSITION = 2;
+
+ //Applicable for EPOCH|<timeUnit>(|<size>)
+ public static final int EPOCH_UNIT_POSITION = 1;
+ public static final int EPOCH_SIZE_POSITION = 2;
+
private final String _format;
private final int _size;
private final DateTimeFormatUnitSpec _unitSpec;
private final DateTimeFormatPatternSpec _patternSpec;
public DateTimeFormatSpec(String format) {
_format = format;
- validateFormat(format);
- String[] formatTokens = StringUtils.split(format, COLON_SEPARATOR,
MAX_FORMAT_TOKENS);
- if (formatTokens.length == MAX_FORMAT_TOKENS) {
- _patternSpec = new
DateTimeFormatPatternSpec(formatTokens[FORMAT_TIMEFORMAT_POSITION],
- formatTokens[FORMAT_PATTERN_POSITION]);
+ if (Character.isDigit(_format.charAt(0))) {
+ String[] formatTokens = validateFormat(format);
+ if (formatTokens.length == MAX_FORMAT_TOKENS) {
+ _patternSpec = new
DateTimeFormatPatternSpec(formatTokens[FORMAT_TIMEFORMAT_POSITION],
+ formatTokens[FORMAT_PATTERN_POSITION]);
+ } else {
+ _patternSpec = new
DateTimeFormatPatternSpec(formatTokens[FORMAT_TIMEFORMAT_POSITION]);
+ }
+ if (_patternSpec.getTimeFormat() == TimeFormat.TIMESTAMP) {
+ // TIMESTAMP type stores millis since epoch
+ _size = 1;
+ _unitSpec = new DateTimeFormatUnitSpec("MILLISECONDS");
+ } else {
+ _size = Integer.parseInt(formatTokens[FORMAT_SIZE_POSITION]);
+ _unitSpec = new
DateTimeFormatUnitSpec(formatTokens[FORMAT_UNIT_POSITION]);
+ }
} else {
- _patternSpec = new
DateTimeFormatPatternSpec(formatTokens[FORMAT_TIMEFORMAT_POSITION]);
- }
- if (_patternSpec.getTimeFormat() == TimeFormat.TIMESTAMP) {
- // TIMESTAMP type stores millis since epoch
- _size = 1;
- _unitSpec = new DateTimeFormatUnitSpec("MILLISECONDS");
- } else {
- _size = Integer.parseInt(formatTokens[FORMAT_SIZE_POSITION]);
- _unitSpec = new
DateTimeFormatUnitSpec(formatTokens[FORMAT_UNIT_POSITION]);
+ String[] formatTokens = validatePipeFormat(format);
+ if
(formatTokens[FORMAT_TIMEFORMAT_POSITION_PIPE].equals(TimeFormat.EPOCH.toString()))
{
+ _patternSpec = new
DateTimeFormatPatternSpec(formatTokens[FORMAT_TIMEFORMAT_POSITION_PIPE]);
+ _unitSpec = new
DateTimeFormatUnitSpec(formatTokens[EPOCH_UNIT_POSITION]);
+ if (formatTokens.length == MAX_FORMAT_TOKENS_PIPE) {
+ _size = Integer.parseInt(formatTokens[EPOCH_SIZE_POSITION]);
+ } else {
+ _size = 1;
+ }
+ } else if
(formatTokens[FORMAT_TIMEFORMAT_POSITION_PIPE].equals(TimeFormat.SIMPLE_DATE_FORMAT.toString()))
{
+ if (formatTokens.length == MAX_FORMAT_TOKENS_PIPE) {
+ _patternSpec = new DateTimeFormatPatternSpec(TimeFormat.valueOf(
+ formatTokens[FORMAT_TIMEFORMAT_POSITION_PIPE]),
+ formatTokens[SDF_PATTERN_POSITION],
+ formatTokens[SDF_TIMEZONE_POSITION]);
+ } else {
+ _patternSpec = new DateTimeFormatPatternSpec(TimeFormat.valueOf(
+ formatTokens[FORMAT_TIMEFORMAT_POSITION_PIPE]),
+ formatTokens[SDF_PATTERN_POSITION],
DateTimeFormatPatternSpec.DEFAULT_DATETIMEZONE.toString());
Review Comment:
We should pass in `null` as the last argument
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeFormatSpec.java:
##########
@@ -47,28 +49,69 @@ 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 MIN_FORMAT_TOKENS_PIPE = 1;
+ public static final int MAX_FORMAT_TOKENS_PIPE = 3;
+
+ //Applicable for SDF|<timeFormat>(|<timezone>)
+ public static final int SDF_PATTERN_POSITION = 1;
+ public static final int SDF_TIMEZONE_POSITION = 2;
+
+ //Applicable for EPOCH|<timeUnit>(|<size>)
+ public static final int EPOCH_UNIT_POSITION = 1;
+ public static final int EPOCH_SIZE_POSITION = 2;
+
private final String _format;
private final int _size;
private final DateTimeFormatUnitSpec _unitSpec;
private final DateTimeFormatPatternSpec _patternSpec;
public DateTimeFormatSpec(String format) {
_format = format;
- validateFormat(format);
- String[] formatTokens = StringUtils.split(format, COLON_SEPARATOR,
MAX_FORMAT_TOKENS);
- if (formatTokens.length == MAX_FORMAT_TOKENS) {
- _patternSpec = new
DateTimeFormatPatternSpec(formatTokens[FORMAT_TIMEFORMAT_POSITION],
- formatTokens[FORMAT_PATTERN_POSITION]);
+ if (Character.isDigit(_format.charAt(0))) {
+ String[] formatTokens = validateFormat(format);
+ if (formatTokens.length == MAX_FORMAT_TOKENS) {
+ _patternSpec = new
DateTimeFormatPatternSpec(formatTokens[FORMAT_TIMEFORMAT_POSITION],
+ formatTokens[FORMAT_PATTERN_POSITION]);
+ } else {
+ _patternSpec = new
DateTimeFormatPatternSpec(formatTokens[FORMAT_TIMEFORMAT_POSITION]);
+ }
+ if (_patternSpec.getTimeFormat() == TimeFormat.TIMESTAMP) {
+ // TIMESTAMP type stores millis since epoch
+ _size = 1;
+ _unitSpec = new DateTimeFormatUnitSpec("MILLISECONDS");
+ } else {
+ _size = Integer.parseInt(formatTokens[FORMAT_SIZE_POSITION]);
+ _unitSpec = new
DateTimeFormatUnitSpec(formatTokens[FORMAT_UNIT_POSITION]);
+ }
} else {
- _patternSpec = new
DateTimeFormatPatternSpec(formatTokens[FORMAT_TIMEFORMAT_POSITION]);
- }
- if (_patternSpec.getTimeFormat() == TimeFormat.TIMESTAMP) {
- // TIMESTAMP type stores millis since epoch
- _size = 1;
- _unitSpec = new DateTimeFormatUnitSpec("MILLISECONDS");
- } else {
- _size = Integer.parseInt(formatTokens[FORMAT_SIZE_POSITION]);
- _unitSpec = new
DateTimeFormatUnitSpec(formatTokens[FORMAT_UNIT_POSITION]);
+ String[] formatTokens = validatePipeFormat(format);
+ if
(formatTokens[FORMAT_TIMEFORMAT_POSITION_PIPE].equals(TimeFormat.EPOCH.toString()))
{
+ _patternSpec = new
DateTimeFormatPatternSpec(formatTokens[FORMAT_TIMEFORMAT_POSITION_PIPE]);
+ _unitSpec = new
DateTimeFormatUnitSpec(formatTokens[EPOCH_UNIT_POSITION]);
+ if (formatTokens.length == MAX_FORMAT_TOKENS_PIPE) {
+ _size = Integer.parseInt(formatTokens[EPOCH_SIZE_POSITION]);
+ } else {
+ _size = 1;
+ }
+ } else if
(formatTokens[FORMAT_TIMEFORMAT_POSITION_PIPE].equals(TimeFormat.SIMPLE_DATE_FORMAT.toString()))
{
+ if (formatTokens.length == MAX_FORMAT_TOKENS_PIPE) {
+ _patternSpec = new DateTimeFormatPatternSpec(TimeFormat.valueOf(
+ formatTokens[FORMAT_TIMEFORMAT_POSITION_PIPE]),
+ formatTokens[SDF_PATTERN_POSITION],
+ formatTokens[SDF_TIMEZONE_POSITION]);
+ } else {
+ _patternSpec = new DateTimeFormatPatternSpec(TimeFormat.valueOf(
+ formatTokens[FORMAT_TIMEFORMAT_POSITION_PIPE]),
+ formatTokens[SDF_PATTERN_POSITION],
DateTimeFormatPatternSpec.DEFAULT_DATETIMEZONE.toString());
+ }
+ _unitSpec = new DateTimeFormatUnitSpec(TimeUnit.DAYS.toString());
+ _size = 1;
+ } else {
Review Comment:
Add some comment here stating that this branch is for `TIMESTAMP` format
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeFormatSpec.java:
##########
@@ -97,6 +157,19 @@ public DateTimeFormatSpec(int columnSize, String
columnUnit, String columnTimeFo
_patternSpec = new DateTimeFormatPatternSpec(columnTimeFormat, sdfPattern);
}
+ public DateTimeFormatSpec(String timeFormat, String columnTimeFormat,
@Nullable String timeZone) {
Review Comment:
Same here, let's not add new constructors for this class
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeFormatPatternSpec.java:
##########
@@ -65,6 +66,20 @@ public DateTimeFormatPatternSpec(String timeFormat,
@Nullable String sdfPatternW
}
}
+ public DateTimeFormatPatternSpec(DateTimeFieldSpec.TimeFormat timeFormat,
String timeFormatStyle,
+ @Nullable String timeZone) {
+ _timeFormat = timeFormat;
+ if (_timeFormat.equals(DateTimeFieldSpec.TimeFormat.SIMPLE_DATE_FORMAT)) {
+ if (Objects.nonNull(timeZone)) {
Review Comment:
(minor)
```suggestion
if (timeZone != null) {
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeFormatSpec.java:
##########
@@ -84,6 +127,23 @@ public DateTimeFormatSpec(int columnSize, String
columnUnit, String columnTimeFo
_patternSpec = new DateTimeFormatPatternSpec(columnTimeFormat);
}
+ /**
+ * Constructs a dateTimeSpec format, given the components of a format
+ */
+ public DateTimeFormatSpec(String columnTimeFormat, String columnUnit, int
columnSize) {
Review Comment:
Let's not add new constructors for this class. It should always be
constructed from the format string
--
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
