You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/09/21 01:44:11 UTC
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9420: Sort scanning based operators by cardinality in AndDocIdSet evaluation
Jackie-Jiang commented on code in PR #9420:
URL: https://github.com/apache/pinot/pull/9420#discussion_r975953063
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/docidsets/AndDocIdSet.java:
##########
@@ -52,9 +54,12 @@
*/
public final class AndDocIdSet implements FilterBlockDocIdSet {
private final List<FilterBlockDocIdSet> _docIdSets;
+ private final boolean _cardinalityBasedRankingForScan;
- public AndDocIdSet(List<FilterBlockDocIdSet> docIdSets) {
+ public AndDocIdSet(List<FilterBlockDocIdSet> docIdSets, Map<String, String> queryOptions) {
_docIdSets = docIdSets;
+ _cardinalityBasedRankingForScan = queryOptions != null
+ && queryOptions.containsKey(OptimizationSwitches.AND_SCAN_CARDINALITY_BASED_REORDERING);
Review Comment:
Consider moving this to the `QueryOptionsUtils`. We usually parse the value for boolean flag
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/docidsets/AndDocIdSet.java:
##########
@@ -52,9 +54,12 @@
*/
public final class AndDocIdSet implements FilterBlockDocIdSet {
private final List<FilterBlockDocIdSet> _docIdSets;
+ private final boolean _cardinalityBasedRankingForScan;
- public AndDocIdSet(List<FilterBlockDocIdSet> docIdSets) {
+ public AndDocIdSet(List<FilterBlockDocIdSet> docIdSets, Map<String, String> queryOptions) {
_docIdSets = docIdSets;
+ _cardinalityBasedRankingForScan = queryOptions != null
Review Comment:
(nit) IIRC, `queryOptions` should never be `null`. If it can be null, we should annotate it accordingly
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -1467,7 +1467,8 @@ private class IndexContainer implements Closeable {
DataSource toDataSource() {
return new MutableDataSource(_fieldSpec, _numDocsIndexed, _numValuesInfo._numValues,
- _numValuesInfo._maxNumValuesPerMVEntry, _partitionFunction, _partitions, _minValue, _maxValue, _forwardIndex,
+ _numValuesInfo._maxNumValuesPerMVEntry, _statsHistory.getEstimatedCardinality(_fieldSpec.getName()),
Review Comment:
I wouldn't read the cardinality from the `_statsHistory` for the following reasons:
1. It can be expensive, and calling it for every column at query time can cause too much overhead
2. It won't reflect the cardinality for the current consumed records
Consider using the `_dictionary.length()` when dictionary is available, `-1` if not
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org