JDBC sources: Improve source column type logs #18193
Conversation
|
NOTE
|
|
meh, don't really want to publish this change on its own, going to just merge it |
| @@ -193,7 +193,7 @@ protected List<TableInfo<CommonField<Datatype>>> discoverInternal(final JdbcData | |||
| .map(f -> { | |||
| final Datatype datatype = getFieldType(f); | |||
| final JsonSchemaType jsonType = getType(datatype); | |||
| LOGGER.info("Table {} column {} (type {}[{}]) -> {}", | |||
| LOGGER.info("Table {} column {} (type {}[{}], nullable {}) -> {}", | |||
There was a problem hiding this comment.
Seeing as the (type {}[{}], nullable {}) isn't abundantly clear on what the format is can this also be updated to have explicit updates to say column size? I guess even the ending -> {} isn't explicitly clear that it represents jsonType information unless someone knew this LOGGER line existed
There was a problem hiding this comment.
welp. I timed that merge very poorly. do you feel strongly enough to warrant opening a new pr >.>
fwiw the syntax VARCHAR[123] is probably not awful, since most SQL-based systems will use something like e.g. NUMBER(42) to represent a 42-digit number. nullable false is... not great, but my hope is that it's at least understandable?
Final log message should look something like:
Table THE_TABLE_NAME column THE_COLUMN_NAME (type VARCHAR[16777216], nullable false) -> {"type": "string"}
which is hopefully not awful
There was a problem hiding this comment.
Based on what you've showed with an example log message, would say the current change is sufficient
These logs currently look like
Table THE_TABLE_NAME column THE_COLUMN_NAME (type VARCHAR[16777216]) -> false. Thatfalserepresents nullability, and we're not logging the json schema at all.Fix the log template so that it actually logs all the parameters, and puts them in the right place.