Add time related built in function and cumulative value function #197
Conversation
| - column: day_of_week | ||
| expression: DayOfWeek(driver_table.Col('eta'), 'Asia/Jakarta') | ||
| - column: pretty_date | ||
| expression: ParseTimeStampsWithFormat(driver_table.Col('eta'), 'Asia/Jakarta', '2006-01-02') |
There was a problem hiding this comment.
I think ParseTimeStampsWithFormat as function name is quite long. Wdyt about FormatTimestamp for function name?
There was a problem hiding this comment.
Aggree, I think FormatTimestamp is better name
|
What is the usecase of |
|
Ready for review @pradithya @tiopramayudi |
api/pkg/transformer/symbol/time.go
Outdated
| panic(err) | ||
| } | ||
|
|
||
| timeLocation, err := LoadLocationCached(fmt.Sprintf("%s", tz)) |
There was a problem hiding this comment.
what happen if tz is:
- slice with single value
- slice with multiple values
There was a problem hiding this comment.
it will return error, hence panic because the location is not found
There was a problem hiding this comment.
do you think we need to validate the timezone data type here? we expect timezone to be single value to match timestamp
There was a problem hiding this comment.
I think it's better to be defensive and check the type as well
api/pkg/transformer/symbol/time.go
Outdated
| type LocationCache map[string]*time.Location | ||
|
|
||
| var ( | ||
| locationCache = LocationCache{} |
There was a problem hiding this comment.
Shall we use sync.Map instead? Based on the benchmark (write less, read more) the performance is the best. https://www.fatalerrors.org/a/map-concurrency-performance-and-principle-analysis-of-golang.html
api/pkg/transformer/symbol/time.go
Outdated
| panic(err) | ||
| } | ||
|
|
||
| timeLocation, err := LoadLocationCached(fmt.Sprintf("%s", tz)) |
There was a problem hiding this comment.
I think it's better to be defensive and check the type as well
docs/user-guide/transformer.md
Outdated
| @@ -85,76 +85,14 @@ transformerConfig: | |||
| features: # list of features to be retrieved | |||
| - name: merchant_t1_discovery:t1_estimate # feature name. make sure to include feature set | |||
| valueType: DOUBLE # feature value type | |||
| defaultValue: '0' # default value if feature value not exist | |||
| defaultValue: "0" # default value if feature value not exist | |||
| ``` | |||
|
|
|||
| > To learn more about JSON Path syntax, please go to [this link](https://goessner.net/articles/JsonPath/), [this](https://restfulapi.net/json-jsonpath/) or [this](https://support.smartbear.com/alertsite/docs/monitors/api/endpoint/jsonpath.html). | |||
|
|
|||
| ### List of Supported UDFs | |||
There was a problem hiding this comment.
It's not UDF. UDF is User-Defined Function. We should replace the mention of UDF in our document with "expression"
docs/user-guide/transformer_udf.md
Outdated
| | Time | [IsWeekend](#isweekend) | | ||
| | Time | [FormatTimestamp](#formattimestamp) | | ||
| | Time | [ParseTimestamp](#parsetimestamp) | | ||
| | Time | [ParseDateTime](#parsedatetime) | |
|
I love that we started to make better documentation about standard transformer!! |
api/pkg/transformer/symbol/time.go
Outdated
| } | ||
|
|
||
| location := &time.Location{} | ||
| if timezoneVals.Kind() == reflect.Slice { |
There was a problem hiding this comment.
can we move out the validation of timezonevals into separate function, i notice it also be used in ParseDateTime function ?
|
LGTM |
What this PR does / why we need it:
In this PR we add several built in functions that can be used as expression in standard transformer spec. Below are the functions:
IsWeekend(timestamp interface{}, timezone string) interface{}. Return whether a timestamp fall in weekend or notDayOfWeek(timestamp interface{}, timezone string). Return int day indicate day of the week.ParseTimeStampsWithFormat(timestamp interface{}, timezone, format string) interface{}. Parsing timestamp into specific formatCumulativeValue(values interface{}) []float64. Accumulate values based on the index, e.g values[1,2,3] => [1, 1+2, 1+2+3] => [1, 3, 6]Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?:
Checklist