chenboat commented on pull request #5444:
URL: https://github.com/apache/incubator-pinot/pull/5444#issuecomment-635802473
> @chenboat There is no new added major classes. Most of the changes are
making the classes compatible with the filter interface change, so it is very
hard to break them into multiple PRs. I can make the removed interface in
BlockValIterator a separate PR as they are independent of the filtering.
> This PR is the first step of re-structuring the filtering in Pinot, so for
historical reason the name for some interfaces (e.g. BlockDocIdSet) might be
confusing. I wouldn't spend too much time renaming and documenting them because
they are going to be re-structured in the following steps.
Thanks for reducing the number of changed files. Can you update the summary
as well? I also realized some new classes are just renaming of previously
existing classes -- my bad.
I still think there is room for breaking up this PR. Based on your summary
there are multiple things going on in this PR:
> 1. Uniformed the behavior of all filter-related classes to bound the
return docIds with numDocs
> 2. Simplified the logic of AND/OR handling
> 3. Pushed down ...
Can we put the 3 items in 3 PRs? I found there are non-trivial coding
refactoring (e.g.,
pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java) mixed
with interface changes in this PR. Can we separate these into smaller PRs?
----------------------------------------------------------------
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
