jon-wei commented on pull request #10267: URL: https://github.com/apache/druid/pull/10267#issuecomment-674289603
> Hmm, thinking about this some more, it may be best to not have it be an error. The risk of doing a sanity-check error is that something that worked before will stop working once you upgrade. It seems to me that any time someone had been specifying dimensions and metrics manually, but not including all input fields, they probably didn't do that intentionally. So this should be a bug fix that they appreciate, even though it's a behavioral change. That sounds fine to me too, I don't really have a strong preference on that. > In new mode, I suggest we skip the check, because that will enable the full power of timestampSpec to be used (you could use it to switch a secondary timestamp to primary, for example). > I'd also consider adding a logged warning if the timestampSpec is anything other than __time + auto / millis, just to let people know that if they didn't intend to be in a special case, they should change their timestampSpec. Ah, I was thinking that the auto/millis check in the new mode would only apply if the column being referenced was `__time`, so that users could still use alternate timestamp columns. The warning log sounds fine to me. > Separately — as a reviewer — would you prefer these changes to be made in this patch, or in a follow-up? I'm OK either way… Let's do it in this patch, the additions I'm guessing won't be too large and we can have everything related in one place. ---------------------------------------------------------------- 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]
