iceberg: fix decimal type json writer #24463
Conversation
Missing fmt::format call resulting in ill-formed serialization of decimal iceberg field type.
andrwng
left a comment
There was a problem hiding this comment.
Thanks for catching and fixing! Probably worth an actual release note
|
Note to reviewers: Test escape as a result of the example schema being non-exhaustive with respect to allowable field types. I'll expand the test schema in a follow up and update any dependent tests as needed. |
Ya, my knowledge of the iceberg schema lexicon is pretty limited. Any suggestions for a more detailed note? |
nit: maybe mention Iceberg somehow? |
|
How about: "Fixes a bug where Iceberg topics with decimal fields would serialize manifests incorrectly" |
Ah yeah i deleted the word iceberg by accident 🤦 Anyway, looks good |
| void operator()(const iceberg::double_type&) { w.String("double"); } | ||
| void operator()(const iceberg::decimal_type& t) { | ||
| w.String("decimal({}, {})", t.precision, t.scale); | ||
| w.String(fmt::format("decimal({}, {})", t.precision, t.scale)); |
There was a problem hiding this comment.
weird was this invoking the String(char*, size_t, bool) interface?
There was a problem hiding this comment.
if so, then
would serialize manifests incorrectly
is an understatement. this fixes what could just be segfault
There was a problem hiding this comment.
Yea, fair. So maybe:
Fixes a bug where serializing manifests for Iceberg topics with decimal fields could cause Redpanda to crash or upload invalid manifests
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/59328#01939996-3469-465b-a369-6019db38791f |
|
/backport v24.3.x |
|
Nice, thanks, @oleiman ! |
Missing fmt::format call resulting in ill-formed serialization of decimal iceberg field type.
Backports Required
Release Notes
Bug Fixes