> 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
>
> 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.
>
>
> Hyunsik Choi wrote:
> 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
As per my understanding the physical directory is created in
'${warehouse_dir}/${table_name}' when we create table.
When I do alter table name I was planning to call catalog.getTable before
updating the new tableName in the database and this getTable call will return
me the path as you mentioned '${warehouse_dir}/{database_name}/${table_name}'
as per the current code, but the physical directory is created in this location
'${warehouse_dir}/${table_name}'. The rename (or move) will fail. The existing
path returned is not the actual physical directory on HDFS.
move/rename '${warehouse_dir}/{database_name}/${table_name}' to
'${warehouse_dir}/{database_name}/newTableName'...WILL FAIL
move/rename '${warehouse_dir}/${table_name}' to
'${warehouse_dir}/{database_name}/newTableName' will work...WILL WORK
Till now it has worked because we have been I believe only inserting / deleting
record in the table.
Is Drop Table with Purge option working for managed Tables ?
I don't think so it is working properly although I could be wrong...It is
working against MemStore because Path in MemStore is
'${warehouse_dir}/${table_name}' i.e. (The same object is returned which was
created and set in memory when invoked create table.) but when you run the test
against MySQL or Derby it will fail because a new Path object is created based
on the Meta Data and that will return at
'${warehouse_dir}/{database_name}/${table_name}.
I Apologize for any inconvenience and I may be wrong but just trying to get to
the bottom of the issue to implement the rename / move command correctly.
I have created drop table test case for managed table.Please test it against
the AbstractDBStore.
TestCatalog skips the portion of executing GlobalEngine so not able to verify
by myself. Appreciate your help.
Thanks!
Warm Regards,
Alvin.
- 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
>
>
