CDA-113 Adding csv support for timeseries#1780
Conversation
rma-bryson
force-pushed
the
feature/CDA-113-support-csv-for-time-series
branch
10 times, most recently
from
7219b05 to
8465b44
Compare
rma-bryson
force-pushed
the
feature/CDA-113-support-csv-for-time-series
branch
from
8465b44 to
76e0901
Compare
MikeNeilson
left a comment
MikeNeilson
left a comment
There was a problem hiding this comment.
Generally good. Definitely concerned about some critical logic duplication.
| qualityForNormalization | ||
| ).as("quality_norm"); | ||
|
|
||
| Field<Double> convertedValue = CWMS_UTIL_PACKAGE.call_CONVERT_UNITS( |
There was a problem hiding this comment.
AV_TSV_DQU where unit_id = 'X' will already provide only units in X, a call to cwms_util.convert_units is redundant.
There was a problem hiding this comment.
removed redundant unit conversion
There was a problem hiding this comment.
Turns out this was needed to handle bad user-requested units. The test_wrong_units() was failing in the TimeSeriesControllerTestIT. Adding this back in fixes it because the conversion procedure handles validation.
There was a problem hiding this comment.
Right, but why are will killing performance or a possible, but not common, failure?
I can see the value in the check, but a single called to the unit conversion itself will inform us if the unit it valid, without an additional context switch for every row.
... yes yes, it might be "common" but not in the various places of automation that will call this endpoint a lot, or from GUIs that may artificially limit the choices in the UI.
There was a problem hiding this comment.
Updated validation to a single indexed existence check before the data query, eliminating any per-row conversion overhead while still failing fast on invalid units.
Was able to get the example csv working
|
MikeNeilson
left a comment
MikeNeilson
left a comment
There was a problem hiding this comment.
Nice trick with the getExample method to fill out that detail.
Seems within that we could also setup our own annotation to retrieve a sample JSON.
rma-bryson
force-pushed
the
feature/CDA-113-support-csv-for-time-series
branch
from
4f0cc79 to
9f402f6
Compare
| qualityForNormalization | ||
| ).as("quality_norm"); | ||
|
|
||
| Field<Double> convertedValue = CWMS_UTIL_PACKAGE.call_CONVERT_UNITS( |
There was a problem hiding this comment.
Right, but why are will killing performance or a possible, but not common, failure?
I can see the value in the check, but a single called to the unit conversion itself will inform us if the unit it valid, without an additional context switch for every row.
... yes yes, it might be "common" but not in the various places of automation that will call this endpoint a lot, or from GUIs that may artificially limit the choices in the UI.
…Moved CsvOnDemandInputStream into its own file. Removed redundant unit conversion logic. Added csv example generator.
…nvalid requested units gracefully.
…handle invalid requested units gracefully." This reverts commit 9f402f6.
…re the data query, eliminating any per-row conversion overhead while still failing fast on invalid units.
rma-bryson
force-pushed
the
feature/CDA-113-support-csv-for-time-series
branch
from
9f402f6 to
ab100dc
Compare
| ); | ||
|
|
||
| if (!unitRequestedExists) { | ||
| String msg = units + " is not a valid unit for time series " + name; |
There was a problem hiding this comment.
User input, needs to go through the sanitizer.
I would've just called the procedure, but wasn't thinking about the unit system level.
There was a problem hiding this comment.
Updated to sanitize message. Also added a check that a row actually exists to guarantee failure is actually due to bad units
…ists to guarantee failure is units
2 participants
Summary
Implements csv support for timeseries. This should also streamline other data types supporting our custom csv formatting in the future - including metadata comment headers at top of payload, unit in column header, and optional-column inclusion via DTO annotations in the spirit of Jackson.
This also adds support for specifying date-format. Currently this only works for csv, but could be extended to other formats in the future.
Also noting decision during cda dev meeting to use query parameters for optional column inclusion and date-format. Updated ADR to reflect that.
Related Issue
Closes #1257
Validation
Added unit tests and integration tests
Checklist