npawar commented on issue #4798: Decouple Key from Record URL: https://github.com/apache/incubator-pinot/pull/4798#issuecomment-550578171 > LGTM > > So all the specific implementation can extend BaseTable and go from there. > > One thing that remains to be seen is if it is a good idea to pollute TableResizer with overloaded methods to work for both Map<Key,Record> (for group by) or Set (for distinct) or should we subclass it (to borrow maximum resizing functionality) or write another version of it. > > Since resizer will be used only by group by and distinct code, probably adding multiple methods that take different data structures should be fine. I can refactor that as part of my pr that adds order by support for distinct -- #4790 Adding a `resizeRecordsSet` to the `TableResizer` sounds fine to me as a starting point. We can revisit that when we begin to have other operations needing resize (selection), or if adding `resizeRecordsSet` looks very messy. One thing that will be good to do in your follow-up PR - rename the `IndexedTable` to say `MapBasedTable` (or AggregationGroupByTable), and the new one you add can be `SetBasedTable`. Another point as noted by Jackie - the constructor for `BaseIndexedTable` takes in things which won't be relevant to all its implementations. It will be good to clean that up as well, when we have another implementation.
---------------------------------------------------------------- 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] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
