KAFKA-6684: Cast transform bytes by amitsela · Pull Request #4820 · apache/kafka

amitsela · 2018-04-03T16:17:48Z

Allow to cast LogicalType to string by calling the serialized (Java) object's toString().

Added tests for BigDecimal and Date as whole record and as fields.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

amitsela · 2018-04-03T16:18:20Z

This helps avoid dealing with bytes in downstream clients such as Python (no Avro) and loading csv/json into downstream databases.

amitsela · 2018-04-03T16:19:17Z

R: @rhauch @ewencp @hachikuji

amitsela · 2018-04-03T16:22:26Z

Marked as [WIP] cause Date.toString() doesn't use UTC I wanted to your opinion on that - should I make it print in UTC? like a proper ISO8601: yyyy-mm-ddTHH:MM:SSZ? or with msec yyyy-mm-ddTHH:MM:SS.fZ?

amitsela · 2018-04-03T17:10:31Z

fixed .* imports and rebased the commit as a fixup

ewencp · 2018-05-22T21:14:09Z

Most of this looks fine, but I think just adding BYTES to this isn't quite right. The way this was originally written assumes that all types this transformation supports have a sensible translation to every other type (which is why it stuck to numbers, boolean, and strings). I think you're trying to add support for an additional set of casts that make sense (e.g. most things have a sensible toString()), but now the input and output are not symmetric -- some things make sense as an input, but not as an output (e.g. BYTES). So a single list might not be sufficient to do these checks correctly (e.g. as it is today, if you tried to do something like bytes:string conversion, instead of catching it as a config issue and giving an easy to understand error, it would be caught later and just say it encountered an unexpected type). If the symmetry is no longer going to be there, I think we need to separate out what we support as input and what we support as output (and I'm not even sure that will be sufficient, but it seems like the minimum we'd need).

Makes sense. Let me check it out and push an update.

amitsela · 2018-05-23T02:00:23Z

@ewencp I think this makes more sense now.
What did you mean by

I'm not even sure that will be sufficient

What do you think is still missing?

On a separate note, WDYT about my note on Date.toString()?

asfgit · 2018-05-23T02:12:18Z

FAILURE
7773 tests run, 1 skipped, 2 failed.
--none--

rhauch · 2018-05-25T14:49:41Z

I don't particular like Date.toString() which outputs strings of the form dow mon dd hh:mm:ss zzz yyyy, or would it be better to use the standard ISO-8601 format? But then this begs the question whether this should know about Connect's Date, Timestamp, and Time logical types.

amitsela · 2018-05-25T15:41:50Z

True. To me it makes sense to say that casting to string uses the Object's toString() implementation, whatever that is.
For Date, there might not be a good reason to cast to string, especially when in serialized form it is still useful.
Gonna remove the [WIP] for this now.

amitsela · 2018-06-11T13:30:32Z

@ewencp @rhauch any thoughts? this PR opened on March 11, it's been a while.

amitsela · 2018-08-23T17:58:31Z

Resolved conflicts, squashed, and rebased.

rayokota · 2018-08-23T19:30:20Z

As @ewencp suggested, for consistency perhaps we can use the same string formats as SimpleHeaderConverter: https://github.com/apache/kafka/blob/trunk/connect/api/src/main/java/org/apache/kafka/connect/data/Values.java#L692-L695

Having them be inconsistent would be confusing IMHO.

amitsela · 2018-08-28T12:22:10Z

Sure thing! I'll update the PR.

amitsela · 2018-08-28T15:05:28Z

@rayokota @ewencp now it uses Values date formatter for consistency.
Also fixed the date pattern for year and day which had a bug.

I assume the goal would be to make the Cast transform use the Values conversion methods? makes sense but feels out-of-scope for this PR anyway.

amitsela · 2018-08-28T18:16:46Z

retest this please

rayokota · 2018-08-28T21:32:29Z

Thanks, @amitsela . Good catch on the broken date pattern. LGTM.

@ewencp , how does this look?

ewencp

seems very close... just a couple of minor comments

ewencp · 2018-09-10T02:10:32Z

hmm, is this just an underlying bug? should this bit be backported?

yeah that's just a bug I happened to stumble upon, probably should be backported.

ewencp · 2018-09-10T02:10:34Z

this is a public API change -- everything public in the Connect API is.

do we actually need this here? Scanning through this PR, it looks like it is used in one specific class and then test classes in the same package. Making it package-protected where it is actually used seems sufficient to me, no?

package-private is not enough since Cast is in package org.apache.kafka.connect.transforms tries to use Values#dateFormatter(java.util.Date) in org.apache.kafka.connect.data.
I think it's fine since we'd want anyone/anything that's manipulating "data values" to use the same formatter, no? so we give access to that method.

infer schema type from schema as fallback tests split cast validation to input/output use Values date fomratter if java.util.Date

amitsela · 2018-09-10T13:00:59Z

@ewencp I squashed and split the bug fix into it's own commit so you can cherry pick it easily.

amitsela · 2018-09-10T15:29:50Z

test failure seems unrelated, I'll kick Jenkins to try again

amitsela · 2018-09-10T15:30:17Z

jenkins retest this please

OneCricketeer · 2018-09-15T20:46:48Z

 public class CastTest {
    private final Cast<SourceRecord> xformKey = new Cast.Key<>();
    private final Cast<SourceRecord> xformValue = new Cast.Value<>();
+    private static final long MILLIS_PER_DAY = 24 * 60 * 60 * 1000;


minor: consider using TimeUnit.DAYS.toMillis(1)

Just being consistent with the project's coding conventions: https://github.com/apache/kafka/blob/trunk/connect/api/src/main/java/org/apache/kafka/connect/data/Values.java#L71

Could you do a static import for that Values class, then?

If a the committer wants, he can. This PR had a ridiculous back and forth and had been stretched for ages compared to what it covers.

lindong28 · 2018-09-25T19:01:37Z

Hey @ewencp, if this PR is close to be merged, would you have time to review it for 2.1.0 release?

Switches to normal year format instead of week date years and day of month instead of day of year. This is directly from #4820, but separated into a different JIRA/PR to keep the fixes independent. Original authorship should be maintained in the commit. Author: Amit Sela <amitsela33@gmail.com> Reviewers: Ewen Cheslack-Postava <ewen@confluent.io> Closes #5718 from ewencp/fix-header-converter-date-format (cherry picked from commit c1457be) Signed-off-by: Ewen Cheslack-Postava <me@ewencp.org>

Switches to normal year format instead of week date years and day of month instead of day of year. This is directly from #4820, but separated into a different JIRA/PR to keep the fixes independent. Original authorship should be maintained in the commit. Author: Amit Sela <amitsela33@gmail.com> Reviewers: Ewen Cheslack-Postava <ewen@confluent.io> Closes #5718 from ewencp/fix-header-converter-date-format

ewencp

@amitsela Thank you! Merging to trunk for 2.1.0.

Sorry for the (slow) back and forth. We prefer to be very certain of patches than to merge them too quickly, and in this case review throughput was limited.

I think there's still substantial follow-up to be done here re: handling of types and test coverage, but this is a significant improvement regardless.

ewencp · 2018-10-01T04:38:20Z


    private static String castToString(Object value) {
-        return value.toString();
+        if (value instanceof java.util.Date) {


Given we're doing this in the core api/transform code now, this probably warrants a more generalizable approach. On first iteration, the logical type classes (in o.a.k.connect.data) were sufficient, but it seems they may not quite expose enough info. In particular, other than the fromLogical return type and toLogical parameter type, they may not make it clear enough what the conversion to primitive type is. Also, they probably aren't sufficiently isolated.

We shouldn't block getting this in since Values currently tries to do its best to determine the correct type, though even that is subject to failure modes (e.g. @rhauch just because a timestamp falls on a day or within the first day after 0AD doesn't mean they actually are that type -- we should really be passing through schema info to determine that). Given lack of that info provided to currently public APIs, I don't think there are real compatibility issues -- in the future we'd want a more correct alternative we'd provide a separate multi-argument version that includes schema info and fallback to the single-argument version.

That makes sense.
One why to do this would be to create an abstraction o.a.k.connect.data.Logical which would avoid the whole "if isinstaceof.." code in Cast or any other place you're looking to convert to/from logical.

amitsela · 2018-10-01T16:17:19Z

I agree both with being thorough, and with the fact that there's a lot that could be done to further improve this area of the code.
Having said that, this PR was very focused and reviews (which weren't a lot of code at any point) sometimes took 1-2 months, but I guess that's due to limited committer resources.
Anyway, thanks!

Switches to normal year format instead of week date years and day of month instead of day of year. This is directly from apache#4820, but separated into a different JIRA/PR to keep the fixes independent. Original authorship should be maintained in the commit. Author: Amit Sela <amitsela33@gmail.com> Reviewers: Ewen Cheslack-Postava <ewen@confluent.io> Closes apache#5718 from ewencp/fix-header-converter-date-format

Allow to cast LogicalType to string by calling the serialized (Java) object's toString(). Added tests for `BigDecimal` and `Date` as whole record and as fields. Author: Amit Sela <amitsela33@gmail.com> Reviewers: Randall Hauch <rhauch@gmail.com>, Robert Yokota <rayokota@gmail.com>, Ewen Cheslack-Postava <ewen@confluent.io> Closes apache#4820 from amitsela/cast-transform-bytes

Allow to cast LogicalType to string by calling the serialized (Java) object's toString(). Added tests for `BigDecimal` and `Date` as whole record and as fields. Author: Amit Sela <amitsela33@gmail.com> Reviewers: Randall Hauch <rhauch@gmail.com>, Robert Yokota <rayokota@gmail.com>, Ewen Cheslack-Postava <ewen@confluent.io> Closes #4820 from amitsela/cast-transform-bytes

amitsela force-pushed the cast-transform-bytes branch from 7c53447 to 5b7166d Compare April 3, 2018 17:09

ewencp reviewed May 22, 2018

View reviewed changes

amitsela force-pushed the cast-transform-bytes branch from a71579d to cbfa8f9 Compare May 23, 2018 12:52

amitsela changed the title ~~KAFKA-6684 [WIP]: Cast transform bytes~~ KAFKA-6684: Cast transform bytes May 25, 2018

amitsela force-pushed the cast-transform-bytes branch from cbfa8f9 to 50783f8 Compare August 23, 2018 15:54

ewencp reviewed Sep 10, 2018

View reviewed changes

amitsela added 2 commits September 10, 2018 08:54

allow casting bytes to string

6d29efb

infer schema type from schema as fallback tests split cast validation to input/output use Values date fomratter if java.util.Date

fix date format pattern

cfd9cb6

amitsela force-pushed the cast-transform-bytes branch from 18cbc78 to cfd9cb6 Compare September 10, 2018 12:56

OneCricketeer reviewed Sep 15, 2018

View reviewed changes

ewencp mentioned this pull request Sep 30, 2018

KAFKA-7460: Fix Connect Values converter date format pattern #5718

Closed

3 tasks

ewencp approved these changes Oct 1, 2018

View reviewed changes

ewencp closed this in fd44dc7 Oct 1, 2018

amitsela deleted the cast-transform-bytes branch October 1, 2018 16:17

ncliang mentioned this pull request Sep 23, 2019

KAFKA-6290: Support casting from logical types in cast transform #7371

Merged

3 tasks

kkonstantine added the connect label Oct 16, 2020

rhauch mentioned this pull request Feb 22, 2021

KAFKA-12170: Fix for Connect Cast SMT to correctly transform a Byte array into a string #9950

Merged

1 task

Conversation

amitsela commented Apr 3, 2018

Committer Checklist (excluded from commit message)

Uh oh!

amitsela commented Apr 3, 2018

Uh oh!

amitsela commented Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amitsela commented Apr 3, 2018

Uh oh!

amitsela commented Apr 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amitsela commented May 23, 2018

Uh oh!

asfgit commented May 23, 2018

Uh oh!

rhauch commented May 25, 2018

Uh oh!

amitsela commented May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amitsela commented Jun 11, 2018

Uh oh!

amitsela commented Aug 23, 2018

Uh oh!

rayokota commented Aug 23, 2018

Uh oh!

amitsela commented Aug 28, 2018

Uh oh!

amitsela commented Aug 28, 2018

Uh oh!

amitsela commented Aug 28, 2018

Uh oh!

rayokota commented Aug 28, 2018

Uh oh!

ewencp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amitsela commented Sep 10, 2018

Uh oh!

amitsela commented Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amitsela commented Sep 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amitsela Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lindong28 commented Sep 25, 2018

Uh oh!

ewencp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amitsela commented Oct 1, 2018

Uh oh!

Reviewers

amitsela commented Apr 3, 2018 •

edited

Loading

amitsela commented May 25, 2018 •

edited

Loading

amitsela commented Sep 10, 2018 •

edited

Loading

amitsela Sep 17, 2018 •

edited

Loading