-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
[FLINK-3872] [table, connector-kafka] Add KafkaJsonTableSource #2069
Conversation
Quickly checked the maven and kafka stuff. Looks good. Very clean, well documented and tested code. |
this.fieldNames = Preconditions.checkNotNull(fieldNames, "Field names"); | ||
this.fieldTypes = Preconditions.checkNotNull(fieldTypes, "Field types"); | ||
|
||
Preconditions.checkArgument(fieldNames.length == fieldNames.length, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be fieldNames.length == fieldTypes.length
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Good catch
Hi @uce, thanks for the PR. Looks really good. I added three comments inline. Regarding the watermarks and timestamp handling, I am thinking about introducing an |
05272ba
to
90d4ad3
Compare
Thanks for the review Fabian. I've addressed all comments except the ObjectMapper configuration. The EvenTimeTableSource makes sense. We can make the Kafka source extend from that one later on. Do you think we can merge this now? |
Hi @uce , the update looks good. I think we should add the new TableSources to the Table API documentation. Maybe adding a table to like to | Class name | Maven dep | BatchSource | Streaming | Description |
- Adds a deserialization schema from byte[] to Row to be used in conjunction with the Table API.
90d4ad3
to
c8766dd
Compare
Thanks for the suggestion Fabian. I've added the table to the docs and added an example about the Kafka JSON source. Furthermore, I've added the configuration flag for the missing field failure behaviour. I'll merge this after my local Travis build passes. |
Test failure is unrelated, going to merge this. |
Adds
StreamTableSource
variants for Kafka with syntactic sugar for parsing JSON streams.You can then continue to work with the stream:
Limitations
/location/area
as field names).