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/02/09 22:29:15 UTC
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #8148: Implement NOT Operator
Jackie-Jiang commented on a change in pull request #8148:
URL: https://github.com/apache/pinot/pull/8148#discussion_r803136508
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/NotDocIdIterator.java
##########
@@ -0,0 +1,97 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.operator.dociditerators;
+
+import org.apache.pinot.core.common.BlockDocIdIterator;
+import org.apache.pinot.segment.spi.Constants;
+
+
+/**
+ * The iterator performs a linear pass through the underlying child iterator and returns
+ * the complement of the result set.
+ */
+public class NotDocIdIterator implements BlockDocIdIterator {
+ private BlockDocIdIterator _childDocIdIterator;
+ private int _lowerLimit;
+ private int _upperLimit;
Review comment:
Suggest renaming `_upperLimit` to `_nextNonMatchingDocId`
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/NotDocIdIterator.java
##########
@@ -0,0 +1,97 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.operator.dociditerators;
+
+import org.apache.pinot.core.common.BlockDocIdIterator;
+import org.apache.pinot.segment.spi.Constants;
+
+
+/**
+ * The iterator performs a linear pass through the underlying child iterator and returns
+ * the complement of the result set.
+ */
+public class NotDocIdIterator implements BlockDocIdIterator {
+ private BlockDocIdIterator _childDocIdIterator;
+ private int _lowerLimit;
+ private int _upperLimit;
+ private int _numDocs;
+
+ public NotDocIdIterator(BlockDocIdIterator childDocIdIterator, int numDocs) {
+ _childDocIdIterator = childDocIdIterator;
+ _lowerLimit = 0;
+
+ int currentDocIdFromChildIterator = childDocIdIterator.next();
+ _upperLimit = currentDocIdFromChildIterator == Constants.EOF ? numDocs : currentDocIdFromChildIterator;
Review comment:
No need to initialize the upper limit here. The value check in `next()` should be able to handle that
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java
##########
@@ -183,6 +183,12 @@ public static FilterContext getFilter(Expression thriftExpression) {
children.add(getFilter(operand));
}
return new FilterContext(FilterContext.Type.OR, children, null);
+ case NOT:
+ children = new ArrayList<>(numOperands);
+ for (Expression operand : operands) {
+ children.add(getFilter(operand));
+ }
+ return new FilterContext(FilterContext.Type.NOT, children, null);
Review comment:
(minor) `numOperands` should always be 1, so this part can be optimized
```suggestion
assert numOperands == 1;
return new FilterContext(FilterContext.Type.NOT, Collections.singletonList(getFilter(operands.get(0))), null);
```
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/filter/FilterOperatorUtils.java
##########
@@ -165,6 +179,9 @@ int getPriority(BaseFilterOperator filterOperator) {
if (filterOperator instanceof OrFilterOperator) {
return 4;
}
+ if (filterOperator instanceof NotFilterOperator) {
Review comment:
`NotFilterOperator` should have the same priority as it's child operator
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/NotDocIdIterator.java
##########
@@ -0,0 +1,97 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.operator.dociditerators;
+
+import org.apache.pinot.core.common.BlockDocIdIterator;
+import org.apache.pinot.segment.spi.Constants;
+
+
+/**
+ * The iterator performs a linear pass through the underlying child iterator and returns
+ * the complement of the result set.
+ */
+public class NotDocIdIterator implements BlockDocIdIterator {
+ private BlockDocIdIterator _childDocIdIterator;
+ private int _lowerLimit;
Review comment:
Suggest renaming `_lowerLimit` to `_nextDocId` to be consistent with other iterators?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/NotDocIdIterator.java
##########
@@ -0,0 +1,97 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.operator.dociditerators;
+
+import org.apache.pinot.core.common.BlockDocIdIterator;
+import org.apache.pinot.segment.spi.Constants;
+
+
+/**
+ * The iterator performs a linear pass through the underlying child iterator and returns
+ * the complement of the result set.
+ */
+public class NotDocIdIterator implements BlockDocIdIterator {
+ private BlockDocIdIterator _childDocIdIterator;
+ private int _lowerLimit;
+ private int _upperLimit;
+ private int _numDocs;
+
+ public NotDocIdIterator(BlockDocIdIterator childDocIdIterator, int numDocs) {
+ _childDocIdIterator = childDocIdIterator;
+ _lowerLimit = 0;
+
+ int currentDocIdFromChildIterator = childDocIdIterator.next();
+ _upperLimit = currentDocIdFromChildIterator == Constants.EOF ? numDocs : currentDocIdFromChildIterator;
+ _numDocs = numDocs;
+ }
+
+ @Override
+ public int next() {
+ while (_lowerLimit == _upperLimit) {
Review comment:
Change this to `while (_lowerLimit >= _upperLimit)`, then both constructor and `advance()` can be simplified
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/NotDocIdIterator.java
##########
@@ -0,0 +1,97 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.operator.dociditerators;
+
+import org.apache.pinot.core.common.BlockDocIdIterator;
+import org.apache.pinot.segment.spi.Constants;
+
+
+/**
+ * The iterator performs a linear pass through the underlying child iterator and returns
+ * the complement of the result set.
+ */
+public class NotDocIdIterator implements BlockDocIdIterator {
+ private BlockDocIdIterator _childDocIdIterator;
+ private int _lowerLimit;
+ private int _upperLimit;
+ private int _numDocs;
+
+ public NotDocIdIterator(BlockDocIdIterator childDocIdIterator, int numDocs) {
+ _childDocIdIterator = childDocIdIterator;
+ _lowerLimit = 0;
+
+ int currentDocIdFromChildIterator = childDocIdIterator.next();
+ _upperLimit = currentDocIdFromChildIterator == Constants.EOF ? numDocs : currentDocIdFromChildIterator;
+ _numDocs = numDocs;
+ }
+
+ @Override
+ public int next() {
+ while (_lowerLimit == _upperLimit) {
+ _lowerLimit = _upperLimit + 1;
+
+ int nextMatchingDocId = _childDocIdIterator.next();
+
+ if (nextMatchingDocId == Constants.EOF) {
+ _upperLimit = _numDocs;
+ } else {
+ _upperLimit = nextMatchingDocId;
+ }
+ }
+
+ if (_lowerLimit >= _numDocs) {
+ return Constants.EOF;
+ }
+
+ return _lowerLimit++;
+ }
+
+ @Override
+ public int advance(int targetDocId) {
+ if (targetDocId == Constants.EOF || targetDocId > _numDocs) {
Review comment:
This can be simplified to:
```
_nextDocId = targetDocId;
return next();
```
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/NotDocIdIterator.java
##########
@@ -0,0 +1,97 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.operator.dociditerators;
+
+import org.apache.pinot.core.common.BlockDocIdIterator;
+import org.apache.pinot.segment.spi.Constants;
+
+
+/**
+ * The iterator performs a linear pass through the underlying child iterator and returns
+ * the complement of the result set.
+ */
+public class NotDocIdIterator implements BlockDocIdIterator {
+ private BlockDocIdIterator _childDocIdIterator;
Review comment:
Make `_childDocIdIterator` and `_numDocs` final
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/filter/NotFilterOperator.java
##########
@@ -0,0 +1,63 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.operator.filter;
+
+import java.util.ArrayList;
+import java.util.List;
+import javax.annotation.Nullable;
+import org.apache.pinot.core.common.Operator;
+import org.apache.pinot.core.operator.blocks.FilterBlock;
+import org.apache.pinot.core.operator.docidsets.NotDocIdSet;
+
+
+public class NotFilterOperator extends BaseFilterOperator {
+ private static final String OPERATOR_NAME = "NotFilterOperator";
+ private static final String EXPLAIN_NAME = "FILTER_NOT";
+ private final BaseFilterOperator _filterOperator;
+ private final int _numDocs;
+
+ public NotFilterOperator(BaseFilterOperator filterOperator, int numDocs) {
+ _filterOperator = filterOperator;
+ _numDocs = numDocs;
+ }
+
+ @Override
+ public String getOperatorName() {
+ return OPERATOR_NAME;
+ }
+
+ @Override
+ public List<Operator> getChildOperators() {
+ List<Operator> resultList = new ArrayList<>();
+ resultList.add(_filterOperator);
+
+ return resultList;
Review comment:
(minor)
```suggestion
return Collections.singletonList(_filterOperator);
```
--
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