----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18376/#review37181 -----------------------------------------------------------
Looks good overall. Few more minor comments in addition to the previous ones. One generic comment is to test out these functions using SQL as well, just to make sure the Optiq to Drill expression transformations is being done correctly. common/src/main/java/org/apache/drill/common/expression/OutputTypeDeterminer.java <https://reviews.apache.org/r/18376/#comment68597> You can reset this file since it doesn't have any changes. exec/java-exec/src/main/codegen/data/MathFunc.tdd <https://reviews.apache.org/r/18376/#comment68599> The format of this tdd file is very similar to MathFunctionTypes.tdd. Do you think it makes sense to consolidate all the definitions in a single tdd file? It might be easier to keep track of what has been implemented exec/java-exec/src/main/codegen/data/MathFunc.tdd <https://reviews.apache.org/r/18376/#comment68598> Divide is already included in MathFunctionTypes.tdd, you can get rid of it. exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctionUtil.java <https://reviews.apache.org/r/18376/#comment68778> I think 0x128 is incorrect? Should be in decimal. exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctionUtil.java <https://reviews.apache.org/r/18376/#comment68779> Same as above should be in decimal exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctionUtil.java <https://reviews.apache.org/r/18376/#comment68780> Is there a reason we are inspecting every byte? For every character we know the length by examining the first byte, so can't we skip over the remaining bytes? exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java <https://reviews.apache.org/r/18376/#comment68793> You aren't using the workspace variable, can you get rid of it? Could you remove it from the other functions that don't use it as well? exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestStringFunctions.java <https://reviews.apache.org/r/18376/#comment68794> Do you think its possible to add a couple of tests with non-ascii characters so we can hit the multi byte code path? - Mehant Baid On March 11, 2014, 11:59 p.m., Jinfeng Ni wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18376/ > ----------------------------------------------------------- > > (Updated March 11, 2014, 11:59 p.m.) > > > Review request for drill and Mehant Baid. > > > Repository: drill-git > > > Description > ------- > > > We add the support of some commonly used String functions and Math functions > (over int, floating-point number). > > 1. like > 2. similar > 3. regexp_replace > 4. char_length > 4. oct_length > 5. bit_length > 6. position > 7. strops > 8. lower > 9. upper > 10. initcap > 11. substring/substr > 12. left > 13. right > 14. replace > 15.lpad > 16 rpad > 17. ltrim > 18. rtrim > 19. concat. > > Math : abs, ceil, floor, sqrt, sign, trunc. > > > Diffs > ----- > > > common/src/main/java/org/apache/drill/common/expression/OutputTypeDeterminer.java > 69acf12 > > common/src/main/java/org/apache/drill/common/expression/fn/MathFunctions.java > ee3a099 > exec/java-exec/src/main/codegen/config.fmpp 8f1060a > exec/java-exec/src/main/codegen/data/MathFunc.tdd PRE-CREATION > exec/java-exec/src/main/codegen/templates/MathFunctions.java PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/CharSubstring.java > f991a41 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/MathFunctions.java > f4e2060 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/RegexpUtil.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctionUtil.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java > PRE-CREATION > > exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSimpleFunctions.java > f6a8096 > > exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestStringFunctions.java > PRE-CREATION > exec/java-exec/src/test/resources/functions/string/testCharLength.json > PRE-CREATION > exec/java-exec/src/test/resources/functions/string/testConcat.json > PRE-CREATION > exec/java-exec/src/test/resources/functions/string/testLeft.json > PRE-CREATION > exec/java-exec/src/test/resources/functions/string/testLike.json > PRE-CREATION > exec/java-exec/src/test/resources/functions/string/testLower.json > PRE-CREATION > exec/java-exec/src/test/resources/functions/string/testLpad.json > PRE-CREATION > exec/java-exec/src/test/resources/functions/string/testLtrim.json > PRE-CREATION > exec/java-exec/src/test/resources/functions/string/testPosition.json > PRE-CREATION > exec/java-exec/src/test/resources/functions/string/testRegexpReplace.json > PRE-CREATION > exec/java-exec/src/test/resources/functions/string/testReplace.json > PRE-CREATION > exec/java-exec/src/test/resources/functions/string/testRight.json > PRE-CREATION > exec/java-exec/src/test/resources/functions/string/testRpad.json > PRE-CREATION > exec/java-exec/src/test/resources/functions/string/testRtrim.json > PRE-CREATION > exec/java-exec/src/test/resources/functions/string/testSimilar.json > PRE-CREATION > exec/java-exec/src/test/resources/functions/string/testSubstr.json > PRE-CREATION > exec/java-exec/src/test/resources/functions/string/testUpper.json > PRE-CREATION > > Diff: https://reviews.apache.org/r/18376/diff/ > > > Testing > ------- > > A JUnit test case is added, to test the string functions. > > > Thanks, > > Jinfeng Ni > >
