----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19685/#review38733 -----------------------------------------------------------
Your patch looks excellent for me. Database support was committed only few days ago. You seem to done a large amount of work for short time. I think that there are few issues to be committed. I leaved some trivial comments. tajo-algebra/src/main/java/org/apache/tajo/algebra/AlterTable.java <https://reviews.apache.org/r/19685/#comment71008> As you already know, ColumnDefinition is duplicated to that of CreateTable.java. It would be nice if you extract ColumnDefinition class into one top-level class and share it. tajo-catalog/tajo-catalog-client/src/main/java/org/apache/tajo/catalog/AbstractCatalogClient.java <https://reviews.apache.org/r/19685/#comment71009> What was the reasoning behind the removal of the annotation @Override? In my opinion, using the annotation '@Override'gives various benefits. tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/CatalogServer.java <https://reviews.apache.org/r/19685/#comment71010> Same as above tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java <https://reviews.apache.org/r/19685/#comment71011> Same as above tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java <https://reviews.apache.org/r/19685/#comment71012> Same as above tajo-core/tajo-core-backend/src/main/antlr4/org/apache/tajo/engine/parser/SQLLexer.g4 <https://reviews.apache.org/r/19685/#comment71014> In my tests, they can be non-reserved keywords because they will not make ambiguous grammars. tajo-core/tajo-core-backend/src/main/antlr4/org/apache/tajo/engine/parser/SQLLexer.g4 <https://reviews.apache.org/r/19685/#comment71015> All non-reserved keywords should be nonreserved_keywords rule in SQLParser.g. Could you add them in nonreserved_keywords rule? tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/parser/SQLAnalyzer.java <https://reviews.apache.org/r/19685/#comment71016> I love this part. tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/query/TestAlterTable.java <https://reviews.apache.org/r/19685/#comment71018> It would be great if these tests check the column names of altered tables. - Hyunsik Choi On March 27, 2014, 8:04 a.m., Alvin Henrick wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19685/ > ----------------------------------------------------------- > > (Updated March 27, 2014, 8:04 a.m.) > > > Review request for Tajo and Hyunsik Choi. > > > Bugs: TAJO-480 > https://issues.apache.org/jira/browse/TAJO-480 > > > Repository: tajo > > > Description > ------- > > 1) Implemented Alter Table. > 2) Made changes to various component like algebra,parser,catalog,planner and > engine etc. > 3) Only implemented MemStore for review.Before I proceed any further felt > like it needs to be reviewed. > 4) Need to implement AbstractDBStore.WIP > 5) Need to improve exception handling and logging.WIP. > > Please feel free to advice comment. Appreciate your help in reviewing the > code. > > Thanks! > Warm Regards, > Alvin. > > > HOLD ON GUYS!!!! I SEE THE DATABASE concept was introduced and it is causing > issues when patch is applied.I will soon publish the new PATCH. > I APOLOGIZE for inconvenience. > > > Diffs > ----- > > tajo-algebra/src/main/java/org/apache/tajo/algebra/AlterTable.java > PRE-CREATION > tajo-algebra/src/main/java/org/apache/tajo/algebra/AlterTableOpType.java > PRE-CREATION > tajo-algebra/src/main/java/org/apache/tajo/algebra/OpType.java c4a007a > > tajo-catalog/tajo-catalog-client/src/main/java/org/apache/tajo/catalog/AbstractCatalogClient.java > 6fb385e > > tajo-catalog/tajo-catalog-client/src/main/java/org/apache/tajo/catalog/CatalogService.java > d69ed7e > tajo-catalog/tajo-catalog-client/src/main/proto/CatalogProtocol.proto > 06b69c1 > > tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableDesc.java > PRE-CREATION > > tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableType.java > PRE-CREATION > > tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/CatalogUtil.java > 92df86a > > tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/ColumnNameAlreadyExistException.java > PRE-CREATION > tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto 14fb39f > > tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/CatalogServer.java > a171fb4 > > tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java > d10f545 > > tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/CatalogStore.java > 3f8686d > > tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java > ef98ee2 > > tajo-core/tajo-core-backend/src/main/antlr4/org/apache/tajo/engine/parser/SQLLexer.g4 > 6eccd12 > > tajo-core/tajo-core-backend/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4 > c0edf09 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/parser/SQLAnalyzer.java > 30d4c2e > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/AlgebraVisitor.java > 84cdc08 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/BaseAlgebraVisitor.java > 62cee57 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/BasicLogicalPlanVisitor.java > 772e5fb > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/LogicalPlanPreprocessor.java > 1ac416f > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/LogicalPlanVisitor.java > 76454b9 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/LogicalPlanner.java > c479f1c > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/PlannerUtil.java > fbd65a9 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/SimpleAlgebraVisitor.java > dc7b7a2 > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/logical/AlterTableNode.java > PRE-CREATION > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/logical/NodeType.java > 2b453fb > > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/GlobalEngine.java > fe3caeb > > tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/QueryTestCaseBase.java > 65a0d47 > > tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/engine/query/TestAlterTable.java > PRE-CREATION > > tajo-core/tajo-core-backend/src/test/resources/dataset/TestAlterTable/table1.tbl > PRE-CREATION > > tajo-core/tajo-core-backend/src/test/resources/queries/TestAlterTable/alter_table_add_new_column_ddl.sql > PRE-CREATION > > tajo-core/tajo-core-backend/src/test/resources/queries/TestAlterTable/alter_table_rename_column_ddl.sql > PRE-CREATION > > tajo-core/tajo-core-backend/src/test/resources/queries/TestAlterTable/alter_table_rename_table_ddl.sql > PRE-CREATION > > tajo-core/tajo-core-backend/src/test/resources/queries/TestAlterTable/table1_ddl.sql > PRE-CREATION > > Diff: https://reviews.apache.org/r/19685/diff/ > > > Testing > ------- > > > Thanks, > > Alvin Henrick > >
