clintropolis commented on pull request #10635: URL: https://github.com/apache/druid/pull/10635#issuecomment-783123959
@FrankChen021 very sorry for the delayed response, I got a bit busy and haven't had a chance to get back to this PR until now. I think I agree that we shouldn't try to do a unified format function right now, I didn't mean to hoist the responsibility onto this PR (but am glad we had the discussion). I think the 3rd option of using the names for the functions @gianm suggested sgtm. I think we can get this merged after the conflicts fixed and naming adjustments made. I still think you could adjust the tests to just expect the correct response based on the value of `druid.generic.useDefaultValueForNull` since travis runs both ways, which would make `NullHandling.updateForTests` not necessary [(this comment)](https://github.com/apache/druid/pull/10635#discussion_r547515782), but thats more of a nitpick on my end, and am +1 either way. ---------------------------------------------------------------- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
