Jackie-Jiang commented on pull request #7435: URL: https://github.com/apache/pinot/pull/7435#issuecomment-920464640
> The way I see it, this feature is buggy: > > * it converted floats to ints > * it doesn't produce evenly sized ranges > * it doesn't handle or test rangeIds of -1, which are ambiguous and can mean smaller than the smallest range _or_ larger than the largest range, and `RangeIndexReader.getDocIds` never knew the difference. > > If this feature is in use, these bugs are probably acceptable to users. What I'm trying to do here is move some code around so I can eventually replace the feature, I don't want to fix every problem with the existing implementation in the process of doing so. I don't agree that we should leave the bug as is just because no one complains. It is hard for user to realize the result returned is inaccurate. It is okay to not fix it in the refactoring PR, but we should at least add a TODO and create an issue to track it so that it can be fixed in a following PR. Also, it shouldn't be hard to fix: - Convert float to int - already fixed in this PR - Not even sized range - not optimal but won't cause wrong result - RangeId of -1 - We can change the upper bound id to Integer.MAX_VALUE to differentiate them and handle them 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. To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org 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
