Jackie-Jiang commented on pull request #5483: URL: https://github.com/apache/incubator-pinot/pull/5483#issuecomment-643508353
> In general this looks fine to me. I am just worried about the confusion that having so many classes with duplicate names will cause. @mayankshriv I thought about it and actually already tried with multiple names, and finally decide to go with the most simple and intuitive ones, although they are the same as the thrift ones for the following reasons: - The thrift and context classes will only co-exist in the converter, on the engine side only the context ones will be used; on the wiring side, only the thrift ones will be used. - I tried to append `Info` after each class, but we already have a class named `FunctionInfo`, and IMO it is easier to distinguish thrift class from context class comparing to another POJO class. - With the most simple and intuitive class name, it can improve the readability of the code, as long as the import is correct (IDE should be able to handle this properly) ---------------------------------------------------------------- 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: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
