[GitHub] [pinot] shenyu0127 commented on a diff in pull request #10613: [feature] [null support # 10] Add null support in all transform function and pass the bitmap to upstream

["shenyu0127 (via GitHub)" <gi...@apache.org>]

shenyu0127 commented on code in PR #10613:
URL: https://github.com/apache/pinot/pull/10613#discussion_r1201343304


##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/NullHandlingIntegrationTest.java:
##########
@@ -299,4 +296,189 @@ public void testNullLiteralSelectionOnlyBroker()
     assertEquals(rows.size(), 1);
     assertEquals(rows.get(0).get(0).asText(), "null");
   }
+
+  @Test
+  public void testNullSelectionOnlyTransform()

Review Comment:
   nit: break down the test into multiple tests so that we test behaviors separately.



##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunction.java:
##########
@@ -904,7 +904,7 @@ public Pair<byte[][], RoaringBitmap> transformToBytesValuesSVWithNull(ValueBlock
     final RoaringBitmap bitmap = new RoaringBitmap();
     int[] selected = getSelectedArray(valueBlock, true);
     int numDocs = valueBlock.getNumDocs();
-    initStringValuesSV(numDocs);
+    initBytesValuesSV(numDocs);

Review Comment:
   optional: add a unit test so that if we revert this line the unit test fails.



##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/NullHandlingIntegrationTest.java:
##########
@@ -299,4 +296,189 @@ public void testNullLiteralSelectionOnlyBroker()
     assertEquals(rows.size(), 1);
     assertEquals(rows.get(0).get(0).asText(), "null");
   }
+
+  @Test
+  public void testNullSelectionOnlyTransform()
+      throws Exception {
+    BitSet nullSalary = new BitSet();
+    String sqlQuery = "SELECT salary FROM " + getTableName() + " OPTION(enableNullHandling=true);";
+    JsonNode response = postQuery(sqlQuery, _brokerBaseApiUrl);
+    JsonNode rows = response.get("resultTable").get("rows");
+    double[] salaries = new double[10];
+    for (int i = 0; i < 10; i++) {
+      if (rows.get(i).get(0).asText().equals("null")) {
+        nullSalary.set(i);
+      } else {
+        salaries[i] = rows.get(i).get(0).asDouble();
+      }
+    }
+    BitSet nullDescription = new BitSet();
+    sqlQuery = "SELECT description FROM " + getTableName() + " OPTION(enableNullHandling=true);";
+    response = postQuery(sqlQuery, _brokerBaseApiUrl);
+    rows = response.get("resultTable").get("rows");
+    for (int i = 0; i < 10; i++) {
+      if (rows.get(i).get(0).asText().equals("null")) {
+        nullDescription.set(i);
+      }
+    }
+    // AdditionTransformFunction
+    sqlQuery = "SELECT add(salary, add(null, 1)) FROM " + getTableName() + " OPTION(enableNullHandling=true);";
+    response = postQuery(sqlQuery, _brokerBaseApiUrl);
+    rows = response.get("resultTable").get("rows");
+    for (int i = 0; i < 10; i++) {
+      if (nullSalary.get(i)) {
+        assertEquals(rows.get(i).get(0).asText(), "null");
+      }

Review Comment:
   Assert that every row is null?



##########
pinot-core/src/main/java/org/apache/pinot/core/common/RowBasedBlockValueFetcher.java:
##########


Review Comment:
   Please file an issue to create a unit test file for this file as well as TransformBlock and TransformBlockValSet.



##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/ScalarTransformFunctionWrapper.java:
##########
@@ -174,7 +174,7 @@ public Pair<int[], RoaringBitmap> transformToIntValuesSVWithNull(ValueBlock valu
       }
       Object result = _functionInvoker.invoke(_scalarArguments);
       if (result != null) {
-        _intValuesSV[i] = (int) result;
+        _intValuesSV[i] = (int) _resultType.toInternal(result);

Review Comment:
   optional: add a unit test so that if we revert this line the unit test fails.



##########
pinot-core/src/main/java/org/apache/pinot/core/operator/docvalsets/TransformBlockValSet.java:
##########
@@ -46,47 +45,23 @@ public class TransformBlockValSet implements BlockValSet {
   private final TransformFunction _transformFunction;
   private final ExpressionContext _expression;
 
-  private boolean _nullBitmapSet;
-  private RoaringBitmap _nullBitmap;
+  private RoaringBitmap _nullBitmap = null;
 
   private int[] _numMVEntries;
 
+  private final boolean _isNullHandlingEnabled;
+
   public TransformBlockValSet(ValueBlock valueBlock, TransformFunction transformFunction,
-      ExpressionContext expression) {
+      ExpressionContext expression, boolean isNullHandlingEnabled) {
     _valueBlock = valueBlock;
     _transformFunction = transformFunction;
     _expression = expression;
+    _isNullHandlingEnabled = isNullHandlingEnabled;
   }
 
   @Nullable
   @Override
   public RoaringBitmap getNullBitmap() {

Review Comment:
   What if `getNullBitmap` is called before any of the `getXXValuesSV` is called?



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