You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by GitBox <gi...@apache.org> on 2020/06/10 06:52:48 UTC

[GitHub] [shardingsphere] taojintianxia opened a new pull request #5976: Fix: fix the type transfer issue (#5957)

taojintianxia opened a new pull request #5976:
URL: https://github.com/apache/shardingsphere/pull/5976


   Fixes #5957.
   
   Changes proposed in this pull request:
   - whenever the BIGINT in this situation will transfer to String, so set the jdbc type explicitly


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] Lucas-307 commented on a change in pull request #5976: Fix: fix the type transfer issue (#5957)

Posted by GitBox <gi...@apache.org>.
Lucas-307 commented on a change in pull request #5976:
URL: https://github.com/apache/shardingsphere/pull/5976#discussion_r437941795



##########
File path: shardingsphere-scaling/shardingsphere-scaling-core/src/main/java/org/apache/shardingsphere/scaling/core/execute/executor/importer/AbstractJDBCImporter.java
##########
@@ -123,12 +126,24 @@ private void executeInsert(final Connection connection, final DataRecord record)
         ps.setQueryTimeout(30);
         try {
             for (int i = 0; i < record.getColumnCount(); i++) {
-                ps.setObject(i + 1, record.getColumn(i).getValue());
+                SQLType sqlType = extractTargetSQLType(record.getColumn(i).getValue());
+                if (sqlType != null) {
+                    ps.setObject(i + 1, record.getColumn(i).getValue(), sqlType);
+                } else {
+                    ps.setObject(i + 1, record.getColumn(i).getValue());
+                }
             }
             ps.execute();
         } catch (SQLIntegrityConstraintViolationException ignored) {
         }
     }
+
+    private SQLType extractTargetSQLType(final Object recordValue) {
+        if (recordValue instanceof BigInteger) {
+            return JDBCType.BIGINT;
+        }
+        return null;
+    }

Review comment:
       Only process Bigint is not eough, we'd better cover all type.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] taojintianxia commented on pull request #5976: Fix: fix the type transfer issue (#5957)

Posted by GitBox <gi...@apache.org>.
taojintianxia commented on pull request #5976:
URL: https://github.com/apache/shardingsphere/pull/5976#issuecomment-642405479


   > This will implicit conversion in mysql, should the proxy support implicit conversion?
   
   Integer or Long or other will be transfered by mysql , but not BigInteger. and JDBC driver 8.x seems have this implicit conversion


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] terrymanu commented on a change in pull request #5976: Fix: fix the type transfer issue (#5957)

Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #5976:
URL: https://github.com/apache/shardingsphere/pull/5976#discussion_r437906870



##########
File path: shardingsphere-scaling/shardingsphere-scaling-core/src/main/java/org/apache/shardingsphere/scaling/core/execute/executor/importer/AbstractJDBCImporter.java
##########
@@ -123,12 +126,24 @@ private void executeInsert(final Connection connection, final DataRecord record)
         ps.setQueryTimeout(30);
         try {
             for (int i = 0; i < record.getColumnCount(); i++) {
-                ps.setObject(i + 1, record.getColumn(i).getValue());
+                SQLType sqlType = extractTargetSQLType(record.getColumn(i).getValue());
+                if (sqlType != null) {

Review comment:
       we prefer use `null != xxx` to instead of `xxx != null`




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] taojintianxia closed pull request #5976: Fix: fix the type transfer issue (#5957)

Posted by GitBox <gi...@apache.org>.
taojintianxia closed pull request #5976:
URL: https://github.com/apache/shardingsphere/pull/5976


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] taojintianxia commented on a change in pull request #5976: Fix: fix the type transfer issue (#5957)

Posted by GitBox <gi...@apache.org>.
taojintianxia commented on a change in pull request #5976:
URL: https://github.com/apache/shardingsphere/pull/5976#discussion_r437955555



##########
File path: shardingsphere-scaling/shardingsphere-scaling-core/src/main/java/org/apache/shardingsphere/scaling/core/execute/executor/importer/AbstractJDBCImporter.java
##########
@@ -123,12 +126,24 @@ private void executeInsert(final Connection connection, final DataRecord record)
         ps.setQueryTimeout(30);
         try {
             for (int i = 0; i < record.getColumnCount(); i++) {
-                ps.setObject(i + 1, record.getColumn(i).getValue());
+                SQLType sqlType = extractTargetSQLType(record.getColumn(i).getValue());
+                if (sqlType != null) {
+                    ps.setObject(i + 1, record.getColumn(i).getValue(), sqlType);
+                } else {
+                    ps.setObject(i + 1, record.getColumn(i).getValue());
+                }
             }
             ps.execute();
         } catch (SQLIntegrityConstraintViolationException ignored) {
         }
     }
+
+    private SQLType extractTargetSQLType(final Object recordValue) {
+        if (recordValue instanceof BigInteger) {
+            return JDBCType.BIGINT;
+        }
+        return null;
+    }

Review comment:
       I tried some types , currently just Bigint will cause that issue.I will test all types later , so that's why I make that check as a method, may add more types in `extractTargetSQLType `




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] codecov-commenter commented on pull request #5976: Fix: fix the type transfer issue (#5957)

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #5976:
URL: https://github.com/apache/shardingsphere/pull/5976#issuecomment-641787126


   # [Codecov](https://codecov.io/gh/apache/shardingsphere/pull/5976?src=pr&el=h1) Report
   > Merging [#5976](https://codecov.io/gh/apache/shardingsphere/pull/5976?src=pr&el=desc) into [master](https://codecov.io/gh/apache/shardingsphere/commit/0a19295740e26141f78e6fe1f132f59e16c639f6&el=desc) will **decrease** coverage by `0.00%`.
   > The diff coverage is `42.85%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/shardingsphere/pull/5976/graphs/tree.svg?width=650&height=150&src=pr&token=ZvlXpWa7so)](https://codecov.io/gh/apache/shardingsphere/pull/5976?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #5976      +/-   ##
   ============================================
   - Coverage     53.22%   53.21%   -0.01%     
     Complexity      438      438              
   ============================================
     Files          1186     1186              
     Lines         20878    20884       +6     
     Branches       3760     3762       +2     
   ============================================
   + Hits          11112    11114       +2     
   - Misses         9068     9070       +2     
   - Partials        698      700       +2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/shardingsphere/pull/5976?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...xecute/executor/importer/AbstractJDBCImporter.java](https://codecov.io/gh/apache/shardingsphere/pull/5976/diff?src=pr&el=tree#diff-c2hhcmRpbmdzcGhlcmUtc2NhbGluZy9zaGFyZGluZ3NwaGVyZS1zY2FsaW5nLWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NoYXJkaW5nc3BoZXJlL3NjYWxpbmcvY29yZS9leGVjdXRlL2V4ZWN1dG9yL2ltcG9ydGVyL0Fic3RyYWN0SkRCQ0ltcG9ydGVyLmphdmE=) | `79.71% <42.85%> (-4.42%)` | `0.00 <0.00> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/shardingsphere/pull/5976?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/shardingsphere/pull/5976?src=pr&el=footer). Last update [0a19295...7c97dde](https://codecov.io/gh/apache/shardingsphere/pull/5976?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] KomachiSion commented on pull request #5976: Fix: fix the type transfer issue (#5957)

Posted by GitBox <gi...@apache.org>.
KomachiSion commented on pull request #5976:
URL: https://github.com/apache/shardingsphere/pull/5976#issuecomment-641852833


   I think we should discuss more to solve the problem. From the issue, I think scaling parses the unsigned bigint as `BigInteger`, and `BigInteger` will be transfer as `String` in PreparedStatement as default.
   
   Or Scaling parses unsigned bigint as `String` directly, not `BigInteger`.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] taojintianxia commented on a change in pull request #5976: Fix: fix the type transfer issue (#5957)

Posted by GitBox <gi...@apache.org>.
taojintianxia commented on a change in pull request #5976:
URL: https://github.com/apache/shardingsphere/pull/5976#discussion_r437943544



##########
File path: shardingsphere-scaling/shardingsphere-scaling-core/src/main/java/org/apache/shardingsphere/scaling/core/execute/executor/importer/AbstractJDBCImporter.java
##########
@@ -123,12 +126,24 @@ private void executeInsert(final Connection connection, final DataRecord record)
         ps.setQueryTimeout(30);
         try {
             for (int i = 0; i < record.getColumnCount(); i++) {
-                ps.setObject(i + 1, record.getColumn(i).getValue());
+                SQLType sqlType = extractTargetSQLType(record.getColumn(i).getValue());
+                if (sqlType != null) {

Review comment:
       understood




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] taojintianxia commented on pull request #5976: Fix: fix the type transfer issue (#5957)

Posted by GitBox <gi...@apache.org>.
taojintianxia commented on pull request #5976:
URL: https://github.com/apache/shardingsphere/pull/5976#issuecomment-642623054


   this PR is not elegent, I will close this PR


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] avalon5666 commented on pull request #5976: Fix: fix the type transfer issue (#5957)

Posted by GitBox <gi...@apache.org>.
avalon5666 commented on pull request #5976:
URL: https://github.com/apache/shardingsphere/pull/5976#issuecomment-641930786


   This will implicit conversion in mysql, should the proxy support implicit conversion?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org