Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-5891: Proper handling of LogicalTypes in Cast #4633

Merged
merged 4 commits into from Aug 20, 2018

Conversation

maver1ck
Copy link
Contributor

@maver1ck maver1ck commented Mar 1, 2018

Currently logical types are dropped during Cast Transformation.
This patch fixes this behaviour.

Committer Checklist (excluded from commit message)

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

@ewencp ewencp added the connect label Mar 6, 2018
@maver1ck maver1ck changed the title [KAFKA-5891] Proper handling of LogicalTypes in Cast KAFKA-5891: Proper handling of LogicalTypes in Cast Mar 13, 2018
@amitsela
Copy link
Member

amitsela commented Mar 16, 2018

@ewencp is there an estimate on this PR?

@hachikuji
Copy link
Contributor

@maver1ck Thanks for the patch. Would you mind adding a test case?

@maver1ck
Copy link
Contributor Author

@hachikuji
No problem. I will add them during next week.

@maver1ck
Copy link
Contributor Author

@hachikuji
Test added.
WDYT ?

Copy link
Contributor

@rhauch rhauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @maver1ck. Generally this looks good, but in addition to the comment/request below it might help to describe how this PR fixes the problem.

@@ -331,6 +335,7 @@ public void castFieldsWithSchema() {
assertEquals(true, ((Struct) transformed.value()).schema().field("float64").schema().defaultValue());
assertEquals((byte) 1, ((Struct) transformed.value()).get("boolean"));
assertEquals(42, ((Struct) transformed.value()).get("string"));
assertEquals(new Date(0), ((Struct) transformed.value()).get("timestamp"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this PR modifies the logic that builds the schema, it might be good to add a block that tests the schemas of each field in the resulting schema. Something like the following (which I don't know if it compiles):


        Schema transformedSchema = ((Struct) transformed.value()).schema();
        assertEquals(Schema.INT16_SCHEMA, transformedSchema.field("int8").schema());
        assertEquals(Schema.OPTIONAL_INT32_SCHEMA, transformedSchema.field("int16").schema());
        assertEquals(Schema.INT64_SCHEMA, transformedSchema.field("int32").schema());
        assertEquals(Schema.BOOLEAN_SCHEMA, transformedSchema.field("int64").schema());
        assertEquals(Schema.FLOAT64_SCHEMA, transformedSchema.field("float32").schema());
        assertEquals(Schema.BOOLEAN_SCHEMA, transformedSchema.field("float64").schema());
        assertEquals(Schema.INT8_SCHEMA, transformedSchema.field("boolean").schema());
        assertEquals(Schema.INT32_SCHEMA, transformedSchema.field("string").schema());
        assertEquals(Schema.OPTIONAL_INT32_SCHEMA, transformedSchema.field("optional").schema());
        // The following fields are not changed
        //assertEquals(Timestamp.SCHEMA, transformedSchema.field("timestamp").schema());

@rhauch
Copy link
Contributor

rhauch commented Apr 9, 2018

@maver1ck, any thoughts on the previous suggestion I had regarding adding a few more checks in the tests?

@amitsela
Copy link
Member

@maver1ck @ewencp @rhauch any news on this?

Copy link
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the patch! Note that @rayokota submitted a separate patch with the additional test cases that @rhauch suggested.

@hachikuji hachikuji merged commit 504824b into apache:trunk Aug 20, 2018
hachikuji pushed a commit that referenced this pull request Aug 20, 2018
Currently logical types are dropped during Cast Transformation.
This patch fixes this behaviour.

Reviewers: Randall Hauch <rhauch@gmail.com>, Jason Gustafson <jason@confluent.io>
hachikuji pushed a commit that referenced this pull request Aug 20, 2018
Currently logical types are dropped during Cast Transformation.
This patch fixes this behaviour.

Reviewers: Randall Hauch <rhauch@gmail.com>, Jason Gustafson <jason@confluent.io>
rayokota added a commit to rayokota/kafka that referenced this pull request Aug 20, 2018
hachikuji pushed a commit that referenced this pull request Aug 20, 2018
Currently logical types are dropped during Cast Transformation.
This patch fixes this behaviour.

Reviewers: Randall Hauch <rhauch@gmail.com>, Jason Gustafson <jason@confluent.io>
Pengxiaolong pushed a commit to Pengxiaolong/kafka that referenced this pull request Jun 14, 2019
Currently logical types are dropped during Cast Transformation.
This patch fixes this behaviour.

Reviewers: Randall Hauch <rhauch@gmail.com>, Jason Gustafson <jason@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants