TAJO-2154: Refactor Datum to use new Type implementation. #1017
Conversation
|
Would you rebase please? |
…into TAJO-2154
|
rebased. Thank you. |
| @@ -36,6 +36,7 @@ | |||
|
|
|||
| // No paramter types | |||
| public static final Any Any = new Any(); | |||
| public static final Bit Bit = new Bit(); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Hi @hyunsik, thank you for your patch. It looks good to me. |
| import static org.apache.tajo.common.TajoDataTypes.Type.BIT; | ||
|
|
||
| public class Bit extends Type { | ||
| public Bit() { |
There was a problem hiding this comment.
I mean, this constructor is used only in Type. Its modifier doesn't have to be public, and it will also reduce contributors' mistakes.
There was a problem hiding this comment.
I got your point. I misunderstood your comment. I'll change them. Thank you!
There was a problem hiding this comment.
Thank you for your understanding. When you update your patch, I'll finish my review immediately.
|
thank you for your review. I fixed them. |
|
fixed. |
|
+1 LGTM! Ship it! |
|
Thank you very much. I'll commit it soon. |
No description provided.