----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19685/#review39000 -----------------------------------------------------------
I also verified and tested your latest patch with Derby and MySQL. Also, I tried to execute various alter commands with full qualified identifiers and simple identifiers in a real cluster. All features work well. Also, the patch looks pretty nice. I found two missed ones. I'm sorry for not mentioning them lately. It would be perfect if they are resolved. If you cannot afford complete them, I also can add some diff to your latest patch. Feel free to tell me your thoughts on this. The first one is the implementation of HCatalogStore. If you enable the maven profile 'hcatalog-0.12.0', you can see some errors HCatalogStore which is a subclass of CatalogStore. It also needs the implementation like AbstractDBStore, but using HiveMeta client. The second one is the problem of renaming managed tables. I leaved an in-line comment on GlobalEngine. Best regards, Hyunsik Choi tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/GlobalEngine.java <https://reviews.apache.org/r/19685/#comment71396> There are two table types: external table and non-external table. We call non-external table "managed table". An external table must have a table path. In contrast, a managed table does not have path. Instead, an automatically generated path in WAREHOUSE directory is assigned to a managed table. The path's name has to be equivalent to the table name. For example, assume that there are the 'MART1' database and the table 'BRANCH1'. The directory location of the table 'BRANCH1' will be ${WAREHOUSE_DIR}/MART1/BRANCH1. Please take a look at Tajo's system directory hierarchy at https://cwiki.apache.org/confluence/display/TAJO/Tajo+Internal. The main reason why I explain it is that we need to rename (or move) the managed table's directory when the managed table is renamed. You can distinguish if one table is *managed* or *external* by checking isExternal() method of TableDesc. I think that here is the best place for it. If my explanation is insufficient, feel free to ask me. - Hyunsik Choi On March 29, 2014, 1 p.m., Alvin Henrick wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19685/ > ----------------------------------------------------------- > > (Updated March 29, 2014, 1 p.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/ColumnDefinition.java > PRE-CREATION > tajo-algebra/src/main/java/org/apache/tajo/algebra/CreateTable.java 42dcc68 > 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 > 9078e60 > > 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 06ffcfd > > 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-catalog/tajo-catalog-server/src/test/java/org/apache/tajo/catalog/TestCatalog.java > 36c6b6b > > 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/HiveQLAnalyzer.java > afd0ce9 > > 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 > >
