----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21614/#review46234 -----------------------------------------------------------
Replace TABS with 2 spaces in all places. exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java <https://reviews.apache.org/r/21614/#comment81541> Shouldn't out.start be equal to text.start? exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java <https://reviews.apache.org/r/21614/#comment81537> This condition will always be true. A byte value range from -128 to 127 and 0x128 = 296. exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java <https://reviews.apache.org/r/21614/#comment81536> Make org.apache.drill.exec.expr.fn.impl.StringFunctionUtil.utf8CharLen(ByteBuf, int) a public function and use that. exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java <https://reviews.apache.org/r/21614/#comment81538> This condition will always be true. exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java <https://reviews.apache.org/r/21614/#comment81544> what is the purpose of this assignment? exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java <https://reviews.apache.org/r/21614/#comment81545> Javadoc is not in sync with the function. exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java <https://reviews.apache.org/r/21614/#comment81547> 1. Since the encoding is going to be constant, why not extract the string value in the setup() method? 2. Use the constant 'com.google.common.base.Charsets.UTF_8' instead. exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java <https://reviews.apache.org/r/21614/#comment81548> Why not wrap the buffer around the byte array created in the previous line instead of allocating a new one of random size and copying again in line 1103. exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java <https://reviews.apache.org/r/21614/#comment81549> This should be out.end = bytes.length; 'end' index is exclusive. exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java <https://reviews.apache.org/r/21614/#comment81550> This is wrong. It will reverse the byte array an not the string. - Aditya Kishore On June 17, 2014, 10:34 p.m., Yash Sharma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21614/ > ----------------------------------------------------------- > > (Updated June 17, 2014, 10:34 p.m.) > > > Review request for drill, Aditya Kishore and Mehant Baid. > > > Repository: drill-git > > > Description > ------- > > Added implementation for String functions: > - ascii(str) > - toascii(str, srcEncoding) > - btrim(str, chars) > - char(int) > - repeat(str, numOfTimes) > - reverse(str) > > > Diffs > ----- > > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java > 51a7dbb > > exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestStringFunctions.java > af741a5 > exec/java-exec/src/test/resources/functions/string/testStringFuncs.json > PRE-CREATION > > Diff: https://reviews.apache.org/r/21614/diff/ > > > Testing > ------- > > Yes. > > $ mvn test -Dtest=TestStringFunctions#testNewStringFuncs > ------------------------------- > ACTUAL EXPECTED > ------------------------------- > 97 97 > 65 65 > -32 -32 > A A > trim trim > Peace Peace Peace Peace Peace Peace > ????? ????? ????? ????? ????? ????? ????? ????? > katcit katcit > âpple âpple > ------------------------------- > > > SQLLINE TESTS: > > SELECT employee_id, toascii('âpple','ISO-8859-1') as ascii_str FROM > cp.`employee.json` limit 10; > +-------------+------------+ > | employee_id | ascii_str | > +-------------+------------+ > | 1 | âpple | > | 2 | âpple | > | 4 | âpple | > | 5 | âpple | > | 6 | âpple | > | 7 | âpple | > | 8 | âpple | > | 9 | âpple | > | 10 | âpple | > | 11 | âpple | > +-------------+------------+ > 10 rows selected (0.315 seconds) > > > SELECT employee_id, first_name as first_name, reverse(first_name) as > revrese_name, repeat(first_name,2) as repeat_name FROM cp.`employee.json` > limit 10; > +-------------+------------+--------------+-------------+ > | employee_id | first_name | revrese_name | repeat_name | > +-------------+------------+--------------+-------------+ > | 1 | Sheri | irehS | SheriSheri | > | 2 | Derrick | kcirreD | DerrickDerrick | > | 4 | Michael | leahciM | MichaelMichael | > | 5 | Maya | ayaM | MayaMaya | > | 6 | Roberta | atreboR | RobertaRoberta | > | 7 | Rebecca | accebeR | RebeccaRebecca | > | 8 | Kim | miK | KimKim | > | 9 | Brenda | adnerB | BrendaBrenda | > | 10 | Darren | nerraD | DarrenDarren | > | 11 | Jonathan | nahtanoJ | JonathanJonatha | > +-------------+------------+--------------+-------------+ > > > SELECT employee_id, first_name as first_name, ascii(first_name) as ascii_ch, > chr(65) as char_65 FROM cp.`employee.json` limit 10; > +-------------+------------+------------+------------+ > | employee_id | first_name | ascii_ch | char_65 | > +-------------+------------+------------+------------+ > | 1 | Sheri | 83 | A | > | 2 | Derrick | 68 | A | > | 4 | Michael | 77 | A | > | 5 | Maya | 77 | A | > | 6 | Roberta | 82 | A | > | 7 | Rebecca | 82 | A | > | 8 | Kim | 75 | A | > | 9 | Brenda | 66 | A | > | 10 | Darren | 68 | A | > | 11 | Jonathan | 74 | A | > +-------------+------------+------------+------------+ > > > Thanks, > > Yash Sharma > >
