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.

Sign up for GitHub

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

Jump to bottom

Add Kafka raw encoder #4417

Merged
merged 2 commits into from
Jul 22, 2020
Merged

Add Kafka raw encoder #4417

merged 2 commits into from
Jul 22, 2020

Conversation

charlesjmorgan
Copy link
Member

Add RawRowEncoder and RawRowEncoderFactory
Add test case in io.prestosql.plugin.kafka.TestKafkaIntegrationSmokeTest#testRoundTripAllFormats

@cla-bot cla-bot bot added the cla-signed label Jul 10, 2020
@charlesjmorgan charlesjmorgan requested review from losipiuk and aalbu July 10, 2020 15:38
@losipiuk
Copy link
Member

Make commit message just Add Kafka raw encoder. The fact that tests come together with implementation is natural.

@charlesjmorgan charlesjmorgan changed the title Add Kafka raw encoder and raw roundtrip test Add Kafka raw encoder Jul 10, 2020
losipiuk
losipiuk reviewed Jul 13, 2020
View reviewed changes
Copy link
Member

@losipiuk losipiuk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Too hard for me :) => a suggestion. Let me know what you think.

presto-kafka/src/main/java/io/prestosql/plugin/kafka/encoder/raw/RawRowEncoder.java Outdated Show resolved Hide resolved
presto-kafka/src/main/java/io/prestosql/plugin/kafka/encoder/raw/RawRowEncoder.java Outdated Show resolved Hide resolved
@charlesjmorgan
Copy link
Member Author

I think I simplified/separated the mapping logic, let me know if what I did works better/worse.

This was referenced Jul 14, 2020
Merged
Closed
losipiuk
losipiuk reviewed Jul 15, 2020
View reviewed changes
Copy link
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

Thanks!

I put bunch of suggestions but nothing super serious. It was much easier to read this for me, than previous version. Despite the fact that this one is probably a bit longer.

presto-kafka/src/main/java/io/prestosql/plugin/kafka/encoder/raw/RawRowEncoder.java Outdated Show resolved Hide resolved
presto-kafka/src/main/java/io/prestosql/plugin/kafka/encoder/raw/RawRowEncoder.java Outdated Show resolved Hide resolved
presto-kafka/src/main/java/io/prestosql/plugin/kafka/encoder/raw/RawRowEncoder.java Outdated Show resolved Hide resolved
presto-kafka/src/main/java/io/prestosql/plugin/kafka/encoder/raw/RawRowEncoder.java Outdated Show resolved Hide resolved
presto-kafka/src/main/java/io/prestosql/plugin/kafka/encoder/raw/RawRowEncoder.java Outdated Show resolved Hide resolved
presto-kafka/src/main/java/io/prestosql/plugin/kafka/encoder/raw/RawRowEncoder.java Outdated Show resolved Hide resolved
presto-kafka/src/main/java/io/prestosql/plugin/kafka/encoder/raw/RawRowEncoder.java Outdated Show resolved Hide resolved
presto-kafka/src/main/java/io/prestosql/plugin/kafka/encoder/raw/RawRowEncoder.java Outdated Show resolved Hide resolved
presto-kafka/src/main/java/io/prestosql/plugin/kafka/encoder/raw/RawRowEncoder.java Show resolved Hide resolved
presto-kafka/src/test/java/io/prestosql/plugin/kafka/TestKafkaIntegrationSmokeTest.java Show resolved Hide resolved
@charlesjmorgan
Copy link
Member Author

charlesjmorgan commented Jul 15, 2020 •
edited
Loading

  • Added a separate commit to add different rows to the round trip tests. I moved some helper methods into ColumnMapping.
  • Tried using a ColumnMappingBuilder class to create ColumnMapping's but realized that it would be easier to just change the type of end from OptionalInt to int. This way end always has a value and it's up to the RawRowEncoder constructor to make sure that the mappings for the columns are valid.
  • Made ColumnMapping static and parameterized the constructor with EncoderColumnHandle.

losipiuk
losipiuk reviewed Jul 16, 2020
View reviewed changes
Copy link
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

Thanks. Looks much better now. MInor comments.

presto-kafka/src/test/java/io/prestosql/plugin/kafka/TestKafkaIntegrationSmokeTest.java Outdated Show resolved Hide resolved
presto-kafka/src/main/java/io/prestosql/plugin/kafka/encoder/raw/RawRowEncoder.java Outdated Show resolved Hide resolved
presto-kafka/src/main/java/io/prestosql/plugin/kafka/encoder/raw/RawRowEncoder.java Outdated Show resolved Hide resolved
presto-kafka/src/main/java/io/prestosql/plugin/kafka/encoder/raw/RawRowEncoder.java Outdated Show resolved Hide resolved
@charlesjmorgan charlesjmorgan force-pushed the kafka-raw-encoder branch 2 times, most recently from fa0c340 to 91cf482 Compare July 16, 2020 20:13
@findepi findepi removed their request for review July 16, 2020 20:37
@charlesjmorgan charlesjmorgan mentioned this pull request Jul 17, 2020
Merged
@losipiuk losipiuk merged commit 8472b62 into trinodb:master Jul 22, 2020
@losipiuk losipiuk mentioned this pull request Jul 22, 2020
Closed
8 tasks
@charlesjmorgan charlesjmorgan deleted the kafka-raw-encoder branch June 7, 2021 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aalbu aalbu aalbu left review comments

@losipiuk losipiuk losipiuk approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@charlesjmorgan @losipiuk @aalbu