> On March 30, 2014, 3:24 p.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
>
> Alvin Henrick wrote:
> 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.
>
Yes, the concept of the managed table and external table is the same to Hive.
>From your question, I've found some bugs of the table directories. Actually, a
>managed table directory should be as follows:
${warehouse_dir} / {database_name} / ${table_name}
But, GlobalEngine::createTable(Session, CreateTableNode, boolean) uses a wrong
path '${warehouse_dir}/${table_name}'. Nevertheless, it has worked well because
the paths of managed tables are not stored in CatalogStore. The paths of
managed table is used as the concatenation of '${warehouse_dir} /
{database_name} / ${table_name}'.
In this issue, it would be Ok if you only use '${warehouse_dir} /
{database_name} / ${table_name}'. After this issue, I'll create another issue
for fixing the bug and improving database API design.
Best regards,
Hyunsik Choi
- Hyunsik
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19685/#review39000
-----------------------------------------------------------
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
>
>
