Typos
- parse_expr.c:2745: "formmatted" → "formatted"
- parse_coerce.c:156: "Cocerce" → "Coerce"
In `coerce_type_with_format()` (parse_coerce.c):
1. The check `if (fmtcategory != TYPCATEGORY_STRING && fmtcategory != TYPCATEGORY_UNKNOWN)`
could be moved earlier. It doesn't depend on any other
calculations, so failing fast here would avoid unnecessary work.
2. consider using` list_make2(node, format)` instead of `list_make1() + lappend()`.
--
Regards,
Man Zeng
Surya and I did a pair-review of this. In addition to the notes above (which we agree with), we have the following notes:
+ if (inputBaseTypeId == targetBaseTypeId)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot cast type %s to %s while using a format template",
+ format_type_be(inputBaseTypeId),
+ format_type_be(targetBaseTypeId)),
+ errdetail("binary coercible type cast is not supported while using a format template"),
+ parser_coercion_errposition(pstate, location, node));
This could use a bit more explanation in a comment - is it because there is no plausible type that can take a FORMAT and be cast to itself?
+ if (s_typcategory != TYPCATEGORY_NUMERIC &&
+ s_typcategory != TYPCATEGORY_STRING &&
+ s_typcategory != TYPCATEGORY_DATETIME &&
+ s_typcategory != TYPCATEGORY_TIMESPAN)
In situations like this, the committers have shown a strong preference for switch() statements. Though it may make more sense to package this if into a static function is_formattable_type() or similar.
+ if (t_typcategory == TYPCATEGORY_STRING)
+ funcname = list_make2(makeString("pg_catalog"),
+ makeString("to_char"));
...
+ else
+ elog(ERROR, "failed to find conversion function from %s to %s while using a format template",
+ format_type_be(inputTypeId), format_type_be(targetTypeId));
Similar thing here, committers will probably want to see a switch.
+create function imm_const() returns text as $$ begin return 'YYYY-MM-DD'; end; $$ language plpgsql immutable;;
+select cast(col1 as date format imm_const()) from tcast;
double-;
Overall, the test coverage appears complete and the close mirroring of existing tests makes the intention very clear.
