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

Add Binary Support for Oid.NUMERIC and Oid.NUMERIC_ARRAY #1636

Merged
merged 5 commits into from Dec 6, 2019

Conversation

@mahmoudbahaa
Copy link
Contributor

mahmoudbahaa commented Dec 4, 2019

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Does mvn checkstyle:check pass ?
  3. Have you added your new test classes to an existing test suite in alphabetical order?

Changes to Existing Features:

  • Does this break existing behaviour? If so please explain.
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully run tests with your changes locally?
@mahmoudbahaa mahmoudbahaa force-pushed the mahmoudbahaa:binary-support-for-number branch from d985177 to fa5c93e Dec 4, 2019
@mahmoudbahaa mahmoudbahaa changed the title Add Binary Support for Oid.NUMERIC and Oid.BIGDECIMAL Add Binary Support for Oid.NUMERIC and Oid.NUMERIC_ARRAY Dec 4, 2019
@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Dec 4, 2019

Awesome! Thanks,
Can you add a test for it and it looks like some tests may need to account for it .
Again, thanks

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Dec 4, 2019

@mahmoudbahaa actually all that needs to be done is to patch it via

diff --git a/pgjdbc/src/main/java/org/postgresql/jdbc/PgArray.java b/pgjdbc/src/main/java/org/postgresql/jdbc/PgArray.java
index b8b256a1..f7cedcb3 100644
--- a/pgjdbc/src/main/java/org/postgresql/jdbc/PgArray.java
+++ b/pgjdbc/src/main/java/org/postgresql/jdbc/PgArray.java
@@ -243,6 +243,9 @@ public class PgArray implements java.sql.Array {
           case Oid.FLOAT8:
             arr[i] = ByteConverter.float8(fieldBytes, pos);
             break;
+          case Oid.NUMERIC:
+            arr[i] = ByteConverter.numeric(fieldBytes,pos);
+            break;
           case Oid.TEXT:
           case Oid.VARCHAR:
             Encoding encoding = connection.getEncoding();
@@ -389,6 +392,8 @@ public class PgArray implements java.sql.Array {
         return Float.class;
       case Oid.FLOAT8:
         return Double.class;
+      case Oid.NUMERIC:
+        return BigDecimal.class;
       case Oid.TEXT:
       case Oid.VARCHAR:
         return String.class;
diff --git a/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java b/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java
index 4ef826fa..c11a0140 100644
--- a/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java
+++ b/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java
@@ -2345,7 +2345,7 @@ public class PgResultSet implements ResultSet, org.postgresql.PGRefCursorResultS
         }
         return toBigDecimal(trimMoney(String.valueOf(obj)), scale);
       } else {
-        Number num = ByteConverter.numeric(thisRow[columnIndex - 1]);
+        Number num = ByteConverter.numeric(thisRow[columnIndex - 1], 0);
         if (allowNaN && Double.isNaN(num.doubleValue())) {
           return Double.NaN;
         }
@@ -3023,7 +3023,7 @@ public class PgResultSet implements ResultSet, org.postgresql.PGRefCursorResultS
       case Oid.FLOAT8:
         return ByteConverter.float8(bytes, 0);
       case Oid.NUMERIC:
-        return ByteConverter.numeric(bytes).doubleValue();
+        return ByteConverter.numeric(bytes, 0).doubleValue();
     }
     throw new PSQLException(GT.tr("Cannot convert the column of type {0} to requested type {1}.",
         Oid.toString(oid), targetType), PSQLState.DATA_TYPE_MISMATCH);
@@ -3069,7 +3069,7 @@ public class PgResultSet implements ResultSet, org.postgresql.PGRefCursorResultS
         val = (long) ByteConverter.float8(bytes, 0);
         break;
       case Oid.NUMERIC:
-        Number num = ByteConverter.numeric(bytes);
+        Number num = ByteConverter.numeric(bytes, 0);
         if (num instanceof  BigDecimal) {
           val = ((BigDecimal) num).setScale(0 , RoundingMode.DOWN).longValueExact();
         } else {
diff --git a/pgjdbc/src/main/java/org/postgresql/util/ByteConverter.java b/pgjdbc/src/main/java/org/postgresql/util/ByteConverter.java
index 75ed5a34..80a77dd7 100644
--- a/pgjdbc/src/main/java/org/postgresql/util/ByteConverter.java
+++ b/pgjdbc/src/main/java/org/postgresql/util/ByteConverter.java
@@ -141,15 +141,15 @@ public class ByteConverter {
    * @param bytes array of bytes that can be decoded as an integer
    * @return integer
    */
-  public static Number numeric(byte []bytes) {
+  public static Number numeric(byte []bytes, int pos) {
     if (bytes.length < 8) {
       throw new IllegalArgumentException("number of bytes should be at-least 8");
     }
 
-    short len = ByteConverter.int2(bytes, 0);
-    short weight = ByteConverter.int2(bytes, 2);
-    short sign = ByteConverter.int2(bytes, 4);
-    short scale = ByteConverter.int2(bytes, 6);
+    short len = ByteConverter.int2(bytes, pos);
+    short weight = ByteConverter.int2(bytes, pos + 2);
+    short sign = ByteConverter.int2(bytes, pos + 4);
+    short scale = ByteConverter.int2(bytes, pos + 6);
 
     if (!(sign == NUMERIC_POS
         || sign == NUMERIC_NEG
@@ -168,7 +168,7 @@ public class ByteConverter {
     short[] digits = new short[len];
     int idx = 8;
     for (int i = 0; i < len; i++) {
-      short d = ByteConverter.int2(bytes, idx);
+      short d = ByteConverter.int2(bytes, pos + idx);
       idx += 2;
 
       if (d < 0 || d >= NBASE) {
@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Dec 5, 2019

@mahmoudbahaa any chance you can get to this today? I want to release this week and I'd like to have this in the release.

@mahmoudbahaa

This comment has been minimized.

Copy link
Contributor Author

mahmoudbahaa commented Dec 5, 2019

@davecramer I was already doing these changes but was caught in work please check now

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Dec 5, 2019

@mahmoudbahaa sorry to be pushy, but I did want to get this in. Thanks!

@mahmoudbahaa mahmoudbahaa force-pushed the mahmoudbahaa:binary-support-for-number branch from b906c8e to 8306d82 Dec 5, 2019
@mahmoudbahaa

This comment has been minimized.

Copy link
Contributor Author

mahmoudbahaa commented Dec 5, 2019

np, tests are green however I have 2 question:

  1. StatementTest.testFastCloses:937 always fails with timeout on my machine is that normal ? (I on Windows not Linux if that any indicator) i will try to linux later but wanted to confirm if this is a known issue or not.
  2. it seems some configurations in build.local.properties break tests for example if you set preparethreshold=-1 many test fails
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 6, 2019

Codecov Report

Merging #1636 into master will increase coverage by 0.02%.
The diff coverage is 67.14%.

@@             Coverage Diff              @@
##             master    #1636      +/-   ##
============================================
+ Coverage     69.06%   69.09%   +0.02%     
- Complexity     4073     4112      +39     
============================================
  Files           181      181              
  Lines         16863    16954      +91     
  Branches       2760     2789      +29     
============================================
+ Hits          11647    11714      +67     
- Misses         3943     3955      +12     
- Partials       1273     1285      +12
@mahmoudbahaa

This comment has been minimized.

Copy link
Contributor Author

mahmoudbahaa commented Dec 6, 2019

@davecramer done and all checks green

@davecramer davecramer merged commit c85b149 into pgjdbc:master Dec 6, 2019
3 checks passed
3 checks passed
codecov/project 69.09% (+0.02%) compared to 8106d3d
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.