jadami10 commented on code in PR #12789:
URL: https://github.com/apache/pinot/pull/12789#discussion_r1552654653


##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeFieldSpec.java:
##########
@@ -74,6 +75,10 @@ public DateTimeFieldSpec(String name, DataType dataType, 
String format, String g
       @Nullable Object sampleValue) {
     super(name, dataType, true);
 
+    // Override format to be "TIMESTAMP" for TIMESTAMP data type because the 
format is implicit
+    if (dataType == DataType.TIMESTAMP) {

Review Comment:
   will this change people's schemas as they're stored in ZK, or just modify 
them every time they're loaded in ZK?
   
   I almost wonder if we should go the route of not letting create/update 
happen with this rather than silently changing it so folks are aware



##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/TimeSegmentPruner.java:
##########
@@ -18,6 +18,7 @@
  */
 package org.apache.pinot.broker.routing.segmentpruner;
 
+import com.google.common.base.Preconditions;

Review Comment:
   this whole file is a refactor. you're getting the format from the 
`DateTimeFieldSpec` rather then unnecessarily passing it. but nothing is 
actually changing here.



-- 
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.

To unsubscribe, e-mail: [email protected]

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


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to