You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@tajo.apache.org by hyunsik <gi...@git.apache.org> on 2016/05/14 01:07:34 UTC
[GitHub] tajo pull request: TAJO-2154: Refactor Datum to use new Type imple...
GitHub user hyunsik opened a pull request:
https://github.com/apache/tajo/pull/1017
TAJO-2154: Refactor Datum to use new Type implementation.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/hyunsik/tajo TAJO-2154
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tajo/pull/1017.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 #1017
----
commit 8d2bf407f5e462216eb84d193c300b46dc2817f9
Author: Hyunsik Choi <hy...@apache.org>
Date: 2016-05-13T05:29:43Z
TAJO-2129: Apply new type implementation to Schema and Catalog.
Closes #1016
commit 2077b867885d533c5cb9940f58252664b33c14fc
Author: Hyunsik Choi <hy...@apache.org>
Date: 2016-05-14T01:06:43Z
TAJO-2154: Refactor Datum to use new Type implementation.
----
---
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: TAJO-2154: Refactor Datum to use new Type imple...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on the pull request:
https://github.com/apache/tajo/pull/1017#issuecomment-220729545
thank you for your review. I fixed them.
---
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: TAJO-2154: Refactor Datum to use new Type imple...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/1017#issuecomment-220501762
Hi @hyunsik, thank you for your patch. It looks good to me.
I left a comment, but it is not directly related to the changed made in this patch.
Also, I believe you are going to do remaining works to support complex data types. So, if you want to consider my comment in another issue, please do that.
---
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: TAJO-2154: Refactor Datum to use new Type imple...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/1017#discussion_r63990411
--- Diff: tajo-common/src/main/java/org/apache/tajo/type/Type.java ---
@@ -36,6 +36,7 @@
// No paramter types
public static final Any Any = new Any();
+ public static final Bit Bit = new Bit();
--- End diff --
Sorry for the confusion. I'm talking about the modifier of the constructor of the classes which are used in only singleton pattern. I left another comment.
---
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: TAJO-2154: Refactor Datum to use new Type imple...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on a diff in the pull request:
https://github.com/apache/tajo/pull/1017#discussion_r63986923
--- Diff: tajo-common/src/main/java/org/apache/tajo/type/Type.java ---
@@ -36,6 +36,7 @@
// No paramter types
public static final Any Any = new Any();
+ public static final Bit Bit = new Bit();
--- End diff --
IMO, the main difference between singleton with (typical) ``getInstance`` method and ```static final``` variable is the initialization time. Singleton pattern is preferred if the variable needs to be initialized lazily. Static variable is initialized at class loading. In this case, they are always used throughout a system. So, IMO, static variable is also right choice here. In addition, static variable provides more concise usages.
---
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: TAJO-2154: Refactor Datum to use new Type imple...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/1017#discussion_r63982956
--- Diff: tajo-common/src/main/java/org/apache/tajo/type/Type.java ---
@@ -36,6 +36,7 @@
// No paramter types
public static final Any Any = new Any();
+ public static final Bit Bit = new Bit();
--- End diff --
How about making the modifier of the constructors of these singleton instances to be ```default```?
This is not the changes made in this patch, but it seems better.
---
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: TAJO-2154: Refactor Datum to use new Type imple...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on the pull request:
https://github.com/apache/tajo/pull/1017#issuecomment-220191548
rebased. Thank you.
---
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: TAJO-2154: Refactor Datum to use new Type imple...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on the pull request:
https://github.com/apache/tajo/pull/1017#issuecomment-220824035
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: TAJO-2154: Refactor Datum to use new Type imple...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/1017#discussion_r63990409
--- Diff: tajo-common/src/main/java/org/apache/tajo/type/Bit.java ---
@@ -0,0 +1,27 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.tajo.type;
+
+import static org.apache.tajo.common.TajoDataTypes.Type.BIT;
+
+public class Bit extends Type {
+ public Bit() {
--- End diff --
I mean, this constructor is used only in Type. Its modifier doesn't have to be ```public```, and it will also reduce contributors' mistakes.
---
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: TAJO-2154: Refactor Datum to use new Type imple...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/1017#discussion_r64004676
--- Diff: tajo-common/src/main/java/org/apache/tajo/type/Bit.java ---
@@ -0,0 +1,27 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.tajo.type;
+
+import static org.apache.tajo.common.TajoDataTypes.Type.BIT;
+
+public class Bit extends Type {
+ public Bit() {
--- End diff --
Thank you for your understanding. When you update your patch, I'll finish my review immediately.
---
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: TAJO-2154: Refactor Datum to use new Type imple...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/1017#issuecomment-219960269
Would you rebase please?
---
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: TAJO-2154: Refactor Datum to use new Type imple...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/1017#issuecomment-220828340
+1 LGTM! Ship it!
---
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: TAJO-2154: Refactor Datum to use new Type imple...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tajo/pull/1017
---
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: TAJO-2154: Refactor Datum to use new Type imple...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on a diff in the pull request:
https://github.com/apache/tajo/pull/1017#discussion_r64004326
--- Diff: tajo-common/src/main/java/org/apache/tajo/type/Bit.java ---
@@ -0,0 +1,27 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.tajo.type;
+
+import static org.apache.tajo.common.TajoDataTypes.Type.BIT;
+
+public class Bit extends Type {
+ public Bit() {
--- End diff --
I got your point. I misunderstood your comment. I'll change them. Thank you!
---
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: TAJO-2154: Refactor Datum to use new Type imple...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on the pull request:
https://github.com/apache/tajo/pull/1017#issuecomment-220865812
Thank you very much. I'll commit it soon.
---
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.
---