[GitHub] [solr] stillalex commented on pull request #1950: SOLR-16994: facilitate date-related FieldValueFeature extensions (docValues=True only)

["stillalex (via GitHub)" <gi...@apache.org>]

stillalex commented on PR #1950:
URL: https://github.com/apache/solr/pull/1950#issuecomment-1739858015

   just a comment from the peanut gallery (will leave the review for the experts).
   
   this method `interprete_NumericDocValues_NumberType_DATE` feels very specific, but at the same time it doesn't have to be. on the surface it looks like you want to post-process the value. the default return from `readNumericDocValues ` is already the `docValues.longValue();`, so you don't really need the type check.
   so just a minor suggestion: why not add a generic wrapper method (name is bad I know)
   ```
   protected long postProcess(long val) {
     return val;
   }
   ```
   
   and just wrap any default return value in it
   ```
   // just take the long value
   return postProcess(docValues.longValue());
   ```
   
   this way it's easy to override if needed.


-- 
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: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org