[FLINK] Support flink timestamp types#54
Conversation
|
Timestamp with zone already implemented under prestosql, try reuse this. |
There was a problem hiding this comment.
Thanks for the change. One concern before merging: the new Flink timestamp types are not consistently wired into the string/type parsing paths. FlinkTimestampType::toString() returns TIMESTAMP(p) / TIMESTAMP_LTZ(p), but the current type parser only supports numeric parameters for DECIMAL(p, s); WORD(...) is parsed as a child type list, so parseType(type->toString()) will not round-trip for these new types. Also, getType() registers FLINK_TIMESTAMP / FLINK_TIMESTAMP_LTZ, while the public string representation exposes TIMESTAMP / TIMESTAMP_LTZ, which makes the external type names inconsistent.
Could you please either add parser/getType support for the string forms emitted by toString(), or change toString() to emit a registered/parseable representation? It would also be good to add tests for parseType(FLINK_TIMESTAMP(3)->toString()), parseType(FLINK_TIMESTAMP_LTZ(9)->toString()), getType(...), and invalid precision bounds.
code review by codex

No description provided.