Re: [DISCUSS] KIP-481: SerDe Improvements for Connect Decimal type in JSON

Thanks for all the changes, Almog. The current KIP looks good to me. Randall On Fri, Aug 30, 2019 at 11:28 AM Almog Gavra wrote: > Thanks again Randall! Here are the changes I made: > > - Defaults. The KIP had mentioned that the default would be BASE64 in the > "Public Interfaces" section. I ha

Re: [DISCUSS] KIP-481: SerDe Improvements for Connect Decimal type in JSON

Thanks again Randall! Here are the changes I made: - Defaults. The KIP had mentioned that the default would be BASE64 in the "Public Interfaces" section. I have also added your suggestion in "Proposed Changes". - I've added a bullet point to change the internal converter to use decimal.format=NUME

Re: [DISCUSS] KIP-481: SerDe Improvements for Connect Decimal type in JSON

Thanks for the updates, Almog. This looks really good, but I have a few more comments (most wording, though one potentially thorny issue): First, the KIP should define the default value for the `decimal.format` property. IIUC, it will be BASE64, and to gain the new behavior users will have to expl

Re: [DISCUSS] KIP-481: SerDe Improvements for Connect Decimal type in JSON

Thanks for the feedback Randall! I have updated the KIP with the following edits: * Updated the reference from "producer" to "source" (I had missed that one!) * Changed the config from "json.decimal.serialization.format" to "decimal.format" * Clarified case sensitivity * Clarified the proposed cha

Re: [DISCUSS] KIP-481: SerDe Improvements for Connect Decimal type in JSON

Thanks for all the work, Almog. For the most part, I think this KIP will be a great improvement, and IMO is almost ready to go. However, I do have a few suggestions that affect the wording more than the intent. First, the name of the `json.decimal.serialization.format` property is pretty long, es

Re: [DISCUSS] KIP-481: SerDe Improvements for Connect Decimal type in JSON

Thanks! KIP reads even better for me now. Just voted. +1 non-binding Konstantine On Wed, Aug 14, 2019 at 7:00 PM Almog Gavra wrote: > Thanks for the review Konstantine! > > I think the terminology suggestion definitely makes things clearer - I will > update the documentation based on your sugge

Re: [DISCUSS] KIP-481: SerDe Improvements for Connect Decimal type in JSON

Thanks for the review Konstantine! I think the terminology suggestion definitely makes things clearer - I will update the documentation based on your suggestion (e.g. s/Consumer/Sink Converter/g and s/Producer/Source Converter/g). Cheers, Almog On Wed, Aug 14, 2019 at 8:13 AM Konstantine Karanta

Re: [DISCUSS] KIP-481: SerDe Improvements for Connect Decimal type in JSON

Thanks Almog for preparing this KIP! I think it will improve usability and troubleshooting with JSON data a lot. The finalized plan seems quite concrete now. I also liked that some implementation specific implications (such as setting the ObjectMapper to deserialize floating point as BigDecimal) a

Re: [DISCUSS] KIP-481: SerDe Improvements for Connect Decimal type in JSON

Good catches! Fixed :) On Thu, Aug 8, 2019 at 10:36 PM Arjun Satish wrote: > Cool! > > Couple of nits: > > - In public interfaces, typo: *json.decimal.serialization.fromat* > - In public interfaces, you use the term "HEX" instead of "BASE64". > > > > On Wed, Aug 7, 2019 at 9:51 AM Almog Gavra w

Re: [DISCUSS] KIP-481: SerDe Improvements for Connect Decimal type in JSON

Cool! Couple of nits: - In public interfaces, typo: *json.decimal.serialization.fromat* - In public interfaces, you use the term "HEX" instead of "BASE64". On Wed, Aug 7, 2019 at 9:51 AM Almog Gavra wrote: > EDIT: everywhere I've been using "HEX" I meant to be using "BASE64". I will > update

Re: [DISCUSS] KIP-481: SerDe Improvements for Connect Decimal type in JSON

EDIT: everywhere I've been using "HEX" I meant to be using "BASE64". I will update the KIP to reflect this. On Wed, Aug 7, 2019 at 9:44 AM Almog Gavra wrote: > Thanks for the feedback Arjun! I'm happy changing the default config to > HEX instead of BINARY, no strong feelings there. > > I'll also

Re: [DISCUSS] KIP-481: SerDe Improvements for Connect Decimal type in JSON

Thanks for the feedback Arjun! I'm happy changing the default config to HEX instead of BINARY, no strong feelings there. I'll also clarify the example in the KIP to be clearer: - serialize the decimal field "foo" with value "10.2345" with the HEX setting: {"foo": "D3J5"} - serialize the decimal f

Re: [DISCUSS] KIP-481: SerDe Improvements for Connect Decimal type in JSON

hey Almog, nice work! couple of thoughts (hope I'm not late since you started the voting thread already): can you please add some examples to show the changes that you are proposing. makes me think that for a given decimal number, we will have two encodings: “asHex” and “asNumber”. should we call

Re: [DISCUSS] KIP-481: SerDe Improvements for Connect Decimal type in JSON

Hello Everyone, Summarizing an in-person discussion with Randall (this is copied from the KIP): The original KIP suggested supporting an additional representation - base10 encoded text (e.g. `{"asText":"10.2345"}`). This causes issues because it is impossible to disambiguate between TEXT and BINA

Re: [DISCUSS] KIP-481: SerDe Improvements for Connect Decimal type in JSON

I'm mostly happy with your current suggestion (two configs, one for serialization and one for deserialization) and your implementation suggestion. One thing to note: > We should _always_ be able to deserialize a standard JSON > number as a decimal I was doing some research into decimals and JSON,

Re: [DISCUSS] KIP-481: SerDe Improvements for Connect Decimal type in JSON

The way I see it, we need to control two seperate things: 1. How do we _deserialize_ a decimal type if we encounter a text node in the JSON?(We should _always_ be able to deserialize a standard JSON number as a decimal). 2. How do we chose how we want decimals to be _serialized_. This looks t

Re: [DISCUSS] KIP-481: SerDe Improvements for Connect Decimal type in JSON

Thanks for the replies Andy and Andrew (2x Andy?)! > Is the text decimal a base16 encoded number, or is it base16 encoded binary > form of the number? The conversion happens as decimal.unscaledValue().toByteArray() and then the byte array is converted to a hex string, so it's definitely the binar

Re: [DISCUSS] KIP-481: SerDe Improvements for Connect Decimal type in JSON

This is a bit orthogonal, but in JsonSchemaConverter I use JSONSchemas to indicate whether a JSON number should be deserialized as an integer or a decimal

Re: [DISCUSS] KIP-481: SerDe Improvements for Connect Decimal type in JSON

Hi Almog, Like the KIP - I think being able to support decimals in JSON in the same way most other systems do is a great improvement. It's not 100% clear to me from the KIP what the current format is. Is the text decimal a base16 encoded number, or is it base16 encoded binary form of the number?

[DISCUSS] KIP-481: SerDe Improvements for Connect Decimal type in JSON

Hi Everyone! Kicking off discussion for a new KIP: https://cwiki.apache.org/confluence/display/KAFKA/KIP-481%3A+SerDe+Improvements+for+Connect+Decimal+type+in+JSON For those who are interested, I have a prototype implementation that helped guide my design: https://github.com/agavra/kafka/pull/1