liyafan82 commented on a change in pull request #2356:
URL: https://github.com/apache/calcite/pull/2356#discussion_r584580953



##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeImpl.java
##########
@@ -50,6 +50,12 @@
  */
 public abstract class RelDataTypeImpl
     implements RelDataType, RelDataTypeFamily {
+
+  /**
+   * Suffix for the digests of non-nullable types.
+   */
+  public static final String NON_NULLABLE_TYPE_DIGEST_SUFFIX = " NOT NULL";
+

Review comment:
       > Is there any special reason that the variable 
`NON_NULLABLE_TYPE_DIGEST_SUFFIX` belongs to this class and can we give it a 
shorter name ?
   
   Sure. I have refactored the code to give it a shorter name.
   
   I think the reason to place it in this class is that, it defines the common 
suffix for all Calcite internal types, and this class is the common base class 
for all Calcite internal types. 
   
   One alternative that I can think of is to put it into the `RelDataType` 
interface, and annotate it as an internal API.
   However, I personally prefer to place it in class `RelDataTypeImpl`, as in 
this PR.
   
   WDYT?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to