[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5706: DateTimeFieldSpec validation in schema validate

[GitBox <gi...@apache.org>]

Jackie-Jiang commented on a change in pull request #5706:
URL: https://github.com/apache/incubator-pinot/pull/5706#discussion_r454770490



##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeGranularitySpec.java
##########
@@ -87,33 +85,43 @@ public TimeUnit getTimeUnit() {
    * <ul>
    * <li>Convert a granularity to millis.
    * This method should not do validation of outputGranularity.
-   * The validation should be handled by caller using {@link #isValidGranularity(String)}</li>
+   * The validation should be handled by caller using {@link #isValidGranularity}</li>
    * <ul>
    * <li>1) granularityToMillis(1:HOURS) = 3600000 (60*60*1000)</li>
    * <li>2) granularityToMillis(1:MILLISECONDS) = 1</li>
    * <li>3) granularityToMillis(15:MINUTES) = 900000 (15*60*1000)</li>
    * </ul>
    * </ul>
-   * @return
    */
   public Long granularityToMillis() {
     return TimeUnit.MILLISECONDS.convert(_size, _timeUnit);
   }
 
   /**
    * Check correctness of granularity of {@link DateTimeFieldSpec}
-   * @param granularity
-   * @return
    */
-  public static boolean isValidGranularity(String granularity) {
-    Preconditions.checkNotNull(granularity);
+  public static boolean isValidGranularity(String granularity, Logger ctxLogger) {

Review comment:
       Recommend using exception to pass the failure message instead of passing in a logger. The problem of using logger is that the caller cannot control the logging level. For user input, in most cases we don't want to log error which usually stands for severe problems.
   ```suggestion
     public static void validateGranularity(String granularity) {
   ```

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/SchemaUtils.java
##########
@@ -39,65 +46,99 @@
 
   private static final Logger LOGGER = LoggerFactory.getLogger(SchemaUtils.class);
 
-  /**
-   * Validates that for a field spec with transform function, the source column name and destination column name are exclusive
-   * i.e. do not allow using source column name for destination column
-   */
-  public static boolean validate(Schema schema) {

Review comment:
       Let's keep this method for simplicity




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org