----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18843/#review36513 -----------------------------------------------------------
It's nice work. This patch adds a really nice feature to Tajo. Besides the inline comments, I have one suggestion. It would be great to rename the config 'tajo.geoip.data' into 'tajo.functions.geoip.data-location'? I think that the second name part 'geoip' is too higher than its importance in Tajo system. tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/CountryInet4.java <https://reviews.apache.org/r/18843/#comment67505> Recently, functions were moved into some specific directories, such as math, datetime, and string. It would be great if you move these functions into some proper package like geoip. tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/CountryInet4.java <https://reviews.apache.org/r/18843/#comment67506> I like the function you proposed. But, this function name 'country' is too general for me. It would be nice if these functions have some prefix like 'geoip_'. In addition, 'country' function results in country code. So, I would like to suggest the name 'geoip_country_code'. It looks very intuitive for me. tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/InCountryInet4.java <https://reviews.apache.org/r/18843/#comment67507> I would like to suggest the name 'geoip_in_country' due to the same reason of geoip_country_code. - Hyunsik Choi On March 7, 2014, 12:09 a.m., Jihoon Son wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18843/ > ----------------------------------------------------------- > > (Updated March 7, 2014, 12:09 a.m.) > > > Review request for Tajo. > > > Repository: tajo > > > Description > ------- > > See the title. SQLParser cannot parse the INET4 type. > I fixed the problem of parser and some missing codes for processing of INET4 > type. > I also added two functions of country() and in_country() for INET4 type. > Lastly, I added the TestNetTypes class to verify the query processing of > INET4 type. > > > Diffs > ----- > > > tajo-core/tajo-core-backend/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4 > db04d4b > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/Country.java > 9e28b55 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/CountryInet4.java > PRE-CREATION > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/CountryText.java > PRE-CREATION > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/InCountry.java > 1cac624 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/InCountryInet4.java > PRE-CREATION > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/function/InCountryText.java > PRE-CREATION > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/parser/SQLAnalyzer.java > 29e4d43 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/ExprsVerifier.java > 358cabd > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/RangePartitionAlgorithm.java > 5bff857 > > tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/query/TestNetTypes.java > PRE-CREATION > > tajo-core/tajo-core-backend/src/test/resources/dataset/TestNetTypes/table1.tbl > PRE-CREATION > > tajo-core/tajo-core-backend/src/test/resources/dataset/TestNetTypes/table2.tbl > PRE-CREATION > > tajo-core/tajo-core-backend/src/test/resources/queries/TestNetTypes/table1_ddl.sql > PRE-CREATION > > tajo-core/tajo-core-backend/src/test/resources/queries/TestNetTypes/table2_ddl.sql > PRE-CREATION > > tajo-core/tajo-core-backend/src/test/resources/queries/TestNetTypes/testGroupby.sql > PRE-CREATION > > tajo-core/tajo-core-backend/src/test/resources/queries/TestNetTypes/testGroupby2.sql > PRE-CREATION > > tajo-core/tajo-core-backend/src/test/resources/queries/TestNetTypes/testJoin.sql > PRE-CREATION > > tajo-core/tajo-core-backend/src/test/resources/queries/TestNetTypes/testSelect.sql > PRE-CREATION > > tajo-core/tajo-core-backend/src/test/resources/queries/TestNetTypes/testSort.sql > PRE-CREATION > > tajo-core/tajo-core-backend/src/test/resources/results/TestNetTypes/testGroupby.result > PRE-CREATION > > tajo-core/tajo-core-backend/src/test/resources/results/TestNetTypes/testGroupby2.result > PRE-CREATION > > tajo-core/tajo-core-backend/src/test/resources/results/TestNetTypes/testJoin.result > PRE-CREATION > > tajo-core/tajo-core-backend/src/test/resources/results/TestNetTypes/testSelect.result > PRE-CREATION > > tajo-core/tajo-core-backend/src/test/resources/results/TestNetTypes/testSort.result > PRE-CREATION > > Diff: https://reviews.apache.org/r/18843/diff/ > > > Testing > ------- > > mvn verify > > > Thanks, > > Jihoon Son > >
