You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@tajo.apache.org by Alvin Henrick <sh...@aol.com> on 2014/03/26 15:29:55 UTC

Review Request 19685: [TAJO-480] Umbrella Jira for adding ALTER TABLE statement

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19685/
-----------------------------------------------------------

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.


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 7122af8 
  tajo-catalog/tajo-catalog-client/src/main/java/org/apache/tajo/catalog/AbstractCatalogClient.java 1932db5 
  tajo-catalog/tajo-catalog-client/src/main/java/org/apache/tajo/catalog/CatalogService.java 494ac0b 
  tajo-catalog/tajo-catalog-client/src/main/proto/CatalogProtocol.proto 6a96b4e 
  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 2a6727e 
  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 35171cc 
  tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/CatalogServer.java 621b475 
  tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java ef485ef 
  tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/CatalogStore.java db15c02 
  tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java 1fef286 
  tajo-core/tajo-core-backend/src/main/antlr4/org/apache/tajo/engine/parser/SQLLexer.g4 1ceaca8 
  tajo-core/tajo-core-backend/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4 71c16f2 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/parser/SQLAnalyzer.java af7c3e9 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/AlgebraVisitor.java acb9008 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/BaseAlgebraVisitor.java cf36539 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/BasicLogicalPlanVisitor.java 94e2482 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/LogicalPlanPreprocessor.java b08109d 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/LogicalPlanVisitor.java bf41996 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/LogicalPlanner.java 01bacc0 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/PlannerUtil.java a928fb5 
  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 acaf85b 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/GlobalEngine.java d7ded93 
  tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/QueryTestCaseBase.java ddb9952 
  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


Re: Review Request 19685: [TAJO-480] Umbrella Jira for adding ALTER TABLE statement

Posted by Hyunsik Choi <hy...@apache.org>.

> On March 31, 2014, 2:13 p.m., Hyunsik Choi wrote:
> > Hi Alvin,
> > 
> > Your work is already awesome! I think that your confusion is caused by some existing buggy code. If you are Ok, I'd like to touch some parts for fixing the bug and I'll upload the diff that I changed.
> > 
> > Best regards,
> > Hyunsik Choi
> 
> Alvin Henrick wrote:
>     Thanks a lot Hyunsik ...Please go ahead and make the change :)...That is the only one thing left so we can close it and move on to another exciting things.

Thank you for your understanding. As I mentioned before, I just uploaded some trivial change in Jira (https://issues.apache.org/jira/browse/TAJO-480).

I tested your latest patch with my trivial change as follows:

mvn clean test -Phcatalog-0.12.0 -Dtajo.catalog.store.class=org.apache.tajo.catalog.store.HCatalogStore
mvn clean test 

All unit tests are passed in both tests, and all features including the table rename in a real cluster work well.

According to my review, your patch looks pretty nice. I don't have no more comments. After you review my trivial patch, I'll finish the review and commit your work.

Best Regards,
Hyunsik Choi


- Hyunsik


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19685/#review39027
-----------------------------------------------------------


On March 31, 2014, 12:44 p.m., Alvin Henrick wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19685/
> -----------------------------------------------------------
> 
> (Updated March 31, 2014, 12:44 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-drivers/tajo-hcatalog/src/main/java/org/apache/tajo/catalog/store/HCatalogStore.java 6ef255f 
>   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 15fe6c0 
>   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/java/org/apache/tajo/engine/query/TestDropTable.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/dataset/TestDropTable/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 
>   tajo-core/tajo-core-backend/src/test/resources/queries/TestDropTable/drop_table_ddl.sql PRE-CREATION 
>   tajo-core/tajo-core-backend/src/test/resources/queries/TestDropTable/table1_ddl.sql PRE-CREATION 
>   tajo-docs/src/main/sphinx/functions/datetime_func_and_operators.rst 51156fa 
> 
> Diff: https://reviews.apache.org/r/19685/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alvin Henrick
> 
>


Re: Review Request 19685: [TAJO-480] Umbrella Jira for adding ALTER TABLE statement

Posted by Alvin Henrick <sh...@aol.com>.

> On March 31, 2014, 5:13 a.m., Hyunsik Choi wrote:
> > Hi Alvin,
> > 
> > Your work is already awesome! I think that your confusion is caused by some existing buggy code. If you are Ok, I'd like to touch some parts for fixing the bug and I'll upload the diff that I changed.
> > 
> > Best regards,
> > Hyunsik Choi

Thanks a lot Hyunsik ...Please go ahead and make the change :)...That is the only one thing left so we can close it and move on to another exciting things.


- Alvin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19685/#review39027
-----------------------------------------------------------


On March 31, 2014, 3:44 a.m., Alvin Henrick wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19685/
> -----------------------------------------------------------
> 
> (Updated March 31, 2014, 3:44 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-drivers/tajo-hcatalog/src/main/java/org/apache/tajo/catalog/store/HCatalogStore.java 6ef255f 
>   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 15fe6c0 
>   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/java/org/apache/tajo/engine/query/TestDropTable.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/dataset/TestDropTable/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 
>   tajo-core/tajo-core-backend/src/test/resources/queries/TestDropTable/drop_table_ddl.sql PRE-CREATION 
>   tajo-core/tajo-core-backend/src/test/resources/queries/TestDropTable/table1_ddl.sql PRE-CREATION 
>   tajo-docs/src/main/sphinx/functions/datetime_func_and_operators.rst 51156fa 
> 
> Diff: https://reviews.apache.org/r/19685/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alvin Henrick
> 
>


Re: Review Request 19685: [TAJO-480] Umbrella Jira for adding ALTER TABLE statement

Posted by Hyunsik Choi <hy...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19685/#review39027
-----------------------------------------------------------


Hi Alvin,

Your work is already awesome! I think that your confusion is caused by some existing buggy code. If you are Ok, I'd like to touch some parts for fixing the bug and I'll upload the diff that I changed.

Best regards,
Hyunsik Choi

- Hyunsik Choi


On March 31, 2014, 12:44 p.m., Alvin Henrick wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19685/
> -----------------------------------------------------------
> 
> (Updated March 31, 2014, 12:44 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-drivers/tajo-hcatalog/src/main/java/org/apache/tajo/catalog/store/HCatalogStore.java 6ef255f 
>   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 15fe6c0 
>   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/java/org/apache/tajo/engine/query/TestDropTable.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/dataset/TestDropTable/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 
>   tajo-core/tajo-core-backend/src/test/resources/queries/TestDropTable/drop_table_ddl.sql PRE-CREATION 
>   tajo-core/tajo-core-backend/src/test/resources/queries/TestDropTable/table1_ddl.sql PRE-CREATION 
>   tajo-docs/src/main/sphinx/functions/datetime_func_and_operators.rst 51156fa 
> 
> Diff: https://reviews.apache.org/r/19685/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alvin Henrick
> 
>


Re: Review Request 19685: [TAJO-480] Umbrella Jira for adding ALTER TABLE statement

Posted by Alvin Henrick <sh...@aol.com>.

> On March 31, 2014, 2:45 p.m., Alvin Henrick wrote:
> > Ship It!

Thanks! Hyunsik.I have uploaded the new merged patch in JIRA.


- Alvin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19685/#review39042
-----------------------------------------------------------


On March 31, 2014, 3:44 a.m., Alvin Henrick wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19685/
> -----------------------------------------------------------
> 
> (Updated March 31, 2014, 3:44 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-drivers/tajo-hcatalog/src/main/java/org/apache/tajo/catalog/store/HCatalogStore.java 6ef255f 
>   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 15fe6c0 
>   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/java/org/apache/tajo/engine/query/TestDropTable.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/dataset/TestDropTable/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 
>   tajo-core/tajo-core-backend/src/test/resources/queries/TestDropTable/drop_table_ddl.sql PRE-CREATION 
>   tajo-core/tajo-core-backend/src/test/resources/queries/TestDropTable/table1_ddl.sql PRE-CREATION 
>   tajo-docs/src/main/sphinx/functions/datetime_func_and_operators.rst 51156fa 
> 
> Diff: https://reviews.apache.org/r/19685/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alvin Henrick
> 
>


Re: Review Request 19685: [TAJO-480] Umbrella Jira for adding ALTER TABLE statement

Posted by Alvin Henrick <sh...@aol.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19685/#review39042
-----------------------------------------------------------

Ship it!


Ship It!

- Alvin Henrick


On March 31, 2014, 3:44 a.m., Alvin Henrick wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19685/
> -----------------------------------------------------------
> 
> (Updated March 31, 2014, 3:44 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-drivers/tajo-hcatalog/src/main/java/org/apache/tajo/catalog/store/HCatalogStore.java 6ef255f 
>   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 15fe6c0 
>   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/java/org/apache/tajo/engine/query/TestDropTable.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/dataset/TestDropTable/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 
>   tajo-core/tajo-core-backend/src/test/resources/queries/TestDropTable/drop_table_ddl.sql PRE-CREATION 
>   tajo-core/tajo-core-backend/src/test/resources/queries/TestDropTable/table1_ddl.sql PRE-CREATION 
>   tajo-docs/src/main/sphinx/functions/datetime_func_and_operators.rst 51156fa 
> 
> Diff: https://reviews.apache.org/r/19685/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alvin Henrick
> 
>


Re: Review Request 19685: [TAJO-480] Umbrella Jira for adding ALTER TABLE statement

Posted by Alvin Henrick <sh...@aol.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19685/
-----------------------------------------------------------

(Updated March 31, 2014, 3:44 a.m.)


Review request for Tajo and Hyunsik Choi.


Changes
-------

1) HCatalogStore Implemented.

2) Renaming managed tables.Not implemented.Reasoning provided below.

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 have mentioned '${warehouse_dir}/{database_name}/${table_name}' as per the current code, 
but the physical directory is created at this location '${warehouse_dir}/${table_name}' when creating table.
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.


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 (updated)
-----

  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-drivers/tajo-hcatalog/src/main/java/org/apache/tajo/catalog/store/HCatalogStore.java 6ef255f 
  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 15fe6c0 
  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/java/org/apache/tajo/engine/query/TestDropTable.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/dataset/TestDropTable/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 
  tajo-core/tajo-core-backend/src/test/resources/queries/TestDropTable/drop_table_ddl.sql PRE-CREATION 
  tajo-core/tajo-core-backend/src/test/resources/queries/TestDropTable/table1_ddl.sql PRE-CREATION 
  tajo-docs/src/main/sphinx/functions/datetime_func_and_operators.rst 51156fa 

Diff: https://reviews.apache.org/r/19685/diff/


Testing
-------


Thanks,

Alvin Henrick


Re: Review Request 19685: [TAJO-480] Umbrella Jira for adding ALTER TABLE statement

Posted by Alvin Henrick <sh...@aol.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19685/
-----------------------------------------------------------

(Updated March 30, 2014, 7:41 p.m.)


Review request for Tajo and Hyunsik Choi.


Changes
-------

Added TestDropTable to verify the Path Issue.


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 (updated)
-----

  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 15fe6c0 
  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/java/org/apache/tajo/engine/query/TestDropTable.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/dataset/TestDropTable/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 
  tajo-core/tajo-core-backend/src/test/resources/queries/TestDropTable/drop_table_ddl.sql PRE-CREATION 
  tajo-core/tajo-core-backend/src/test/resources/queries/TestDropTable/table1_ddl.sql PRE-CREATION 
  tajo-docs/src/main/sphinx/functions/datetime_func_and_operators.rst 51156fa 

Diff: https://reviews.apache.org/r/19685/diff/


Testing
-------


Thanks,

Alvin Henrick


Re: Review Request 19685: [TAJO-480] Umbrella Jira for adding ALTER TABLE statement

Posted by Alvin Henrick <sh...@aol.com>.

> On March 30, 2014, 6:24 a.m., Hyunsik Choi wrote:
> > tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/GlobalEngine.java, line 306
> > <https://reviews.apache.org/r/19685/diff/5/?file=540341#file540341line306>
> >
> >     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.

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. 


- 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
> 
>


Re: Review Request 19685: [TAJO-480] Umbrella Jira for adding ALTER TABLE statement

Posted by Alvin Henrick <sh...@aol.com>.

> 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
> 
>


Re: Review Request 19685: [TAJO-480] Umbrella Jira for adding ALTER TABLE statement

Posted by Hyunsik Choi <hy...@apache.org>.

> 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
> 
>


Re: Review Request 19685: [TAJO-480] Umbrella Jira for adding ALTER TABLE statement

Posted by Alvin Henrick <sh...@aol.com>.

> 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
> 
>


Re: Review Request 19685: [TAJO-480] Umbrella Jira for adding ALTER TABLE statement

Posted by Hyunsik Choi <hy...@apache.org>.
-----------------------------------------------------------
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
> 
>


Re: Review Request 19685: [TAJO-480] Umbrella Jira for adding ALTER TABLE statement

Posted by Alvin Henrick <sh...@aol.com>.
-----------------------------------------------------------
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.


Changes
-------

Done!!!. End to End solution implemented.Ready for review.
AbstractDBStore implemented.Added new unit test in TestCatalog to verify various drivers.


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 (updated)
-----

  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


Re: Review Request 19685: [TAJO-480] Umbrella Jira for adding ALTER TABLE statement

Posted by Hyunsik Choi <hy...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19685/#review38890
-----------------------------------------------------------


The latest patch looks nice for me. You are right. I missed the implementation of AbstractDBStore and HCatalogStore. Think you for reminding me :)

As you concerned, we cannot use ALTER TABLE in practice if they are not implemented. The below link would be helpful for you. With some JVM options, you can perform unit tests in TestCatalog with Derby, or MySQL. Also, you can verify all integration tests with HCatalogStore. 

https://cwiki.apache.org/confluence/display/TAJO/Unit+Tests

If you complete the implementation of AbstractDBStore, I'll also test your patch in various CatalogStores like Derby, MySQL, and Hive meta.

Best Regards,
Hyunsik Choi

- Hyunsik Choi


On March 28, 2014, 6:10 a.m., Alvin Henrick wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19685/
> -----------------------------------------------------------
> 
> (Updated March 28, 2014, 6:10 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 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/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
> 
>


Re: Review Request 19685: [TAJO-480] Umbrella Jira for adding ALTER TABLE statement

Posted by Alvin Henrick <sh...@aol.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19685/
-----------------------------------------------------------

(Updated March 27, 2014, 9:10 p.m.)


Review request for Tajo and Hyunsik Choi.


Changes
-------

Thanks for reviewing...The @Override were removed in auto formatting in intellij..I have fixed the settings :).

I have incorporated all the changes you have mentioned..They all sounded good and valid points.

The only thing missing is AbstractDBStore implementation...I am throwing unimplemented exception.

That should be the easiest to implement I think :)..I will continue working on that..

I will upload the patch to JIRA as well...
Do you think so we can commit this ? I will take the latest version and will only have to worry about AbstractDBStore implementation only.


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 (updated)
-----

  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 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/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


Re: Review Request 19685: [TAJO-480] Umbrella Jira for adding ALTER TABLE statement

Posted by Hyunsik Choi <hy...@apache.org>.
-----------------------------------------------------------
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
> 
>


Re: Review Request 19685: [TAJO-480] Umbrella Jira for adding ALTER TABLE statement

Posted by Alvin Henrick <sh...@aol.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19685/
-----------------------------------------------------------

(Updated March 26, 2014, 11:04 p.m.)


Review request for Tajo and Hyunsik Choi.


Changes
-------

Please review...Taking Database into consideration this time....Had to merge the changes...At  last did it. :)

Still the work is in progress.

Thanks! Guys 


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 (updated)
-----

  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


Re: Review Request 19685: [TAJO-480] Umbrella Jira for adding ALTER TABLE statement

Posted by Alvin Henrick <sh...@aol.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19685/
-----------------------------------------------------------

(Updated March 26, 2014, 5:17 p.m.)


Review request for Tajo and Hyunsik Choi.


Bugs: TAJO-480
    https://issues.apache.org/jira/browse/TAJO-480


Repository: tajo


Description (updated)
-------

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 7122af8 
  tajo-catalog/tajo-catalog-client/src/main/java/org/apache/tajo/catalog/AbstractCatalogClient.java 1932db5 
  tajo-catalog/tajo-catalog-client/src/main/java/org/apache/tajo/catalog/CatalogService.java 494ac0b 
  tajo-catalog/tajo-catalog-client/src/main/proto/CatalogProtocol.proto 6a96b4e 
  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 2a6727e 
  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 35171cc 
  tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/CatalogServer.java 621b475 
  tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java ef485ef 
  tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/CatalogStore.java db15c02 
  tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java 1fef286 
  tajo-core/tajo-core-backend/src/main/antlr4/org/apache/tajo/engine/parser/SQLLexer.g4 1ceaca8 
  tajo-core/tajo-core-backend/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4 71c16f2 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/parser/SQLAnalyzer.java af7c3e9 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/AlgebraVisitor.java acb9008 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/BaseAlgebraVisitor.java cf36539 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/BasicLogicalPlanVisitor.java 94e2482 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/LogicalPlanPreprocessor.java b08109d 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/LogicalPlanVisitor.java bf41996 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/LogicalPlanner.java 01bacc0 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/PlannerUtil.java a928fb5 
  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 acaf85b 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/GlobalEngine.java d7ded93 
  tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/QueryTestCaseBase.java ddb9952 
  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


Re: Review Request 19685: [TAJO-480] Umbrella Jira for adding ALTER TABLE statement

Posted by Alvin Henrick <sh...@aol.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19685/
-----------------------------------------------------------

(Updated March 26, 2014, 2:31 p.m.)


Review request for Tajo and Hyunsik Choi.


Changes
-------

uploading patch


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.


Diffs (updated)
-----

  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 7122af8 
  tajo-catalog/tajo-catalog-client/src/main/java/org/apache/tajo/catalog/AbstractCatalogClient.java 1932db5 
  tajo-catalog/tajo-catalog-client/src/main/java/org/apache/tajo/catalog/CatalogService.java 494ac0b 
  tajo-catalog/tajo-catalog-client/src/main/proto/CatalogProtocol.proto 6a96b4e 
  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 2a6727e 
  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 35171cc 
  tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/CatalogServer.java 621b475 
  tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java ef485ef 
  tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/CatalogStore.java db15c02 
  tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java 1fef286 
  tajo-core/tajo-core-backend/src/main/antlr4/org/apache/tajo/engine/parser/SQLLexer.g4 1ceaca8 
  tajo-core/tajo-core-backend/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4 71c16f2 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/parser/SQLAnalyzer.java af7c3e9 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/AlgebraVisitor.java acb9008 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/BaseAlgebraVisitor.java cf36539 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/BasicLogicalPlanVisitor.java 94e2482 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/LogicalPlanPreprocessor.java b08109d 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/LogicalPlanVisitor.java bf41996 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/LogicalPlanner.java 01bacc0 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/engine/planner/PlannerUtil.java a928fb5 
  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 acaf85b 
  tajo-core/tajo-core-backend/src/main/java/org/apache/tajo/master/GlobalEngine.java d7ded93 
  tajo-core/tajo-core-backend/src/test/java/org/apache/tajo/QueryTestCaseBase.java ddb9952 
  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