----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20643/#review41440 -----------------------------------------------------------
Thank you for your contribution. I leave some comments. They are usually performance issues. There is another suggestion. The suggestion is to use NumberFormat which is a builtin java class. NumberFormat provides locales and various format string. Even though its format string is different from PostgreSQL or Oracle, you can easily replace the format string of Java NumberFormat with PostgreSQL ones by using String replace methods. tajo-core/src/main/java/org/apache/tajo/engine/function/dataformat/ToCharDataFormat.java <https://reviews.apache.org/r/20643/#comment74871> You should use ArrayList instead of Vector because Vector is a synchronized class. So, it causes unnecessary synchronization cost. tajo-core/src/main/java/org/apache/tajo/engine/function/dataformat/ToCharDataFormat.java <https://reviews.apache.org/r/20643/#comment74877> You should use ArrayList instead of Vector because Vector is a synchronized class. So, it causes unnecessary synchronization cost. tajo-core/src/main/java/org/apache/tajo/engine/function/dataformat/ToCharDataFormat.java <https://reviews.apache.org/r/20643/#comment74872> This code parses double represented as string multiple times. The parsing cost is very expensive. You should avoid it. tajo-core/src/main/java/org/apache/tajo/engine/function/dataformat/ToCharDataFormat.java <https://reviews.apache.org/r/20643/#comment74873> You should open and close curly braces in if-statement. This is our coding convention. tajo-core/src/main/java/org/apache/tajo/engine/function/dataformat/ToCharDataFormat.java <https://reviews.apache.org/r/20643/#comment74874> You should open and close curly braces in if-statement. This is our coding convention. tajo-core/src/main/java/org/apache/tajo/engine/function/dataformat/ToCharDataFormat.java <https://reviews.apache.org/r/20643/#comment74875> You should open and close curly braces in if-statement. This is our coding convention. tajo-core/src/main/java/org/apache/tajo/engine/function/dataformat/ToCharDataFormat.java <https://reviews.apache.org/r/20643/#comment74876> You should open and close curly braces in if-statement. This is our coding convention. - Hyunsik Choi On April 24, 2014, 10:34 a.m., Seungun Choe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20643/ > ----------------------------------------------------------- > > (Updated April 24, 2014, 10:34 a.m.) > > > Review request for Tajo. > > > Bugs: TAJO-637 > https://issues.apache.org/jira/browse/TAJO-637 > > > Repository: tajo > > > Description > ------- > > Currently, Tajo only supports to_char(timestamp, text) function. > However, PostgreSQL supports to_char() for timestamp, interval, int, double, > and numeric types > Tajo needs to support more data types in to_char() as shown in > http://www.postgresql.org/docs/9.3/static/functions-formatting.html > > > Diffs > ----- > > > tajo-core/src/main/java/org/apache/tajo/engine/function/dataformat/ToCharDataFormat.java > PRE-CREATION > > tajo-core/src/test/java/org/apache/tajo/engine/function/TestDataFormatFunctions.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/20643/diff/ > > > Testing > ------- > > mvn clean install is OK. > > > Thanks, > > Seungun Choe > >
