You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@tajo.apache.org by dongjinleekr <gi...@git.apache.org> on 2016/08/13 10:40:16 UTC
[GitHub] tajo pull request #1045: TAJO-2165: Disable unsetting timezone property
GitHub user dongjinleekr opened a pull request:
https://github.com/apache/tajo/pull/1045
TAJO-2165: Disable unsetting timezone property
Hello. I just completed [TAJO-2170, Disable unsetting timezone property](https://issues.apache.org/jira/browse/TAJO-2170). However, It would be good to explain my decisions made on this issue. In short, it became a little bit different from our initial expectation.
A little bit different from [your comment in TAJO-2165](https://github.com/apache/tajo/pull/1036#issuecomment-230673810), which became the starting point of this feature, I didn't move the timezone property from KeyValueSet to TableMeta or TableDesc. Here is why:
- Our goal is to disable the removal of the timezone property from the catalog. So, it is sufficient to keep this property from removal by raising Error when the user attempts to remove it from the catalog storage; However, KeyValueSet, TableMeta and TableDesc are all components in alter table message, not storage. So, they don't need to be modified.
- If we modify KeyValueSet, TableMeta or TableDesc, we have to reconduct similar task every time some new unremovable properties are added. This repetitive task does not worth.
With above reasons, I decided not to modify KeyValueSet, TableMeta or TableDesc.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dongjinleekr/tajo feature/TAJO-2170
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tajo/pull/1045.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1045
----
commit b66ce500134637cba052addb80586e9ca5d2be26
Author: Lee Dongjin <do...@apache.org>
Date: 2016-08-13T09:07:48Z
Define new error code: UNREMOVABLE_TABLE_PROPERTY (1007)
commit cd1f2bf098c3a8eef6adb399a99b6a6e5b73961b
Author: Lee Dongjin <do...@apache.org>
Date: 2016-08-13T09:17:23Z
Define UnremovableTablePropertyException
commit 3cfd8875cc7b48cef87b94e491c337a27c58dbc3
Author: Lee Dongjin <do...@apache.org>
Date: 2016-08-13T09:57:40Z
Disable unsetting timezone property
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo issue #1045: TAJO-2165: Disable unsetting timezone property
Posted by dongjinleekr <gi...@git.apache.org>.
Github user dongjinleekr commented on the issue:
https://github.com/apache/tajo/pull/1045
Add to this, I have another question: is there any reason you separated CreateTableExecutor from DDLExecutor? For all DDL operations except 'Create Table' are executed in DDLExecutor, which invokes RPC operations via CatalogService object. However, 'Create Table' operation is executed in an indirect way, via CreateTableExecutor. I thought there must be some design intention or development history I don't know. If it's no inconvenience to you, I would like to know.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo issue #1045: TAJO-2170: Disable unsetting timezone property
Posted by dongjinleekr <gi...@git.apache.org>.
Github user dongjinleekr commented on the issue:
https://github.com/apache/tajo/pull/1045
Fixed!! :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request #1045: TAJO-2170: Disable unsetting timezone property
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tajo/pull/1045
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request #1045: TAJO-2170: Disable unsetting timezone property
Posted by jinossy <gi...@git.apache.org>.
Github user jinossy commented on a diff in the pull request:
https://github.com/apache/tajo/pull/1045#discussion_r76527387
--- Diff: tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java ---
@@ -1110,12 +1113,20 @@ private void setProperties(final int tableId, final KeyValueSetProto properties)
}
}
- private void unsetProperties(final int tableId, final PrimitiveProtos.StringListProto propertyKeys) {
+ private void unsetProperties(final int tableId, final PrimitiveProtos.StringListProto propertyKeys)
+ throws UnremovableTablePropertyException {
final String deleteSql = "DELETE FROM " + TB_OPTIONS + " WHERE TID=? AND KEY_=?";
Connection conn;
PreparedStatement pstmt = null;
+ Set<String> keys = Sets.newHashSet(propertyKeys.getValuesList());
--- End diff --
Please fix indentation in this code block
https://cwiki.apache.org/confluence/display/TAJO/Code+Style
Thanks
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request #1045: TAJO-2170: Disable unsetting timezone property
Posted by dongjinleekr <gi...@git.apache.org>.
Github user dongjinleekr commented on a diff in the pull request:
https://github.com/apache/tajo/pull/1045#discussion_r76527498
--- Diff: tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java ---
@@ -1110,12 +1113,20 @@ private void setProperties(final int tableId, final KeyValueSetProto properties)
}
}
- private void unsetProperties(final int tableId, final PrimitiveProtos.StringListProto propertyKeys) {
+ private void unsetProperties(final int tableId, final PrimitiveProtos.StringListProto propertyKeys)
+ throws UnremovableTablePropertyException {
final String deleteSql = "DELETE FROM " + TB_OPTIONS + " WHERE TID=? AND KEY_=?";
Connection conn;
PreparedStatement pstmt = null;
+ Set<String> keys = Sets.newHashSet(propertyKeys.getValuesList());
--- End diff --
Fixed!! :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo issue #1045: TAJO-2170: Disable unsetting timezone property
Posted by jinossy <gi...@git.apache.org>.
Github user jinossy commented on the issue:
https://github.com/apache/tajo/pull/1045
+1 LGTM!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---