[CALCITE-921] Fix incorrectness when calling getString() on binary data (Josh Elser)
Close apache/incubator-calcite#160 Project: http://git-wip-us.apache.org/repos/asf/calcite/repo Commit: http://git-wip-us.apache.org/repos/asf/calcite/commit/6f326d03 Tree: http://git-wip-us.apache.org/repos/asf/calcite/tree/6f326d03 Diff: http://git-wip-us.apache.org/repos/asf/calcite/diff/6f326d03 Branch: refs/heads/branch-release Commit: 6f326d0391923f4c4af5f9e60eb2ce263fdad081 Parents: bf178d5 Author: Josh Elser <[email protected]> Authored: Tue Oct 13 19:31:47 2015 -0400 Committer: Julian Hyde <[email protected]> Committed: Mon Oct 26 12:13:43 2015 -0700 ---------------------------------------------------------------------- .../calcite/avatica/remote/RemoteMetaTest.java | 31 +++++++++++++++++ .../calcite/avatica/remote/AbstractService.java | 22 +++++++++++- .../calcite/avatica/remote/JsonService.java | 4 +++ .../calcite/avatica/remote/ProtobufService.java | 5 +++ .../calcite/avatica/util/AbstractCursor.java | 35 ++++++++++++++++++-- 5 files changed, 93 insertions(+), 4 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/calcite/blob/6f326d03/avatica-server/src/test/java/org/apache/calcite/avatica/remote/RemoteMetaTest.java ---------------------------------------------------------------------- diff --git a/avatica-server/src/test/java/org/apache/calcite/avatica/remote/RemoteMetaTest.java b/avatica-server/src/test/java/org/apache/calcite/avatica/remote/RemoteMetaTest.java index 3ccebf8..e4d8690 100644 --- a/avatica-server/src/test/java/org/apache/calcite/avatica/remote/RemoteMetaTest.java +++ b/avatica-server/src/test/java/org/apache/calcite/avatica/remote/RemoteMetaTest.java @@ -42,13 +42,16 @@ import org.junit.runners.Parameterized.Parameters; import java.lang.reflect.Field; import java.lang.reflect.Method; +import java.nio.charset.StandardCharsets; import java.sql.Array; import java.sql.Connection; import java.sql.DriverManager; +import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; import java.sql.Statement; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.Map; import java.util.Random; @@ -416,6 +419,34 @@ public class RemoteMetaTest { ConnectionSpec.getDatabaseLock().unlock(); } } + + @Test public void testBinaryAndStrings() throws Exception { + final String tableName = "testbinaryandstrs"; + final byte[] data = "asdf".getBytes(StandardCharsets.UTF_8); + ConnectionSpec.getDatabaseLock().lock(); + try (final Connection conn = DriverManager.getConnection(url); + final Statement stmt = conn.createStatement()) { + assertFalse(stmt.execute("DROP TABLE IF EXISTS " + tableName)); + assertFalse(stmt.execute("CREATE TABLE " + tableName + "(id int, bin BINARY(4))")); + try (final PreparedStatement prepStmt = conn.prepareStatement( + "INSERT INTO " + tableName + " values(1, ?)")) { + prepStmt.setBytes(1, data); + assertFalse(prepStmt.execute()); + } + try (ResultSet results = stmt.executeQuery("SELECT id, bin from " + tableName)) { + assertTrue(results.next()); + assertEquals(1, results.getInt(1)); + // byte comparison should work + assertArrayEquals("Bytes were " + Arrays.toString(results.getBytes(2)), + data, results.getBytes(2)); + // as should string + assertEquals(new String(data, StandardCharsets.UTF_8), results.getString(2)); + assertFalse(results.next()); + } + } finally { + ConnectionSpec.getDatabaseLock().unlock(); + } + } } // End RemoteMetaTest.java http://git-wip-us.apache.org/repos/asf/calcite/blob/6f326d03/avatica/src/main/java/org/apache/calcite/avatica/remote/AbstractService.java ---------------------------------------------------------------------- diff --git a/avatica/src/main/java/org/apache/calcite/avatica/remote/AbstractService.java b/avatica/src/main/java/org/apache/calcite/avatica/remote/AbstractService.java index e88a47c..93ad3ef 100644 --- a/avatica/src/main/java/org/apache/calcite/avatica/remote/AbstractService.java +++ b/avatica/src/main/java/org/apache/calcite/avatica/remote/AbstractService.java @@ -29,6 +29,19 @@ import java.util.List; */ public abstract class AbstractService implements Service { + /** + * Represents the serialization of the data over a transport. + */ + enum SerializationType { + JSON, + PROTOBUF + } + + /** + * @return The manner in which the data is serialized. + */ + abstract SerializationType getSerializationType(); + /** Modifies a signature, changing the representation of numeric columns * within it. This deals with the fact that JSON transmits a small long value, * or a float which is a whole number, as an integer. Thus the accessors need @@ -68,7 +81,14 @@ public abstract class AbstractService implements Service { switch (column.type.id) { case Types.VARBINARY: case Types.BINARY: - return column.setRep(ColumnMetaData.Rep.STRING); + switch (getSerializationType()) { + case JSON: + return column.setRep(ColumnMetaData.Rep.STRING); + case PROTOBUF: + return column; + default: + throw new IllegalStateException("Unhadled case statement"); + } case Types.DECIMAL: case Types.NUMERIC: return column.setRep(ColumnMetaData.Rep.NUMBER); http://git-wip-us.apache.org/repos/asf/calcite/blob/6f326d03/avatica/src/main/java/org/apache/calcite/avatica/remote/JsonService.java ---------------------------------------------------------------------- diff --git a/avatica/src/main/java/org/apache/calcite/avatica/remote/JsonService.java b/avatica/src/main/java/org/apache/calcite/avatica/remote/JsonService.java index 86d69f1..a223069 100644 --- a/avatica/src/main/java/org/apache/calcite/avatica/remote/JsonService.java +++ b/avatica/src/main/java/org/apache/calcite/avatica/remote/JsonService.java @@ -43,6 +43,10 @@ public abstract class JsonService extends AbstractService { * responses to and from the peer service. */ public abstract String apply(String request); + @Override SerializationType getSerializationType() { + return SerializationType.JSON; + } + //@VisibleForTesting protected static <T> T decode(String response, Class<T> expectedType) throws IOException { http://git-wip-us.apache.org/repos/asf/calcite/blob/6f326d03/avatica/src/main/java/org/apache/calcite/avatica/remote/ProtobufService.java ---------------------------------------------------------------------- diff --git a/avatica/src/main/java/org/apache/calcite/avatica/remote/ProtobufService.java b/avatica/src/main/java/org/apache/calcite/avatica/remote/ProtobufService.java index 35cb35a..8414708 100644 --- a/avatica/src/main/java/org/apache/calcite/avatica/remote/ProtobufService.java +++ b/avatica/src/main/java/org/apache/calcite/avatica/remote/ProtobufService.java @@ -17,6 +17,7 @@ package org.apache.calcite.avatica.remote; import com.google.protobuf.Descriptors.Descriptor; + import com.google.protobuf.Message; /** @@ -30,6 +31,10 @@ public abstract class ProtobufService extends AbstractService { */ public abstract Response _apply(Request request); + @Override SerializationType getSerializationType() { + return SerializationType.PROTOBUF; + } + @Override public ResultSetResponse apply(CatalogsRequest request) { return finagle((ResultSetResponse) _apply(request)); } http://git-wip-us.apache.org/repos/asf/calcite/blob/6f326d03/avatica/src/main/java/org/apache/calcite/avatica/util/AbstractCursor.java ---------------------------------------------------------------------- diff --git a/avatica/src/main/java/org/apache/calcite/avatica/util/AbstractCursor.java b/avatica/src/main/java/org/apache/calcite/avatica/util/AbstractCursor.java index cd65199..dd38da0 100644 --- a/avatica/src/main/java/org/apache/calcite/avatica/util/AbstractCursor.java +++ b/avatica/src/main/java/org/apache/calcite/avatica/util/AbstractCursor.java @@ -26,6 +26,7 @@ import java.lang.reflect.Field; import java.math.BigDecimal; import java.math.RoundingMode; import java.net.URL; +import java.nio.charset.StandardCharsets; import java.sql.Array; import java.sql.Blob; import java.sql.Clob; @@ -782,7 +783,7 @@ public abstract class AbstractCursor implements Cursor { } //FIXME: Protobuf gets byte[] - public byte[] getBytes() { + @Override public byte[] getBytes() { Object obj = getObject(); try { final ByteString o = (ByteString) obj; @@ -791,6 +792,19 @@ public abstract class AbstractCursor implements Cursor { return obj == null ? null : (byte[]) obj; } } + + @Override public String getString() { + Object o = getObject(); + if (null == o) { + return null; + } + if (o instanceof byte[]) { + return new String((byte[]) o, StandardCharsets.UTF_8); + } else if (o instanceof ByteString) { + return ((ByteString) o).toString(); + } + throw new IllegalStateException("Unhandled value type: " + o.getClass()); + } } /** @@ -810,17 +824,32 @@ public abstract class AbstractCursor implements Cursor { @Override public byte[] getBytes() { // JSON sends this as a base64-enc string, protobuf can do binary. Object obj = getObject(); + if (obj instanceof byte[]) { // If we already have bytes, just send them back. return (byte[]) obj; } - final String string = getString(); - if (string == null) { + return getBase64Decoded(); + } + + private byte[] getBase64Decoded() { + final String string = super.getString(); + if (null == string) { return null; } + // Need to base64 decode the string. return ByteString.parseBase64(string); } + + @Override public String getString() { + final byte[] bytes = getBase64Decoded(); + if (null == bytes) { + return null; + } + // Need to base64 decode the string. + return new String(bytes, StandardCharsets.UTF_8); + } } /**
