|
|
DescriptionDateSplitTransform example
Patch Set 1 #
Total comments: 6
Patch Set 2 : Removed multi-value date transform, modified logging #
Total comments: 4
Patch Set 3 : modified logging #MessagesTotal messages: 13
I am not sure what are exact requirements here but I assume this is originated from customer support case or query. https://codereview.appspot.com/217200043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/examples/DateSplitTransform.java (right): https://codereview.appspot.com/217200043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/examples/DateSplitTransform.java:84: Integer.toString(cal.get(Calendar.YEAR))); In case of multi-value field or by calling transform on more than one metedata field (via transform pipeline), code will overwrite previous value for year, date and month fields. What if metadata already has year, date or month field? What is the expected behavior?
Sign in to reply to this message.
https://codereview.appspot.com/217200043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/examples/DateSplitTransform.java (right): https://codereview.appspot.com/217200043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/examples/DateSplitTransform.java:84: Integer.toString(cal.get(Calendar.YEAR))); Yes, this came from a customer request for better filtering options in the SRP UI based on date. They're using the FS connector, so there would be only one date field. I tried to make it a little more useful in other cases but if you have multiple dates for a doc already in a single field this probably wouldn't be a good idea to split them. I could add some checks here, or just some comments. Since this really just an example I would expect people to customize it a bit as needed.
Sign in to reply to this message.
2 comments. LGTM. Please wait for Tanmay to also LGTM. Thank you very much! https://codereview.appspot.com/217200043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/examples/DateSplitTransform.java (right): https://codereview.appspot.com/217200043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/examples/DateSplitTransform.java:40: throw new NullPointerException(); throw wth the log message. and remove the log line. https://codereview.appspot.com/217200043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/examples/DateSplitTransform.java:47: log.log(Level.INFO, "using default dateFormat: {0}", dateFormat); s/INFO/CONFIG/
Sign in to reply to this message.
Removed multi-value date transform, modified logging
Sign in to reply to this message.
Changes made - I removed the multi-value logic since I don't think it makes sense for most date fields. Let me know if it's good to go Tanmay, thanks! https://codereview.appspot.com/217200043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/examples/DateSplitTransform.java (right): https://codereview.appspot.com/217200043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/examples/DateSplitTransform.java:40: throw new NullPointerException(); On 2015/03/24 18:16:34, pjo wrote: > throw wth the log message. > and remove the log line. Done. https://codereview.appspot.com/217200043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/examples/DateSplitTransform.java:47: log.log(Level.INFO, "using default dateFormat: {0}", dateFormat); On 2015/03/24 18:16:34, pjo wrote: > s/INFO/CONFIG/ Done.
Sign in to reply to this message.
Still LGTM. Please wait for Tanmay to also give LGTM. Thank you, PJ https://codereview.appspot.com/217200043/diff/20001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/examples/DateSplitTransform.java (right): https://codereview.appspot.com/217200043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/examples/DateSplitTransform.java:41: log.log(Level.INFO, "Using dateField: {0}", fieldname); this one should prolly be CONFIG also.
Sign in to reply to this message.
On 2015/03/25 22:43:31, pjo wrote: > Still LGTM. Please wait for Tanmay to also give LGTM. > > Thank you, PJ > > https://codereview.appspot.com/217200043/diff/20001/src/com/google/enterprise... > File src/com/google/enterprise/adaptor/examples/DateSplitTransform.java (right): > > https://codereview.appspot.com/217200043/diff/20001/src/com/google/enterprise... > src/com/google/enterprise/adaptor/examples/DateSplitTransform.java:41: > log.log(Level.INFO, "Using dateField: {0}", fieldname); > this one should prolly be CONFIG also. LGTM
Sign in to reply to this message.
I missed my comments earlier but still LGTM https://codereview.appspot.com/217200043/diff/20001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/examples/DateSplitTransform.java (right): https://codereview.appspot.com/217200043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/examples/DateSplitTransform.java:90: log.log(Level.FINEST, "Unable to parse date: {0}", dateField); Should we log the exception along with dateValue? logging dateValue here will be more helpful for debugging. I will prefer to check Strings.isNullOrEmpty(dateValue) above and log the exception along with the value at least at FINE level and indicating that not adding value for year, date and month. I know we are considering datetime field and we don't expect date field as multi-value field. So below comment might not be that important. Now we are just using first value from date field. We will not populate additional metadata if first value is invalid, even if second or third value might be valid date.
Sign in to reply to this message.
modified logging
Sign in to reply to this message.
OK, uploaded the latest revisions. Not sure there's a good one-size-fits-all solution for multivalue, so I think it's safe to assume single-value date fields https://codereview.appspot.com/217200043/diff/20001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/examples/DateSplitTransform.java (right): https://codereview.appspot.com/217200043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/examples/DateSplitTransform.java:41: log.log(Level.INFO, "Using dateField: {0}", fieldname); On 2015/03/25 22:43:31, pjo wrote: > this one should prolly be CONFIG also. Acknowledged. https://codereview.appspot.com/217200043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/examples/DateSplitTransform.java:90: log.log(Level.FINEST, "Unable to parse date: {0}", dateField); On 2015/03/26 16:34:23, Tanmay Vartak wrote: > Should we log the exception along with dateValue? logging dateValue here will be > more helpful for debugging. > > I will prefer to check Strings.isNullOrEmpty(dateValue) above and log the > exception along with the value at least at FINE level and indicating that not > adding value for year, date and month. > > I know we are considering datetime field and we don't expect date field as > multi-value field. So below comment might not be that important. > > Now we are just using first value from date field. We will not populate > additional metadata if first value is invalid, even if second or third value > might be valid date. Done.
Sign in to reply to this message.
Thank you. You have the LGTMs. Push it to github. Cheers
Sign in to reply to this message.
please close at https://codereview.appspot.com/ thank you
Sign in to reply to this message.
|