TAJO-2165: Add 'ALTER TABLE UNSET PROPERTY' statement to Tajo DDL #1036

dongjinleekr · 2016-07-05T15:09:28Z

No description provided.

…leType#UNSET_PROPERTY, AlterTableDescProto#property_keys 추가.

…bstractDBStore#alterTable에 위 메소드를 호출하는 기능 추가. 3. TestCatalog#testAlterTableName -> TestCatalog#testAlterTable 4. TestCatalog#createMockAlterTableUnsetProperty 헬퍼 메소드 추가. 5. TestCatalog#testAlterTable에 AbstractDBStore#unsetProperties 테스트 추가.

…rotobuf message 기능 구현

…talogUtil#unsetProperty를 호출해 주는 기능 추가.

…erty 및 기능 테스트 추가

jihoonson · 2016-07-05T23:45:46Z

Thank you for your patch. I'll review soon. Would you please check the test failure?

…된 property를 unset하도록 수정

dongjinleekr · 2016-07-06T04:08:59Z

Updated! Thanks for your immediate response.

Followings are additional comments on this issue:

During updating the (potentially) broken test, one question arose: is there any list of properties that should not be unset? If there are, it seems like we have to implement an additional feature to make them unset-proof. (for example, show error message when the user tries to unset some reserved properties.) Do you have any opinion on that? Or, is it better to query about this problem on dev mailing list?
I omitted implementation for HiveCatalogStore - as you remember, it is a deprecated feature. (TAJO-1775)

… 존재성만을 검증하도록 수정. (instance별로 달라질 수 있는 값)

jihoonson · 2016-07-06T05:23:26Z

Thank you for the quick update. Here are my answers.

Basically, table properties are used for optional settings per table. So, every table property is optional, and tajo should be able to work with default property values if they are absent.
However, among table properties, timezone is recently changed as a mandatory property of a table. So, currently I think it is the only one which should not be removed.
The best solution seems to store the timezone value in the TableMeta or TableDesc instead of in the optional KeyValue set. But this would be better to be done in a separate ticket, and it would be enough to force timezone as unsettable here.
The deprecated one is HCatalogStore, and HiveCatalogStore should be implemented. But, as you see, many functions of HiveCatalogStore are not implemented yet. I think it also would be better to do in a separate ticket.

dongjinleekr · 2016-07-06T06:42:43Z

Oh, my mistake. I confused HCatalogStore with HiveCatalogStore.

Let's sum up. The following is the list of tickets that should be opened. Some of them are from this discussion, and the others are what I found during this work. (I think it is better to implement disabling unset timezone feature on separate issue, not here.)

[Major] Move timezone property from KeyValue set and disable to unset it. (Related: TAJO-1185, TAJO-1241)
[Major, Umbrella] Implement missing HiveCatalogStore features.
[Trivial] Remove AlterTableNode#clone - it has been commented out.

If you have any opinion, don't hesitate to comment to me. If you merge and close this request without any comments, I will regard it you accepted my proposal. Thanks again for your guidance! :)

jihoonson · 2016-07-06T10:28:33Z

tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/CatalogUtil.java

@@ -552,6 +552,16 @@ public static AlterTableDesc setProperty(String tableName, KeyValueSet params, A
    return alterTableDesc;
  }

+  public static AlterTableDesc unsetProperty(String tableName, String[] propertyKeys, AlterTableType alterTableType) {


alterTableType must be UNSET_PROPERTY, so the last parameter looks unnecessary.

I know. But I did it for the consistency with the other AlterTable methods: CatalogUtil#renameColumn, renameTable, addNewColumn and setProperty. How about clear this issue with AlterTableNode#clone? These methods worth to be refined.

Right. All those methods need to be refined. But, IMO, they are all internal APIs, and don't have to be consistent. So, I think that the new method has a better signature.

jihoonson · 2016-07-06T10:42:33Z

Hi @dongjinleekr, I looked over your patch. It looks good to me in overall. I left a single trivial comment.

jinossy · 2016-07-07T04:22:53Z

tajo-core-tests/src/test/java/org/apache/tajo/engine/query/TestAlterTable.java

+    tableMeta = tableDesc.getMeta();
+    assertEquals(tableMeta.getPropertySet().size(), 2);
+    assertNotNull(tableMeta.getProperty("timezone"));
+    assertNotNull(tableMeta.getProperty("text.null"));


You should verify the unset property.

remove final parameter. see: apache#1036 (comment)

Add non-existence check for unsetted property. see: apache#1036 (comment)

see: apache#1036 (comment)

dongjinleekr · 2016-07-07T15:54:29Z

@jihoonson @jinossy Thanks for your comments. I applied all of them. (You can see the link to the comment in the log message of each commit.) Thanks for reviewing!

dongjinleekr · 2016-07-07T15:56:39Z

As soon as you accept this pull request, I will request another one - the fix for the glitches discussed in this issue. I already completed that work. :)

jihoonson · 2016-07-08T14:32:21Z

tajo-algebra/src/main/java/org/apache/tajo/algebra/AlterTable.java

@@ -43,6 +44,8 @@
  private AlterTableOpType alterTableOpType;
  @Expose @SerializedName("TableProperties")
  private Map<String, String> params;
+  @Expose @SerializedName("TablePropertyKeys")
+  private List<String> propertyKeys;


It would be better to change this variable to be more intuitive as well. I think getter and setter also need to be changed together.

jihoonson · 2016-07-08T14:34:51Z

@dongjinleekr thank you for the quick update! I left my last comment.
Thanks!

…rty_keys

jihoonson · 2016-07-11T13:05:08Z

+1. The latest patch looks good to me.
@dongjinleekr thank you for your contribution!

jinossy · 2016-07-11T13:39:26Z

+1
@dongjinleekr Thanks for your contribution!!

dongjinleekr added 20 commits July 4, 2016 21:28

Implement TestAlterTable#testAlterTableUnSetProperty

4ef3041

1. AlterTableType#UNSET_PROPERTY 정의. 2. CatalogProtos.proto에 AlterTab…

e566766

…leType#UNSET_PROPERTY, AlterTableDescProto#property_keys 추가.

HiveCatalogStore#alterTable에 UNSET_PROPERTY 경우 추가

625e220

AlterTableDesc에 UNSET_PROPERTY 기능 추가

960d6ad

tajo-algebra에 alter table unset statement 구현

c70805a

Lexer, Parser에 UNSET 예약어 및 문법 정의

4b691f5

SQLAnalyzer 확장

02e4229

Plan message (Plan.proto) 확장: alter table unset을 표현할 수 있도록 수정.

ea9207a

LogicalNode 자료구조 AlterTableNode 확장

89f4530

LogicalNodeSerializer, LogicalNodeDeserializer 확장: Logical Node <-> P…

b73a78e

…rotobuf message 기능 구현

LogicalPlanner에 UNSET PROPERTY 파싱 기능 추가

0f60724

Plan.proto의 AlterTableNode#propertyKeys 수정

ac7f351

CatalogUtil#unsetProperty 추가. DDLExecutor에 alter table unset문을 받아서 Ca…

3be4bfc

…talogUtil#unsetProperty를 호출해 주는 기능 추가.

commit b73a78e에 누락되어 있던 부분 추가

749b285

SQLAnalyzer 오류 수정

e1cd8ee

commit 3be4bfc에 누락된 내용 추가

f540352

SQLAnalyzer#evaluateAlterTableOperationTye 오류 수정

9181f2f

commit 0f60724에 누락된 부분 추가

6d2a28b

TestAlterTable#testAlterTableUnSetProperty -> testAlterTableUnsetProp…

c312352

…erty 및 기능 테스트 추가

TestAlterTable#testAlterTableUnsetProperty 업데이트: 디폴트 값이 존재하지 않는 + 미설정…

c0a6020

…된 property를 unset하도록 수정

TestAlterTable#testAlterTableUnsetProperty 업데이트: timezone, text.null의…

0675579

… 존재성만을 검증하도록 수정. (instance별로 달라질 수 있는 값)

jihoonson reviewed Jul 6, 2016
View reviewed changes

jinossy reviewed Jul 7, 2016
View reviewed changes

dongjinleekr added 3 commits July 7, 2016 23:58

Updated CatalogUtil#unsetProperty signiture

2838169

remove final parameter. see: apache#1036 (comment)

Update TestAlterTable#testAlterTableUnsetProperty

082313b

Add non-existence check for unsetted property. see: apache#1036 (comment)

AlterTableDesc#propertyKeys -> unsetPropertyKeys

56ffb2f

see: apache#1036 (comment)

jihoonson reviewed Jul 8, 2016
View reviewed changes

dongjinleekr added 4 commits July 10, 2016 09:32

AlterTable#propertyKeys -> unsetPropertyKeys

8fbeab8

Plan.proto: AlterTableNode#propertyKeys -> unsetPropertyKeys

f1c61b4

CatalogProtos.proto: AlterTableDescProto#property_keys -> unset_prope…

fb4d598

…rty_keys

AlterTableNode#propertyKeys-> unsetPropertyKeys

11f85b7

asfgit closed this in fab36f9 Jul 11, 2016

dongjinleekr deleted the feature/TAJO-2165 branch July 11, 2016 20:32

dongjinleekr mentioned this pull request Aug 13, 2016

TAJO-2170: Disable unsetting timezone property #1045

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TAJO-2165: Add 'ALTER TABLE UNSET PROPERTY' statement to Tajo DDL #1036

TAJO-2165: Add 'ALTER TABLE UNSET PROPERTY' statement to Tajo DDL #1036

dongjinleekr commented Jul 5, 2016

jihoonson commented Jul 5, 2016

dongjinleekr commented Jul 6, 2016

jihoonson commented Jul 6, 2016

dongjinleekr commented Jul 6, 2016 •

edited

Loading

jihoonson Jul 6, 2016

dongjinleekr Jul 6, 2016

jihoonson Jul 7, 2016

jihoonson commented Jul 6, 2016

jinossy Jul 7, 2016

dongjinleekr commented Jul 7, 2016 •

edited

Loading

dongjinleekr commented Jul 7, 2016

jihoonson Jul 8, 2016

jihoonson commented Jul 8, 2016

jihoonson commented Jul 11, 2016

jinossy commented Jul 11, 2016

TAJO-2165: Add 'ALTER TABLE UNSET PROPERTY' statement to Tajo DDL #1036

TAJO-2165: Add 'ALTER TABLE UNSET PROPERTY' statement to Tajo DDL #1036

Conversation

dongjinleekr commented Jul 5, 2016

jihoonson commented Jul 5, 2016

dongjinleekr commented Jul 6, 2016

jihoonson commented Jul 6, 2016

dongjinleekr commented Jul 6, 2016 • edited Loading

jihoonson Jul 6, 2016

Choose a reason for hiding this comment

dongjinleekr Jul 6, 2016

Choose a reason for hiding this comment

jihoonson Jul 7, 2016

Choose a reason for hiding this comment

jihoonson commented Jul 6, 2016

jinossy Jul 7, 2016

Choose a reason for hiding this comment

dongjinleekr commented Jul 7, 2016 • edited Loading

dongjinleekr commented Jul 7, 2016

jihoonson Jul 8, 2016

Choose a reason for hiding this comment

jihoonson commented Jul 8, 2016

jihoonson commented Jul 11, 2016

jinossy commented Jul 11, 2016

dongjinleekr commented Jul 6, 2016 •

edited

Loading

dongjinleekr commented Jul 7, 2016 •

edited

Loading