> On March 30, 2014, 6:24 a.m., Hyunsik Choi wrote: > > 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
Not a problem.I will resolve the issues you mentioned. Since hcatalog profile is not enabled by default so I did not notice HCatalogStore :)... I understand the managed table concept.It is almost like managed vs external table in Hive. Do I see a potential defect here in createTable function in GlobalEngine ??? It is not taking database name in consideration when creating the Path object.I have pasted the code below. Path tablePath = new Path(sm.getWarehouseDir(), createTable.getTableName()); Let me know if I am right or wrong because this will cause an issue when renaming the directory. Thank you for reviewing. - Alvin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19685/#review39000 ----------------------------------------------------------- On March 29, 2014, 4 a.m., Alvin Henrick wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19685/ > ----------------------------------------------------------- > > (Updated March 29, 2014, 4 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/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 > >
