Adding basic null predicate support #4943

icefury71 · 2019-12-20T02:58:26Z

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.

codecov-io · 2019-12-20T03:50:54Z

Codecov Report

Merging #4943 into master will decrease coverage by 0.22%.
The diff coverage is 65.21%.

@@             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	Coverage Δ	Complexity Δ
...mpl/invertedindex/RealtimeInvertedIndexReader.java	`100% <ø> (+3.03%)`	`0 <0> (ø)`	⬇️
...gment/index/readers/NullValueVectorReaderImpl.java	`100% <100%> (ø)`	`0 <0> (ø)`	⬇️
.../apache/pinot/pql/parsers/pql2/ast/FilterKind.java	`100% <100%> (ø)`	`0 <0> (ø)`	⬇️
...ava/org/apache/pinot/core/plan/FilterPlanNode.java	`92% <100%> (+1.09%)`	`0 <0> (ø)`	⬇️
.../org/apache/pinot/pql/parsers/Pql2AstListener.java	`91.82% <100%> (+2.51%)`	`0 <0> (ø)`	⬇️
...t/pql/parsers/pql2/ast/IsNullPredicateAstNode.java	`32% <32%> (ø)`	`0 <0> (?)`
...e/pinot/core/common/predicate/IsNullPredicate.java	`66.66% <66.66%> (ø)`	`0 <0> (?)`
...inot/core/common/predicate/IsNotNullPredicate.java	`66.66% <66.66%> (ø)`	`0 <0> (?)`
...rg/apache/pinot/common/request/FilterOperator.java	`85.18% <66.66%> (-6.12%)`	`0 <0> (ø)`
...nullvalue/RealtimeNullValueVectorReaderWriter.java	`85.71% <66.66%> (-14.29%)`	`0 <0> (ø)`
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b250439...321a07c. Read the comment docs.

kishoreg

LGTM. Minor comments

pinot-common/src/main/java/org/apache/pinot/pql/parsers/pql2/ast/IsNullPredicateAstNode.java

kishoreg · 2019-12-20T04:28:13Z

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();


move this code to FilterOperatorUtils.getLeafOperator? we can have an IsPredicateEvaluator that return empty list for matchings and non matching dictIds

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.

Yeah - couldn't figure out how to do this in predicate evaluator. Leaving it as is for now.

...ore/src/main/java/org/apache/pinot/core/segment/index/readers/NullValueVectorReaderImpl.java

pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java

Jackie-Jiang

Mostly good except for the thread-safety issue

Jackie-Jiang · 2019-12-20T19:52:19Z

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();


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.

Jackie-Jiang · 2019-12-20T19:58:41Z

.../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() {


This is not thread-safe. You might want to use the ThreadSafeMutableRoaringBitmap wrapper in RealtimeInvertedIndexReader

@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?

@kishoreg MutableRoaringBitmap itself is not thread-safe. You cannot read the bitmap while updating it. One possible solution is using ReadWriteLock for this.

Actually ReadWriteLock might not work in this case as there are too many reading methods

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.

We need synchronization because clone and update cannot happen at the same time either

Missed that. let's create another issue to measure and optimize this.

pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java

...ore/src/main/java/org/apache/pinot/core/segment/index/readers/NullValueVectorReaderImpl.java

- Refactor and use ThreadSafeMutableRoaringBitmap for thread safety - Other minor fixes

icefury71

Addressed comments

Jackie-Jiang

LGTM

pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java

jamesyfshao · 2019-12-20T21:35:38Z

pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java

+      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;


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?

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 }

They are different performance wise. This if condition is redundant.

Adding basic null predicate support

703f1f0

kishoreg reviewed Dec 20, 2019

View reviewed changes

haibow reviewed Dec 20, 2019

View reviewed changes

pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java Outdated Show resolved Hide resolved

Jackie-Jiang reviewed Dec 20, 2019

View reviewed changes

Addressing review comments:

e5dd7ad

- Refactor and use ThreadSafeMutableRoaringBitmap for thread safety - Other minor fixes

icefury71 commented Dec 20, 2019

View reviewed changes

Jackie-Jiang approved these changes Dec 20, 2019

View reviewed changes

pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java Outdated Show resolved Hide resolved

Reformatting code

321a07c

jamesyfshao reviewed Dec 20, 2019

View reviewed changes

Jackie-Jiang merged commit 09db4d9 into apache:master Dec 21, 2019

kishoreg mentioned this pull request Jun 16, 2020

NULL value for metrics #5574

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding basic null predicate support #4943

Adding basic null predicate support #4943

icefury71 commented Dec 20, 2019

codecov-io commented Dec 20, 2019 •

edited

Loading

kishoreg left a comment

kishoreg Dec 20, 2019

Jackie-Jiang Dec 20, 2019

icefury71 Dec 20, 2019

Jackie-Jiang left a comment

Jackie-Jiang Dec 20, 2019

Jackie-Jiang Dec 20, 2019

kishoreg Dec 20, 2019

Jackie-Jiang Dec 20, 2019

Jackie-Jiang Dec 20, 2019

kishoreg Dec 20, 2019

Jackie-Jiang Dec 20, 2019

kishoreg Dec 20, 2019

icefury71 left a comment

Jackie-Jiang left a comment

jamesyfshao Dec 20, 2019

kishoreg Dec 20, 2019

Jackie-Jiang Dec 20, 2019

Adding basic null predicate support #4943

Adding basic null predicate support #4943

Conversation

icefury71 commented Dec 20, 2019

codecov-io commented Dec 20, 2019 • edited Loading

Codecov Report

kishoreg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icefury71 left a comment

Choose a reason for hiding this comment

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Dec 20, 2019 •

edited

Loading