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 2019/12/20 02:58:27 UTC

[GitHub] [incubator-pinot] icefury71 opened a new pull request #4943: Adding basic null predicate support

icefury71 opened a new pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943
 
 
   Adding null predicate support in reference to Issue #4230 
   
   This PR adds limited support for "IS NULL" and "IS NOT NULL" filter predicates. Currently this only works for leaf filter predicates.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#discussion_r360590839
 
 

 ##########
 File path: pinot-core/src/main/java/org/apache/pinot/core/realtime/impl/nullvalue/RealtimeNullValueVectorReaderWriter.java
 ##########
 @@ -39,4 +40,9 @@ public void setNull(int docId) {
   public boolean isNull(int docId) {
     return _nullBitmap.contains(docId);
   }
+
+  @Override
+  public ImmutableRoaringBitmap getNullBitmap() {
 
 Review comment:
   @kishoreg MutableRoaringBitmap itself is not thread-safe. You cannot read the bitmap while updating it. One possible solution is using ReadWriteLock for this.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] kishoreg commented on a change in pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#discussion_r360588163
 
 

 ##########
 File path: pinot-core/src/main/java/org/apache/pinot/core/realtime/impl/nullvalue/RealtimeNullValueVectorReaderWriter.java
 ##########
 @@ -39,4 +40,9 @@ public void setNull(int docId) {
   public boolean isNull(int docId) {
     return _nullBitmap.contains(docId);
   }
+
+  @Override
+  public ImmutableRoaringBitmap getNullBitmap() {
 
 Review comment:
   @Jackie-Jiang why do we need that? there is only one thread updating the bitmap and multiple threads reading it. cloning entire bitmap for every access is going to be expensive. when did we do this change?

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#discussion_r360580010
 
 

 ##########
 File path: pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java
 ##########
 @@ -111,6 +113,15 @@ private static BaseFilterOperator constructPhysicalOperator(FilterQueryTree filt
       // Leaf filter operator
       Predicate predicate = Predicate.newPredicate(filterQueryTree);
 
+      // Check for null predicate
+      Predicate.Type type = predicate.getType();
+      if (type.equals(Predicate.Type.IS_NULL) || type.equals(Predicate.Type.IS_NOT_NULL)) {
+        DataSource dataSource = segment.getDataSource(filterQueryTree.getColumn());
+        ImmutableRoaringBitmap nullBitmap = dataSource.getNullValueVector().getNullBitmap();
+        boolean exclusive = (type == Predicate.Type.IS_NULL) ? false : true;
+        return new BitmapBasedFilterOperator(new ImmutableRoaringBitmap[]{nullBitmap}, 0, numDocs-1, exclusive);
 
 Review comment:
   I tried reformatting, which will change `numDocs-1` to `numDocs - 1`, others remain the same

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] kishoreg commented on a change in pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#discussion_r360587208
 
 

 ##########
 File path: pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java
 ##########
 @@ -111,6 +113,15 @@ private static BaseFilterOperator constructPhysicalOperator(FilterQueryTree filt
       // Leaf filter operator
       Predicate predicate = Predicate.newPredicate(filterQueryTree);
 
+      // Check for null predicate
+      Predicate.Type type = predicate.getType();
+      if (type.equals(Predicate.Type.IS_NULL) || type.equals(Predicate.Type.IS_NOT_NULL)) {
+        DataSource dataSource = segment.getDataSource(filterQueryTree.getColumn());
+        ImmutableRoaringBitmap nullBitmap = dataSource.getNullValueVector().getNullBitmap();
+        boolean exclusive = (type == Predicate.Type.IS_NULL) ? false : true;
 
 Review comment:
   I prefer avoiding ternary operators altogether as a convention :) https://agiletribe.wordpress.com/2011/11/01/21-avoid-ternary-conditional-operator/ 
   ```
   boolean exclusive = false;
   if(type == Predicate.Type.IS_NULL) {
     exclusive = true
   }
   ```

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#discussion_r360548796
 
 

 ##########
 File path: pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java
 ##########
 @@ -111,6 +113,15 @@ private static BaseFilterOperator constructPhysicalOperator(FilterQueryTree filt
       // Leaf filter operator
       Predicate predicate = Predicate.newPredicate(filterQueryTree);
 
+      // Check for null predicate
+      Predicate.Type type = predicate.getType();
 
 Review comment:
   I think it is a little bit hard to model NULL into PredicateEvaluator though, as there is no easy way to hook the nullValueVector into it. I'm okay making this an extra check for now.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#discussion_r360552026
 
 

 ##########
 File path: pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java
 ##########
 @@ -111,6 +113,15 @@ private static BaseFilterOperator constructPhysicalOperator(FilterQueryTree filt
       // Leaf filter operator
       Predicate predicate = Predicate.newPredicate(filterQueryTree);
 
+      // Check for null predicate
+      Predicate.Type type = predicate.getType();
+      if (type.equals(Predicate.Type.IS_NULL) || type.equals(Predicate.Type.IS_NOT_NULL)) {
+        DataSource dataSource = segment.getDataSource(filterQueryTree.getColumn());
+        ImmutableRoaringBitmap nullBitmap = dataSource.getNullValueVector().getNullBitmap();
+        boolean exclusive = (type == Predicate.Type.IS_NULL) ? false : true;
 
 Review comment:
   `boolean exclusive = type == Predicate.Type.IS_NOT_NULL;` for readability?

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] jamesyfshao commented on a change in pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
jamesyfshao commented on a change in pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#discussion_r360582910
 
 

 ##########
 File path: pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java
 ##########
 @@ -111,6 +113,15 @@ private static BaseFilterOperator constructPhysicalOperator(FilterQueryTree filt
       // Leaf filter operator
       Predicate predicate = Predicate.newPredicate(filterQueryTree);
 
+      // Check for null predicate
+      Predicate.Type type = predicate.getType();
+      if (type.equals(Predicate.Type.IS_NULL) || type.equals(Predicate.Type.IS_NOT_NULL)) {
+        DataSource dataSource = segment.getDataSource(filterQueryTree.getColumn());
+        ImmutableRoaringBitmap nullBitmap = dataSource.getNullValueVector().getNullBitmap();
+        boolean exclusive = (type == Predicate.Type.IS_NULL) ? false : true;
 
 Review comment:
   I think we can either change this to boolean exclusive = type != Predicate.Type.IS_NULL or maybe move the entire thing to the next statement?

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] icefury71 commented on a change in pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
icefury71 commented on a change in pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#discussion_r360612396
 
 

 ##########
 File path: pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java
 ##########
 @@ -111,6 +113,15 @@ private static BaseFilterOperator constructPhysicalOperator(FilterQueryTree filt
       // Leaf filter operator
       Predicate predicate = Predicate.newPredicate(filterQueryTree);
 
+      // Check for null predicate
+      Predicate.Type type = predicate.getType();
+      if (type.equals(Predicate.Type.IS_NULL) || type.equals(Predicate.Type.IS_NOT_NULL)) {
+        DataSource dataSource = segment.getDataSource(filterQueryTree.getColumn());
+        ImmutableRoaringBitmap nullBitmap = dataSource.getNullValueVector().getNullBitmap();
+        boolean exclusive = (type == Predicate.Type.IS_NULL) ? false : true;
 
 Review comment:
   Done

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] kishoreg commented on a change in pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#discussion_r360597013
 
 

 ##########
 File path: pinot-core/src/main/java/org/apache/pinot/core/realtime/impl/nullvalue/RealtimeNullValueVectorReaderWriter.java
 ##########
 @@ -39,4 +40,9 @@ public void setNull(int docId) {
   public boolean isNull(int docId) {
     return _nullBitmap.contains(docId);
   }
+
+  @Override
+  public ImmutableRoaringBitmap getNullBitmap() {
 
 Review comment:
   Missed that. let's create another issue to measure and optimize this.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] icefury71 commented on a change in pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
icefury71 commented on a change in pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#discussion_r360535885
 
 

 ##########
 File path: pinot-common/src/main/java/org/apache/pinot/pql/parsers/pql2/ast/IsNullPredicateAstNode.java
 ##########
 @@ -0,0 +1,85 @@
+/**
+ * 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.pql.parsers.pql2.ast;
+
+import java.util.Collections;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.FilterOperator;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.HavingQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.pql.parsers.Pql2CompilationException;
+
+
+/**
+ * AST node for IS predicates (foo IS NULL, foo IS NOT NULL).
+ */
+public class IsNullPredicateAstNode extends PredicateAstNode {
+
+  private final boolean _isNegation;
+
+  public IsNullPredicateAstNode(boolean notNodeExists) {
+    _isNegation = notNodeExists;
+  }
+
+  @Override
+  public void addChild(AstNode childNode) {
+    if (childNode instanceof IdentifierAstNode) {
+      if (_identifier == null) {
+        _identifier = ((IdentifierAstNode) childNode).getName();
+      } else {
+        throw new Pql2CompilationException("Only one column supported in IS predicate.");
+      }
+    } else if (childNode instanceof FunctionCallAstNode) {
+      throw new Pql2CompilationException("Function not supported in IS predicate");
+    } else if (childNode instanceof LiteralAstNode) {
+      throw new Pql2CompilationException("Constants not supported in IS predicate");
+    }
+
+    // Add the child nonetheless
+    super.addChild(childNode);
 
 Review comment:
   will move this to the final else part (as is done in other predicate ast node classes)

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-io edited a comment on issue #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#issuecomment-567776681
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/4943?src=pr&el=h1) Report
   > Merging [#4943](https://codecov.io/gh/apache/incubator-pinot/pull/4943?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/b250439fb7f14d1217e47bfdd22f67154d0fbd6b?src=pr&el=desc) will **decrease** coverage by `0.22%`.
   > The diff coverage is `65.21%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/4943/graphs/tree.svg?width=650&token=4ibza2ugkz&height=150&src=pr)](https://codecov.io/gh/apache/incubator-pinot/pull/4943?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #4943      +/-   ##
   ============================================
   - Coverage     56.57%   56.34%   -0.23%     
     Complexity       16       16              
   ============================================
     Files          1176     1180       +4     
     Lines         62835    63068     +233     
     Branches       9222     9251      +29     
   ============================================
   - Hits          35549    35538      -11     
   - Misses        24643    24881     +238     
   - Partials       2643     2649       +6
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/4943?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...mpl/invertedindex/RealtimeInvertedIndexReader.java](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2ludmVydGVkaW5kZXgvUmVhbHRpbWVJbnZlcnRlZEluZGV4UmVhZGVyLmphdmE=) | `100% <ø> (+3.03%)` | `0 <0> (ø)` | :arrow_down: |
   | [...gment/index/readers/NullValueVectorReaderImpl.java](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L2luZGV4L3JlYWRlcnMvTnVsbFZhbHVlVmVjdG9yUmVhZGVySW1wbC5qYXZh) | `100% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
   | [.../apache/pinot/pql/parsers/pql2/ast/FilterKind.java](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9wcWwyL2FzdC9GaWx0ZXJLaW5kLmphdmE=) | `100% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
   | [...ava/org/apache/pinot/core/plan/FilterPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0ZpbHRlclBsYW5Ob2RlLmphdmE=) | `92% <100%> (+1.09%)` | `0 <0> (ø)` | :arrow_down: |
   | [.../org/apache/pinot/pql/parsers/Pql2AstListener.java](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9QcWwyQXN0TGlzdGVuZXIuamF2YQ==) | `91.82% <100%> (+2.51%)` | `0 <0> (ø)` | :arrow_down: |
   | [...t/pql/parsers/pql2/ast/IsNullPredicateAstNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9wcWwyL2FzdC9Jc051bGxQcmVkaWNhdGVBc3ROb2RlLmphdmE=) | `32% <32%> (ø)` | `0 <0> (?)` | |
   | [...e/pinot/core/common/predicate/IsNullPredicate.java](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vcHJlZGljYXRlL0lzTnVsbFByZWRpY2F0ZS5qYXZh) | `66.66% <66.66%> (ø)` | `0 <0> (?)` | |
   | [...inot/core/common/predicate/IsNotNullPredicate.java](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vcHJlZGljYXRlL0lzTm90TnVsbFByZWRpY2F0ZS5qYXZh) | `66.66% <66.66%> (ø)` | `0 <0> (?)` | |
   | [...rg/apache/pinot/common/request/FilterOperator.java](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVxdWVzdC9GaWx0ZXJPcGVyYXRvci5qYXZh) | `85.18% <66.66%> (-6.12%)` | `0 <0> (ø)` | |
   | [...nullvalue/RealtimeNullValueVectorReaderWriter.java](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL251bGx2YWx1ZS9SZWFsdGltZU51bGxWYWx1ZVZlY3RvclJlYWRlcldyaXRlci5qYXZh) | `85.71% <66.66%> (-14.29%)` | `0 <0> (ø)` | |
   | ... and [40 more](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/4943?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/4943?src=pr&el=footer). Last update [b250439...321a07c](https://codecov.io/gh/apache/incubator-pinot/pull/4943?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#discussion_r360596600
 
 

 ##########
 File path: pinot-core/src/main/java/org/apache/pinot/core/realtime/impl/nullvalue/RealtimeNullValueVectorReaderWriter.java
 ##########
 @@ -39,4 +40,9 @@ public void setNull(int docId) {
   public boolean isNull(int docId) {
     return _nullBitmap.contains(docId);
   }
+
+  @Override
+  public ImmutableRoaringBitmap getNullBitmap() {
 
 Review comment:
   We need synchronization because clone and update cannot happen at the same time either

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#discussion_r360594862
 
 

 ##########
 File path: pinot-core/src/main/java/org/apache/pinot/core/realtime/impl/nullvalue/RealtimeNullValueVectorReaderWriter.java
 ##########
 @@ -39,4 +40,9 @@ public void setNull(int docId) {
   public boolean isNull(int docId) {
     return _nullBitmap.contains(docId);
   }
+
+  @Override
+  public ImmutableRoaringBitmap getNullBitmap() {
 
 Review comment:
   Actually ReadWriteLock might not work in this case as there are too many reading methods

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] icefury71 commented on a change in pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
icefury71 commented on a change in pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#discussion_r360612358
 
 

 ##########
 File path: pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java
 ##########
 @@ -111,6 +113,15 @@ private static BaseFilterOperator constructPhysicalOperator(FilterQueryTree filt
       // Leaf filter operator
       Predicate predicate = Predicate.newPredicate(filterQueryTree);
 
+      // Check for null predicate
+      Predicate.Type type = predicate.getType();
+      if (type.equals(Predicate.Type.IS_NULL) || type.equals(Predicate.Type.IS_NOT_NULL)) {
+        DataSource dataSource = segment.getDataSource(filterQueryTree.getColumn());
+        ImmutableRoaringBitmap nullBitmap = dataSource.getNullValueVector().getNullBitmap();
+        boolean exclusive = (type == Predicate.Type.IS_NULL) ? false : true;
+        return new BitmapBasedFilterOperator(new ImmutableRoaringBitmap[]{nullBitmap}, 0, numDocs-1, exclusive);
 
 Review comment:
   Formatted it manually for now. 

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] kishoreg commented on a change in pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#discussion_r360596039
 
 

 ##########
 File path: pinot-core/src/main/java/org/apache/pinot/core/realtime/impl/nullvalue/RealtimeNullValueVectorReaderWriter.java
 ##########
 @@ -39,4 +40,9 @@ public void setNull(int docId) {
   public boolean isNull(int docId) {
     return _nullBitmap.contains(docId);
   }
+
+  @Override
+  public ImmutableRoaringBitmap getNullBitmap() {
 
 Review comment:
   I didn't know about this. we should measure the performance impact on the ingestion rate (due to synchronization) and query throughput (bcos to clone). Readwrite lock will not help since its getting updated frequently.
   
    The synchronization is not really needed since only one thread(consumer) is updating it. clone is important.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang merged pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943
 
 
   

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#discussion_r360551112
 
 

 ##########
 File path: pinot-core/src/main/java/org/apache/pinot/core/realtime/impl/nullvalue/RealtimeNullValueVectorReaderWriter.java
 ##########
 @@ -39,4 +40,9 @@ public void setNull(int docId) {
   public boolean isNull(int docId) {
     return _nullBitmap.contains(docId);
   }
+
+  @Override
+  public ImmutableRoaringBitmap getNullBitmap() {
 
 Review comment:
   This is not thread-safe. You might want to use the ThreadSafeMutableRoaringBitmap wrapper in RealtimeInvertedIndexReader

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-io edited a comment on issue #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#issuecomment-567776681
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/4943?src=pr&el=h1) Report
   > Merging [#4943](https://codecov.io/gh/apache/incubator-pinot/pull/4943?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/b250439fb7f14d1217e47bfdd22f67154d0fbd6b?src=pr&el=desc) will **decrease** coverage by `0.27%`.
   > The diff coverage is `65.21%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/4943/graphs/tree.svg?width=650&token=4ibza2ugkz&height=150&src=pr)](https://codecov.io/gh/apache/incubator-pinot/pull/4943?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##             master   #4943      +/-   ##
   ===========================================
   - Coverage     56.57%   56.3%   -0.28%     
     Complexity       16      16              
   ===========================================
     Files          1176    1180       +4     
     Lines         62835   63068     +233     
     Branches       9222    9251      +29     
   ===========================================
   - Hits          35549   35508      -41     
   - Misses        24643   24917     +274     
     Partials       2643    2643
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/4943?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...mpl/invertedindex/RealtimeInvertedIndexReader.java](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2ludmVydGVkaW5kZXgvUmVhbHRpbWVJbnZlcnRlZEluZGV4UmVhZGVyLmphdmE=) | `100% <ø> (+3.03%)` | `0 <0> (ø)` | :arrow_down: |
   | [...gment/index/readers/NullValueVectorReaderImpl.java](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L2luZGV4L3JlYWRlcnMvTnVsbFZhbHVlVmVjdG9yUmVhZGVySW1wbC5qYXZh) | `100% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
   | [.../apache/pinot/pql/parsers/pql2/ast/FilterKind.java](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9wcWwyL2FzdC9GaWx0ZXJLaW5kLmphdmE=) | `100% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
   | [...ava/org/apache/pinot/core/plan/FilterPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0ZpbHRlclBsYW5Ob2RlLmphdmE=) | `92% <100%> (+1.09%)` | `0 <0> (ø)` | :arrow_down: |
   | [.../org/apache/pinot/pql/parsers/Pql2AstListener.java](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9QcWwyQXN0TGlzdGVuZXIuamF2YQ==) | `91.82% <100%> (+2.51%)` | `0 <0> (ø)` | :arrow_down: |
   | [...t/pql/parsers/pql2/ast/IsNullPredicateAstNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9wcWwyL2FzdC9Jc051bGxQcmVkaWNhdGVBc3ROb2RlLmphdmE=) | `32% <32%> (ø)` | `0 <0> (?)` | |
   | [...e/pinot/core/common/predicate/IsNullPredicate.java](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vcHJlZGljYXRlL0lzTnVsbFByZWRpY2F0ZS5qYXZh) | `66.66% <66.66%> (ø)` | `0 <0> (?)` | |
   | [...inot/core/common/predicate/IsNotNullPredicate.java](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vcHJlZGljYXRlL0lzTm90TnVsbFByZWRpY2F0ZS5qYXZh) | `66.66% <66.66%> (ø)` | `0 <0> (?)` | |
   | [...rg/apache/pinot/common/request/FilterOperator.java](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVxdWVzdC9GaWx0ZXJPcGVyYXRvci5qYXZh) | `85.18% <66.66%> (-6.12%)` | `0 <0> (ø)` | |
   | [...nullvalue/RealtimeNullValueVectorReaderWriter.java](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL251bGx2YWx1ZS9SZWFsdGltZU51bGxWYWx1ZVZlY3RvclJlYWRlcldyaXRlci5qYXZh) | `85.71% <66.66%> (-14.29%)` | `0 <0> (ø)` | |
   | ... and [34 more](https://codecov.io/gh/apache/incubator-pinot/pull/4943/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/4943?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/4943?src=pr&el=footer). Last update [b250439...321a07c](https://codecov.io/gh/apache/incubator-pinot/pull/4943?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#discussion_r360594167
 
 

 ##########
 File path: pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java
 ##########
 @@ -111,6 +113,15 @@ private static BaseFilterOperator constructPhysicalOperator(FilterQueryTree filt
       // Leaf filter operator
       Predicate predicate = Predicate.newPredicate(filterQueryTree);
 
+      // Check for null predicate
+      Predicate.Type type = predicate.getType();
+      if (type.equals(Predicate.Type.IS_NULL) || type.equals(Predicate.Type.IS_NOT_NULL)) {
+        DataSource dataSource = segment.getDataSource(filterQueryTree.getColumn());
+        ImmutableRoaringBitmap nullBitmap = dataSource.getNullValueVector().getNullBitmap();
+        boolean exclusive = (type == Predicate.Type.IS_NULL) ? false : true;
 
 Review comment:
   They are different performance wise. This if condition is redundant.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] icefury71 commented on a change in pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
icefury71 commented on a change in pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#discussion_r360574352
 
 

 ##########
 File path: pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java
 ##########
 @@ -111,6 +113,15 @@ private static BaseFilterOperator constructPhysicalOperator(FilterQueryTree filt
       // Leaf filter operator
       Predicate predicate = Predicate.newPredicate(filterQueryTree);
 
+      // Check for null predicate
+      Predicate.Type type = predicate.getType();
 
 Review comment:
   Yeah - couldn't figure out how to do this in predicate evaluator. Leaving it as is for now.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] icefury71 commented on a change in pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
icefury71 commented on a change in pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#discussion_r360612329
 
 

 ##########
 File path: pinot-core/src/main/java/org/apache/pinot/core/segment/index/readers/NullValueVectorReaderImpl.java
 ##########
 @@ -36,4 +36,8 @@ public boolean isNull(int docId) {
     return _nullBitmap.contains(docId);
   }
 
+  @Override
+  public ImmutableRoaringBitmap getNullBitmap() {
+    return _nullBitmap.clone();
 
 Review comment:
   Done

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#discussion_r360552202
 
 

 ##########
 File path: pinot-core/src/main/java/org/apache/pinot/core/segment/index/readers/NullValueVectorReaderImpl.java
 ##########
 @@ -36,4 +36,8 @@ public boolean isNull(int docId) {
     return _nullBitmap.contains(docId);
   }
 
+  @Override
+  public ImmutableRoaringBitmap getNullBitmap() {
+    return _nullBitmap.clone();
 
 Review comment:
   +1

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] icefury71 commented on a change in pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
icefury71 commented on a change in pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#discussion_r360612301
 
 

 ##########
 File path: pinot-common/src/main/java/org/apache/pinot/pql/parsers/pql2/ast/IsNullPredicateAstNode.java
 ##########
 @@ -0,0 +1,85 @@
+/**
+ * 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.pql.parsers.pql2.ast;
+
+import java.util.Collections;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.FilterOperator;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.HavingQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.pql.parsers.Pql2CompilationException;
+
+
+/**
+ * AST node for IS predicates (foo IS NULL, foo IS NOT NULL).
+ */
+public class IsNullPredicateAstNode extends PredicateAstNode {
+
+  private final boolean _isNegation;
+
+  public IsNullPredicateAstNode(boolean notNodeExists) {
+    _isNegation = notNodeExists;
+  }
+
+  @Override
+  public void addChild(AstNode childNode) {
+    if (childNode instanceof IdentifierAstNode) {
+      if (_identifier == null) {
+        _identifier = ((IdentifierAstNode) childNode).getName();
+      } else {
+        throw new Pql2CompilationException("Only one column supported in IS predicate.");
+      }
+    } else if (childNode instanceof FunctionCallAstNode) {
+      throw new Pql2CompilationException("Function not supported in IS predicate");
+    } else if (childNode instanceof LiteralAstNode) {
+      throw new Pql2CompilationException("Constants not supported in IS predicate");
+    }
+
+    // Add the child nonetheless
+    super.addChild(childNode);
 
 Review comment:
   Done

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] icefury71 commented on a change in pull request #4943: Adding basic null predicate support

Posted by GitBox <gi...@apache.org>.
icefury71 commented on a change in pull request #4943: Adding basic null predicate support
URL: https://github.com/apache/incubator-pinot/pull/4943#discussion_r360560846
 
 

 ##########
 File path: pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java
 ##########
 @@ -111,6 +113,15 @@ private static BaseFilterOperator constructPhysicalOperator(FilterQueryTree filt
       // Leaf filter operator
       Predicate predicate = Predicate.newPredicate(filterQueryTree);
 
+      // Check for null predicate
+      Predicate.Type type = predicate.getType();
+      if (type.equals(Predicate.Type.IS_NULL) || type.equals(Predicate.Type.IS_NOT_NULL)) {
+        DataSource dataSource = segment.getDataSource(filterQueryTree.getColumn());
+        ImmutableRoaringBitmap nullBitmap = dataSource.getNullValueVector().getNullBitmap();
+        boolean exclusive = (type == Predicate.Type.IS_NULL) ? false : true;
+        return new BitmapBasedFilterOperator(new ImmutableRoaringBitmap[]{nullBitmap}, 0, numDocs-1, exclusive);
 
 Review comment:
   Tried reformatting - doesn't seem to affect this. 

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org