changefeedccl: fix bug with avro encoding and zero-scale decimal cols #118815
Conversation
andyyang890
requested review from
nicktrav,
rharding6373,
jayshrivastava,
wenyihu6 and
a team
|
This change is |
|
An alternative idea would be to either submit a patch to |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @nicktrav, @rharding6373, and @wenyihu6)
pkg/ccl/changefeedccl/avro.go line 78 at r1 (raw file):
SchemaType avroSchemaType `json:"type"` LogicalType string `json:"logicalType"` Precision int `json:"precision,omitempty"`
Looks like tests are failing due to precision 0 and scale 0 being included when they shouldn't. Maybe we need to do this https://stackoverflow.com/questions/38486564/unmarshal-marshal-json-with-int-set-to-0-does-not-seem-to-work
This patch fixes a bug where creating a changefeed that targeted tables with a `DECIMAL(n)` column (i.e. zero-scale `DECIMAL` column), `format='avro'`, and `diff` would cause a panic. The cause of this panic was the fact that the third-party `goavro` library we use expected the JSON encoding of the schema to have a numeric `scale` field for decimal types, but we omitted this field whenever it was zero (using `omitempty`), which led to a runtime type assertion failure. We've updated the field to a pointer type in our type definition so that we can distinguish between an unset value and a zero value. Release note (enterprise change): Fixed a bug where creating a changefeed that targeted tables with a `DECIMAL(n)` column (i.e. zero-scale `DECIMAL` column), `format='avro'`, and `diff` would cause a panic.
andyyang890
force-pushed
the
avro_decimal_fix
branch
from
a5273b7
to
907aaa6
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/changefeedccl/avro.go line 78 at r1 (raw file):
Previously, jayshrivastava (Jayant) wrote…
Looks like tests are failing due to precision 0 and scale 0 being included when they shouldn't. Maybe we need to do this https://stackoverflow.com/questions/38486564/unmarshal-marshal-json-with-int-set-to-0-does-not-seem-to-work
Done. Thanks for the pointer (no pun intended).
andyyang890
added
backport-23.1.x
There was a problem hiding this comment.
Reviewable status:
|
TFTRs! bors r=jayshrivastava,rharding6373 |
|
Build succeeded: |
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 907aaa6 to blathers/backport-release-23.1-118815: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
blathers backport 23.2.1-rc |
This patch fixes a bug where creating a changefeed that targeted
tables with a
DECIMAL(n)column (i.e. zero-scaleDECIMALcolumn),format='avro', anddiffwould cause a panic.The cause of this panic was the fact that the third-party
goavrolibrary we use expected the JSON encoding of the schema to have
a numeric
scalefield for decimal types, but we omitted thisfield whenever it was zero (using
omitempty), which led to aruntime type assertion failure. We've updated the field to a pointer
type in our type definition so that we can distinguish between an
unset value and a zero value.
Fixes #118647
Release note (enterprise change): Fixed a bug where creating a
changefeed that targeted tables with a
DECIMAL(n)column(i.e. zero-scale
DECIMALcolumn),format='avro', anddiffwould cause a panic.