ISO Support for Time Grains #1739
Conversation
...tastore-aggregation/src/main/java/com/yahoo/elide/datastores/aggregation/timegrains/Day.java
Outdated
Show resolved
Hide resolved
| import java.text.ParseException; | ||
| import java.text.SimpleDateFormat; | ||
|
|
||
| /** | ||
| * Time Grain class for Day. | ||
| */ | ||
| public class Day extends Date { | ||
| public class Day extends Date implements TimeGrainFormatter { |
There was a problem hiding this comment.
Why is a Day an instance of TimeGrainFormatter? Can we remove this association?
There was a problem hiding this comment.
TimeGrainFormatter has common logic that will be used by all the Grains for ISO handling. So instead of creating a Util class with that supporting method, we went the implement/interface route.
There was a problem hiding this comment.
maybe DaySerde can implement it instead of Day.
There was a problem hiding this comment.
We are calling a static method on the interface. Technically nothing needs to implement it. DaySerde would make more sense though if we do.
| } | ||
| date = new Day(FORMATTER.parse(FORMATTER.format(val))); |
There was a problem hiding this comment.
Don't we still need the else case? Otherwise this line stomps on line 36.
| import java.text.ParseException; | ||
| import java.text.SimpleDateFormat; | ||
|
|
||
| /** | ||
| * Time Grain class for Day. | ||
| */ | ||
| public class Day extends Date { | ||
| public class Day extends Date implements TimeGrainFormatter { |
There was a problem hiding this comment.
We are calling a static method on the interface. Technically nothing needs to implement it. DaySerde would make more sense though if we do.
| /** Interface for ISO timegrain Support. */ | ||
| public interface TimeGrainFormatter { | ||
|
|
||
| String ISO_FORMAT = "yyyy-MM-dd'T'HH:mm:ssZ"; |
There was a problem hiding this comment.
It should be "yyyy-MM-dd'T'HH:mm:ss'Z'", single quotes around Z
|
|
||
| static Timestamp formatDateString(SimpleDateFormat formatter, String val) throws ParseException { | ||
| try { | ||
| return new Timestamp(formatter.parse((String) val).getTime()); |
There was a problem hiding this comment.
You don't need to cast val here as a String.
| @Override | ||
| public Minute deserialize(Object val) { | ||
|
|
||
| Minute date = null; | ||
|
|
||
| try { | ||
| if (val instanceof String) { | ||
| date = new Minute(new Timestamp(FORMATTER.parse((String) val).getTime())); | ||
| date = new Minute(FORMATTER.parse(FORMATTER.format(TimeGrainFormatter.formatDateString(FORMATTER, |
There was a problem hiding this comment.
This doesn't look correct - why do we need to parse and then format, and then parse again? It looks like we are doing unnecessary work here.
| @Test | ||
| public void testISODateString() throws ParseException { | ||
| String dateInString = "2020-01-01T00:00:00-0500"; | ||
| Day expectedDate = new Day(formatter.parse(dateInString)); |
There was a problem hiding this comment.
This should be isoFormatter instead of formatter. Also might be a good idea to update dateInString to have different hour minutes.
Resolves #1545
Description
ISO Support for time grains
Motivation and Context
How Has This Been Tested?
Unit tests included
Screenshots (if appropriate):
License
I confirm that this contribution is made under an Apache 2.0 license and that I have the authority necessary to make this contribution on behalf of its copyright owner.