----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20852/#review41720 -----------------------------------------------------------
The patch looks nice for me. You seem to go the right way. There are good utilities that you can reuse. So, I leaved some comments about them. Also, you explained the table schema in Jira, I'll leave additional comment about the schema soon. tajo-algebra/src/main/java/org/apache/tajo/algebra/ColumnValueType.java <https://reviews.apache.org/r/20852/#comment75437> You can reuse BinaryOperator instead of ColumnValueType. I think that ColumnValueType has a subset feature of BinaryOperator. You only need to restrict that the left side should be only column and right side should be an expression. Could you take a look at the below code sample in SQLParser.g4? You may also use ANTLR 'comparison_predicate' parse rule instead of 'partition_key_value' and 'drop_partition_key_value' in SQLParser.g4. If so do, you would easily implement the condition of DROP PARTITION. ``` public BinaryOperator visitComparison_predicate(SQLParser.Comparison_predicateContext ctx) { TerminalNode operator = (TerminalNode) ctx.comp_op().getChild(0); return new BinaryOperator(tokenToExprType(operator.getSymbol().getType()), visitRow_value_predicand(ctx.left), visitRow_value_predicand(ctx.right)); } ``` tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/Column.java <https://reviews.apache.org/r/20852/#comment75438> I don't think that it is not good way because Column class represents a logical column. If you want to some partition condition, you need to use another way. I'd like to suggest using EvalNode which is an expression tree consisting of arithmetic, logical operators and operands. tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/Schema.java <https://reviews.apache.org/r/20852/#comment75439> As I mentioned before, schema is only a schema representation. If you want to represent partition specified information, you need to use another one. tajo-core/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4 <https://reviews.apache.org/r/20852/#comment75440> comparison_predicate would be a good substitution. tajo-core/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4 <https://reviews.apache.org/r/20852/#comment75441> comparison_predicate would be a good substitution. Also, if you use comparison_predicate, you can easily transform an Expr into EvalNode. tajo-core/src/main/java/org/apache/tajo/master/GlobalEngine.java <https://reviews.apache.org/r/20852/#comment75442> As I mentioned, if you use comparison_predicate and EvalNode, this part would be simplified. tajo-project/pom.xml <https://reviews.apache.org/r/20852/#comment75300> Usually, bumping up version is good. But, in real cluster environments, some guys experienced some guava dependency conflict between Tajo's 15.0 and Hadoop's 11.0.2. So, we fixed it to 11.0.2. :) - Hyunsik Choi On April 30, 2014, 1:15 a.m., Alvin Henrick wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20852/ > ----------------------------------------------------------- > > (Updated April 30, 2014, 1:15 a.m.) > > > Review request for Tajo and Hyunsik Choi. > > > Bugs: TAJO-744 > https://issues.apache.org/jira/browse/TAJO-744 > > > Repository: tajo > > > Description > ------- > > TAJO-744 review request. The work is still in progress but wanted to get some > feedback. > > > Diffs > ----- > > tajo-algebra/src/main/java/org/apache/tajo/algebra/AlterTable.java 4bb0ed2 > tajo-algebra/src/main/java/org/apache/tajo/algebra/AlterTableOpType.java > 67b28a2 > > tajo-algebra/src/main/java/org/apache/tajo/algebra/ColumnValuePartition.java > PRE-CREATION > tajo-algebra/src/main/java/org/apache/tajo/algebra/ColumnValueType.java > PRE-CREATION > tajo-algebra/src/main/java/org/apache/tajo/algebra/CreateTable.java 63ca364 > > tajo-algebra/src/main/java/org/apache/tajo/algebra/PartitionMethodDescExpr.java > PRE-CREATION > tajo-algebra/src/main/java/org/apache/tajo/algebra/PartitionType.java > PRE-CREATION > > tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableDesc.java > fcaa4c3 > > tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableType.java > 0b7639c > > tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/CatalogUtil.java > 1dd33b2 > > tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/Column.java > aceb6f1 > > tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/Schema.java > b2dde3d > > tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/partition/PartitionMethodDesc.java > 48a105c > tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto 7f41596 > > tajo-catalog/tajo-catalog-drivers/tajo-hcatalog/src/main/java/org/apache/tajo/catalog/store/HCatalogStore.java > e6f6acc > > tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java > 234af19 > > tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java > ca99160 > > tajo-catalog/tajo-catalog-server/src/test/java/org/apache/tajo/catalog/TestCatalog.java > 32ea83b > tajo-core/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4 > 0076794 > tajo-core/src/main/java/org/apache/tajo/engine/parser/SQLAnalyzer.java > e7de8f6 > tajo-core/src/main/java/org/apache/tajo/engine/planner/LogicalPlanner.java > f2ddf13 > > tajo-core/src/main/java/org/apache/tajo/engine/planner/logical/AlterTableNode.java > 76a47d0 > tajo-core/src/main/java/org/apache/tajo/master/GlobalEngine.java 35b8ab8 > tajo-core/src/test/java/org/apache/tajo/engine/parser/TestSQLAnalyzer.java > 2010502 > tajo-core/src/test/java/org/apache/tajo/engine/query/TestAlterTable.java > 57d8b32 > tajo-core/src/test/java/org/apache/tajo/engine/util/TestTupleUtil.java > cecb281 > > tajo-core/src/test/resources/queries/TestAlterTable/alter_table_add_partition_ddl.sql > PRE-CREATION > > tajo-core/src/test/resources/queries/TestAlterTable/alter_table_drop_partition_ddl.sql > PRE-CREATION > tajo-project/pom.xml 30c721d > > Diff: https://reviews.apache.org/r/20852/diff/ > > > Testing > ------- > > > Thanks, > > Alvin Henrick > >
