From 557be43f898609080e492e544155ea9a9595b466 Mon Sep 17 00:00:00 2001 From: chunhui-shi Date: Thu, 25 Aug 2016 10:23:53 -0700 Subject: [PATCH] DRILL-4862: binary_string should use another buffer as out buffer to be used in more generic usage. Previously, binary_string uses input buffer as output buffer. So after calling binary_string, the original content will be destroyed. If therer are other expressions/functions need to access the original input buffer, they will get error results. This fix also set readerIndex and writerIndex correctly for output buffer, otherwise the consumer of the output buffer will hit issues. --- .../drill/common/util/DrillStringUtils.java | 13 ++++---- .../exec/expr/fn/impl/StringFunctions.java | 7 +++-- .../physical/impl/TestConvertFunctions.java | 30 +++++++++++++++++++ .../test/resources/functions/conv/conv.json | 4 +++ 4 files changed, 44 insertions(+), 10 deletions(-) create mode 100644 exec/java-exec/src/test/resources/functions/conv/conv.json diff --git a/common/src/main/java/org/apache/drill/common/util/DrillStringUtils.java b/common/src/main/java/org/apache/drill/common/util/DrillStringUtils.java index 4dad397874a..4e4042fe742 100644 --- a/common/src/main/java/org/apache/drill/common/util/DrillStringUtils.java +++ b/common/src/main/java/org/apache/drill/common/util/DrillStringUtils.java @@ -134,7 +134,7 @@ public static String toBinaryString(byte[] buf) { */ public static String toBinaryString(byte[] buf, int strStart, int strEnd) { StringBuilder result = new StringBuilder(); - for (int i = strStart; i < strEnd ; ++i) { + for (int i = strStart; i < strEnd; ++i) { appendByte(result, buf[i]); } return result.toString(); @@ -153,17 +153,16 @@ private static void appendByte(StringBuilder result, byte b) { } /** - * In-place parsing of a hex encoded binary string. + * parsing a hex encoded binary string and write to an output buffer. * * This function does not modify the {@code readerIndex} and {@code writerIndex} * of the byte buffer. * * @return Index in the byte buffer just after the last written byte. */ - public static int parseBinaryString(ByteBuf str, int strStart, int strEnd) { - int length = (strEnd - strStart); - int dstEnd = strStart; - for (int i = strStart; i < strStart+length ; i++) { + public static int parseBinaryString(ByteBuf str, int strStart, int strEnd, ByteBuf out) { + int dstEnd = 0; + for (int i = strStart; i < strEnd; i++) { byte b = str.getByte(i); if (b == '\\' && strEnd > i+3 @@ -177,7 +176,7 @@ public static int parseBinaryString(ByteBuf str, int strStart, int strEnd) { i += 3; // skip 3 } } - str.setByte(dstEnd++, b); + out.setByte(dstEnd++, b); } return dstEnd; } diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java b/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java index 50ff4359250..8196728d847 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java @@ -1540,15 +1540,16 @@ public void eval() { public static class BinaryString implements DrillSimpleFunc { @Param VarCharHolder in; @Output VarBinaryHolder out; + @Inject DrillBuf buffer; @Override public void setup() {} @Override public void eval() { - out.buffer = in.buffer; - out.start = in.start; - out.end = org.apache.drill.common.util.DrillStringUtils.parseBinaryString(in.buffer, in.start, in.end); + out.buffer = buffer.reallocIfNeeded(in.end - in.start); + out.start = out.end = 0; + out.end = org.apache.drill.common.util.DrillStringUtils.parseBinaryString(in.buffer, in.start, in.end, out.buffer); out.buffer.setIndex(out.start, out.end); } } diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestConvertFunctions.java b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestConvertFunctions.java index 80189d56c76..a0013a03a53 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestConvertFunctions.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestConvertFunctions.java @@ -655,6 +655,36 @@ public void testHadooopVInt() throws Exception { buffer.release(); } + @Test // DRILL-4862 + public void testBinaryString() throws Exception { + // TODO(DRILL-2326) temporary until we fix the scalar replacement bug for this case + final OptionValue srOption = setupScalarReplacementOption(bits[0], ScalarReplacementOption.TRY); + + try { + final String[] queries = { + "SELECT convert_from(binary_string(key), 'INT_BE') as intkey \n" + + "FROM cp.`functions/conv/conv.json`" + }; + + for (String query: queries) { + testBuilder() + .sqlQuery(query) + .ordered() + .baselineColumns("intkey") + .baselineValues(1244739896) + .baselineValues(new Object[] { null }) + .baselineValues(1313814865) + .baselineValues(1852782897) + .build() + .run(); + } + + } finally { + // restore the system option + restoreScalarReplacementOption(bits[0], srOption); + } + } + protected void verifySQL(String sql, T expectedResults) throws Throwable { verifyResults(sql, expectedResults, getRunResult(QueryType.SQL, sql)); } diff --git a/exec/java-exec/src/test/resources/functions/conv/conv.json b/exec/java-exec/src/test/resources/functions/conv/conv.json new file mode 100644 index 00000000000..9e8e667c9d2 --- /dev/null +++ b/exec/java-exec/src/test/resources/functions/conv/conv.json @@ -0,0 +1,4 @@ +{"row": "0", "key": "\\x4a\\x31\\x39\\x38", "key2": "4a313938", "kp1": "4a31", "kp2": "38"} +{"row": "1", "key": null, "key2": null, "kp1": null, "kp2": null} +{"row": "2", "key": "\\x4e\\x4f\\x39\\x51", "key2": "4e4f3951", "kp1": "4e4f", "kp2": "51"} +{"row": "3", "key": "\\x6e\\x6f\\x39\\x31", "key2": "6e6f3931", "kp1": "6e6f", "kp2": "31"}