-
Notifications
You must be signed in to change notification settings - Fork 34
[Plugin-1614][Plugin-1507][Plugin-1615][Plugin-1611] Int datatype changes #390
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
Conversation
3d4c4e7 to
575cd8d
Compare
7e239ad to
940e3ce
Compare
403dd5a to
800314e
Compare
mysql-plugin/src/main/java/io/cdap/plugin/mysql/MysqlConnectorConfig.java
Show resolved
Hide resolved
mysql-plugin/docs/Mysql-batchsink.md
Outdated
| | BOOL, BOOLEAN | boolean | | | ||
| | SMALLINT | int | | | ||
| | MEDIUMINT | double | | | ||
| | MEDIUMINT | int | Users can manually set output schema to map it to Long. | |
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.
what does this comment mean? There is no output schema for a sink.
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.
There is similar existing comment for YEAR. Seems like this table was just copied from the source? For a sink I would expect to find what CDAP types can be written to which MySQL types
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.
Currently, all the Sink documents present in the database-plugins project contain a copy of mapping details from their source plugins. I believe it might be a little tricky in adding a CDAP to sink type mapping, given we have multiple types mapped to single sink types. I feel that might be a bigger documentation change, which might require some more thoughts.
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.
ok, though can we remove the comments column in the sink doc? I think it's pretty confusing.
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.
I will work on this comment in the next set of changes, proposing to take this after this PR submission.
mysql-plugin/src/main/java/io/cdap/plugin/mysql/MysqlConnectorConfig.java
Show resolved
Hide resolved
mysql-plugin/src/main/java/io/cdap/plugin/mysql/MysqlDBRecord.java
Outdated
Show resolved
Hide resolved
albertshau
left a comment
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.
minor suggestion
albertshau
left a comment
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.
minor suggestion
b4fc77d to
e6f0e41
Compare
albertshau
left a comment
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.
minor comment, lgtm
0678238 to
4a1b678
Compare
| prop.put(JDBC_PROPERTY_SOCKET_TIMEOUT, "20000"); | ||
| prop.put(JDBC_REWRITE_BATCHED_STATEMENTS, "true"); | ||
| // MySQL property to ensure that TINYINT(1) type data is not converted to MySQL Bit/Boolean type in the ResultSet. | ||
| prop.put(MYSQL_TINYINT1_IS_BIT, "false"); |
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.
Does it makes sense to use putIfAbsent() so that if user has provided it in connection args in plugin config we don't end up overriding it?
JIRA
https://cdap.atlassian.net/browse/PLUGIN-1614
https://cdap.atlassian.net/browse/PLUGIN-1507
https://cdap.atlassian.net/browse/PLUGIN-1615
https://cdap.atlassian.net/browse/PLUGIN-1611