Bugfixes #129
Bugfixes #129
Conversation
- Timestamp field respects arraysize. - Arraysize=1 is ignored to be single letter field instead of an array.
| case 17: | ||
| case 18: | ||
| if (!isVariable) { | ||
| throw new IllegalArgumentException("Invalid array size " + arraysize + " for timestamp. Standard sizes are 10, 19, or 23."); |
There was a problem hiding this comment.
Please rename the String format (line 120) to something less confusing. I usually, use ret for the intended return value.
Would it be better to check the arraysize/isVariable in the constructor and throw the exception there? Here it happens when formatting the first value...
Since the parse method is more flexible, I could see a scenario where someone uploads a table with a bad arraysize, it is accepted, stored in db, and then selecting the timestamp column would fail on output. If we moved the arraysize/isVariable checks to the ctor, it would also fail on input... which is more strict than I envisioned but nominally better. Thoughts?
For the actual substring call: I see what you are doing with coercing arraysize of 10-18 into date only and I get that the assumption is that the input probably had date only (but the raw/full string value will be filled with 00:00:00.000 at this point)... I'm not sure that allowing arraysize="15" (eg) to work is better or worse than just rejecting it. Certainly the current logic relies heavily on how DateUtil.flexToDate in the parse method behaves. I think this argues for moving the checks to the ctor so they apply to input and output.
As a side effect of doing that, we could then truncate the IVOA_DATE_FORMAT string to the right length and create the DateFormat object in the ctor -- and then format() becomes very simple and probably can't fail. parse() becomes a little more tricky because we should only use flexToDate when isVariable is true...
There was a problem hiding this comment.
Pushed new version with added checks in constructor, simplified format method, and updated parse method.
Agree on date filled with 00:00:00.000. If this version does not look good, I will update it to allow standard sizes only, even for arraysize endong with *.
| // arraysize = null/0 and isVariable = null/true indicates * (no limit) | ||
| public UTCTimestampFormat(Integer arraysize, Boolean isVariable) { | ||
| if (isVariable == null) { | ||
| this.isVariable = isVariable = true; |
There was a problem hiding this comment.
something odd here; should be false??
| this.isVariable = isVariable; | ||
| } | ||
|
|
||
| if (arraysize == null || arraysize == 0 || arraysize == 23) { |
There was a problem hiding this comment.
For a default setup, we should simply provide a no-arg constructor. The it can be documented more simply. So no-arg ctor defaults to (23, true). That probably helps with backwards compat usage as well.
| if (arraysize == null || arraysize == 0 || arraysize == 23) { | ||
| arraysize = 23; // default to full timestamp | ||
| } | ||
| if (arraysize < 10 || arraysize > 23) { |
There was a problem hiding this comment.
I feel like valid combinations of args are:
(null, true) : aka arraysize="*"
(10, b) : date only
(19, b) : date+time to sec
(23, b) : date+time to millis
(null, true) == (23,true) == no-arg ctor
b = true or false (or null)
I feel like this ctor validation could be simplified alot, maybe by only allowing 10,19,23 and throwing for anything else??
aside: I think in practice only the 23* usage can actually be variable length (because the DateFormat may truncate trailing 0 in the milliseconds part) but it is probably fine to accept 10* and 19* even though output strings will never actually vary in size.
2 participants
CADC-14941 - VOTable and arraysize=1
CADC-14950 - cadc-dali: improve UTCTimeStampFormat to respect arraysize