61yao commented on code in PR #10594:
URL: https://github.com/apache/pinot/pull/10594#discussion_r1178576003


##########
pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/CoalesceTransformFunctionTest.java:
##########


Review Comment:
   good point. and it actually catches a bug.. 



##########
pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/CoalesceTransformFunctionTest.java:
##########
@@ -18,456 +18,78 @@
  */
 package org.apache.pinot.core.operator.transform.function;
 
-import java.io.File;
-import java.math.BigDecimal;
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Random;
-import java.util.Set;
-import org.apache.commons.io.FileUtils;
-import org.apache.pinot.common.request.context.ExpressionContext;
 import org.apache.pinot.common.request.context.RequestContextUtils;
-import org.apache.pinot.core.operator.DocIdSetOperator;
-import org.apache.pinot.core.operator.ProjectionOperator;
-import org.apache.pinot.core.operator.blocks.ProjectionBlock;
-import org.apache.pinot.core.operator.filter.MatchAllFilterOperator;
-import org.apache.pinot.core.plan.DocIdSetPlanNode;
-import 
org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentLoader;
-import 
org.apache.pinot.segment.local.segment.creator.impl.SegmentIndexCreationDriverImpl;
-import org.apache.pinot.segment.local.segment.readers.GenericRowRecordReader;
-import org.apache.pinot.segment.spi.IndexSegment;
-import org.apache.pinot.segment.spi.creator.SegmentGeneratorConfig;
-import org.apache.pinot.segment.spi.datasource.DataSource;
-import org.apache.pinot.spi.config.table.TableConfig;
-import org.apache.pinot.spi.config.table.TableType;
-import org.apache.pinot.spi.data.FieldSpec;
-import org.apache.pinot.spi.data.Schema;
-import org.apache.pinot.spi.data.readers.GenericRow;
-import org.apache.pinot.spi.utils.ReadMode;
-import org.apache.pinot.spi.utils.builder.TableConfigBuilder;
+import org.roaringbitmap.RoaringBitmap;
 import org.testng.Assert;
-import org.testng.annotations.BeforeClass;
 import org.testng.annotations.Test;
 
 
 public class CoalesceTransformFunctionTest extends BaseTransformFunctionTest {
-  private static final String ENABLE_NULL_SEGMENT_NAME = "testSegment1";
-  private static final String DISABLE_NULL_SEGMENT_NAME = "testSegment2";
-  private static final Random RANDOM = new Random();
-
-  private static final int NUM_ROWS = 1000;
-  private static final String INT_SV_COLUMN1 = "intSV1";
-  private static final String INT_SV_COLUMN2 = "intSV2";
-  private static final String STRING_SV_COLUMN1 = "StringSV1";
-  private static final String STRING_SV_COLUMN2 = "StringSV2";
-  private static final String BIG_DECIMAL_SV_COLUMN1 = "BigDecimalSV1";
-  private static final String BIG_DECIMAL_SV_COLUMN2 = "BigDecimalSV2";
-  private static final String LONG_SV_COLUMN1 = "LongSV1";
-  private static final String LONG_SV_COLUMN2 = "LongSV2";
-  private static final String DOUBLE_SV_COLUMN1 = "DoubleSV1";
-  private static final String DOUBLE_SV_COLUMN2 = "DoubleSV2";
-
-  private static final String FLOAT_SV_COLUMN1 = "FloatSV1";
-  private static final String FLOAT_SV_COLUMN2 = "FLoatSV2";
-  private final int[] _intSVValues = new int[NUM_ROWS];
-  private final double[] _doubleValues = new double[NUM_ROWS];
-  private final float[] _floatValues = new float[NUM_ROWS];
-  private final String[] _stringSVValues = new String[NUM_ROWS];
-  private Map<String, DataSource> _enableNullDataSourceMap;
-  private Map<String, DataSource> _disableNullDataSourceMap;
-  private ProjectionBlock _enableNullProjectionBlock;
-  private ProjectionBlock _disableNullProjectionBlock;
-  // Mod decides whether the first column of the same type should be null.
-  private static final int NULL_MOD1 = 3;
-  // Mod decides whether the second column of the same type should be null.
-  private static final int NULL_MOD2 = 5;
-  // Difference between two same type numeric columns.
-  private static final int INT_VALUE_SHIFT = 2;
-  private static final double DOUBLE_VALUE_SHIFT = 0.1;
-  private static final float FLOAT_VALUE_SHIFT = 0.1f;
-
-  // Suffix for second string column.
-  private static final String SUFFIX = "column2";
-
-  private static String getIndexDirPath(String segmentName) {
-    return FileUtils.getTempDirectoryPath() + File.separator + segmentName;
-  }
-
-  private static Map<String, DataSource> getDataSourceMap(Schema schema, 
List<GenericRow> rows, String segmentName)
-      throws Exception {
-    TableConfig tableConfig =
-        new 
TableConfigBuilder(TableType.OFFLINE).setTableName(segmentName).setNullHandlingEnabled(true).build();
-    SegmentGeneratorConfig config = new SegmentGeneratorConfig(tableConfig, 
schema);
-    config.setOutDir(getIndexDirPath(segmentName));
-    config.setSegmentName(segmentName);
-    SegmentIndexCreationDriverImpl driver = new 
SegmentIndexCreationDriverImpl();
-    driver.init(config, new GenericRowRecordReader(rows));
-    driver.build();
-    IndexSegment indexSegment =
-        ImmutableSegmentLoader.load(new File(getIndexDirPath(segmentName), 
segmentName), ReadMode.heap);
-    Set<String> columnNames = indexSegment.getPhysicalColumnNames();
-    Map<String, DataSource> enableNullDataSourceMap = new 
HashMap<>(columnNames.size());
-    for (String columnName : columnNames) {
-      enableNullDataSourceMap.put(columnName, 
indexSegment.getDataSource(columnName));
-    }
-    return enableNullDataSourceMap;
-  }
-
-  private static ProjectionBlock getProjectionBlock(Map<String, DataSource> 
dataSourceMap) {
-    return new ProjectionOperator(dataSourceMap,
-        new DocIdSetOperator(new MatchAllFilterOperator(NUM_ROWS), 
DocIdSetPlanNode.MAX_DOC_PER_CALL)).nextBlock();
-  }
-
-  private static boolean isColumn1Null(int i) {
-    return i % NULL_MOD1 == 0;
-  }
-
-  private static boolean isColumn2Null(int i) {
-    return i % NULL_MOD2 == 0;
-  }
-
-  @BeforeClass
-  public void setup()
-      throws Exception {
-    // Set up two tables: one with null option enable, the other with null 
option disable.
-    // Each table one string column, and one int column with some rows set to 
null.
-    FileUtils.deleteQuietly(new 
File(getIndexDirPath(DISABLE_NULL_SEGMENT_NAME)));
-    FileUtils.deleteQuietly(new 
File(getIndexDirPath(ENABLE_NULL_SEGMENT_NAME)));
-    for (int i = 0; i < NUM_ROWS; i++) {
-      _intSVValues[i] = RANDOM.nextInt();
-      _doubleValues[i] = RANDOM.nextDouble();
-      _floatValues[i] = RANDOM.nextFloat();
-      _stringSVValues[i] = "a" + RANDOM.nextInt();
-    }
-    List<GenericRow> rows = new ArrayList<>(NUM_ROWS);
-    for (int i = 0; i < NUM_ROWS; i++) {
-      Map<String, Object> map = new HashMap<>();
-      map.put(INT_SV_COLUMN1, _intSVValues[i]);
-      map.put(INT_SV_COLUMN2, _intSVValues[i] + INT_VALUE_SHIFT);
-      map.put(DOUBLE_SV_COLUMN1, _doubleValues[i]);
-      map.put(DOUBLE_SV_COLUMN2, _doubleValues[i] + DOUBLE_VALUE_SHIFT);
-      map.put(FLOAT_SV_COLUMN1, _floatValues[i]);
-      map.put(FLOAT_SV_COLUMN2, _floatValues[i] + FLOAT_VALUE_SHIFT);
-      map.put(STRING_SV_COLUMN1, _stringSVValues[i]);
-      map.put(STRING_SV_COLUMN2, _stringSVValues[i] + SUFFIX);
-      map.put(BIG_DECIMAL_SV_COLUMN1, BigDecimal.valueOf(_intSVValues[i]));
-      map.put(BIG_DECIMAL_SV_COLUMN2, BigDecimal.valueOf(_intSVValues[i] + 
INT_VALUE_SHIFT));
-      map.put(LONG_SV_COLUMN1, _intSVValues[i]);
-      map.put(LONG_SV_COLUMN2, _intSVValues[i] + INT_VALUE_SHIFT);
-
-      if (isColumn1Null(i)) {
-        map.put(INT_SV_COLUMN1, null);
-        map.put(STRING_SV_COLUMN1, null);
-        map.put(BIG_DECIMAL_SV_COLUMN1, null);
-        map.put(LONG_SV_COLUMN1, null);
-        map.put(DOUBLE_SV_COLUMN1, null);
-        map.put(FLOAT_SV_COLUMN1, null);
-      }
-      if (isColumn2Null(i)) {
-        map.put(INT_SV_COLUMN2, null);
-        map.put(STRING_SV_COLUMN2, null);
-        map.put(LONG_SV_COLUMN2, null);
-        map.put(BIG_DECIMAL_SV_COLUMN2, null);
-        map.put(DOUBLE_SV_COLUMN2, null);
-        map.put(FLOAT_SV_COLUMN2, null);
-      }
-      GenericRow row = new GenericRow();
-      row.init(map);
-      rows.add(row);
-    }
-    Schema schema = new 
Schema.SchemaBuilder().addSingleValueDimension(INT_SV_COLUMN1, 
FieldSpec.DataType.INT)
-        .addSingleValueDimension(INT_SV_COLUMN2, FieldSpec.DataType.INT)
-        .addSingleValueDimension(STRING_SV_COLUMN1, FieldSpec.DataType.STRING)
-        .addSingleValueDimension(STRING_SV_COLUMN2, FieldSpec.DataType.STRING)
-        .addSingleValueDimension(LONG_SV_COLUMN1, FieldSpec.DataType.LONG)
-        .addSingleValueDimension(LONG_SV_COLUMN2, FieldSpec.DataType.LONG)
-        .addSingleValueDimension(DOUBLE_SV_COLUMN1, FieldSpec.DataType.DOUBLE)
-        .addSingleValueDimension(DOUBLE_SV_COLUMN2, FieldSpec.DataType.DOUBLE)
-        .addSingleValueDimension(FLOAT_SV_COLUMN1, FieldSpec.DataType.FLOAT)
-        .addSingleValueDimension(FLOAT_SV_COLUMN2, FieldSpec.DataType.FLOAT)
-        .addMetric(BIG_DECIMAL_SV_COLUMN1, FieldSpec.DataType.BIG_DECIMAL)
-        .addMetric(BIG_DECIMAL_SV_COLUMN2, 
FieldSpec.DataType.BIG_DECIMAL).build();
-    _enableNullDataSourceMap = getDataSourceMap(schema, rows, 
ENABLE_NULL_SEGMENT_NAME);
-    _enableNullProjectionBlock = getProjectionBlock(_enableNullDataSourceMap);
-    _disableNullDataSourceMap = getDataSourceMap(schema, rows, 
DISABLE_NULL_SEGMENT_NAME);
-    _disableNullProjectionBlock = 
getProjectionBlock(_disableNullDataSourceMap);
-  }
-
-  private static void testIntTransformFunction(ExpressionContext expression, 
int[] expectedValues,
-      ProjectionBlock projectionBlock, Map<String, DataSource> dataSourceMap)
-      throws Exception {
-    int[] actualValues =
-        TransformFunctionFactory.get(expression, 
dataSourceMap).transformToIntValuesSV(projectionBlock);
-    for (int i = 0; i < NUM_ROWS; i++) {
-      Assert.assertEquals(actualValues[i], expectedValues[i]);
-    }
-  }
-
-  private static void testStringTransformFunction(ExpressionContext 
expression, String[] expectedValues,
-      ProjectionBlock projectionBlock, Map<String, DataSource> dataSourceMap)
-      throws Exception {
-    String[] actualValues =
-        TransformFunctionFactory.get(expression, 
dataSourceMap).transformToStringValuesSV(projectionBlock);
-    for (int i = 0; i < NUM_ROWS; i++) {
-      Assert.assertEquals(actualValues[i], expectedValues[i]);
-    }
-  }
-
-  private static void testLongTransformFunction(ExpressionContext expression, 
long[] expectedValues,
-      ProjectionBlock projectionBlock, Map<String, DataSource> dataSourceMap)
-      throws Exception {
-    long[] actualValues =
-        TransformFunctionFactory.get(expression, 
dataSourceMap).transformToLongValuesSV(projectionBlock);
-    for (int i = 0; i < NUM_ROWS; i++) {
-      Assert.assertEquals(actualValues[i], expectedValues[i]);
-    }
-  }
-
-  private static void testDoubleTransformFunction(ExpressionContext 
expression, double[] expectedValues,
-      ProjectionBlock projectionBlock, Map<String, DataSource> dataSourceMap)
-      throws Exception {
-    double[] actualValues =
-        TransformFunctionFactory.get(expression, 
dataSourceMap).transformToDoubleValuesSV(projectionBlock);
-    for (int i = 0; i < NUM_ROWS; i++) {
-      Assert.assertEquals(actualValues[i], expectedValues[i]);
-    }
-  }
-
-  private static void testFloatTransformFunction(ExpressionContext expression, 
float[] expectedValues,
-      ProjectionBlock projectionBlock, Map<String, DataSource> dataSourceMap)
-      throws Exception {
-    float[] actualValues =
-        TransformFunctionFactory.get(expression, 
dataSourceMap).transformToFloatValuesSV(projectionBlock);
-    for (int i = 0; i < NUM_ROWS; i++) {
-      Assert.assertEquals(actualValues[i], expectedValues[i]);
-    }
-  }
-
-  private static void testBigDecimalTransformFunction(ExpressionContext 
expression, BigDecimal[] expectedValues,
-      ProjectionBlock projectionBlock, Map<String, DataSource> dataSourceMap)
-      throws Exception {
-    BigDecimal[] actualValues =
-        TransformFunctionFactory.get(expression, 
dataSourceMap).transformToBigDecimalValuesSV(projectionBlock);
-    for (int i = 0; i < NUM_ROWS; i++) {
-      Assert.assertEquals(actualValues[i], expectedValues[i]);
-    }
-  }
-
-  // Test the Coalesce on two Int columns where one or the other or both can 
be null.
   @Test
-  public void testCoalesceIntColumns()
-      throws Exception {
-    ExpressionContext coalesceExpr =
-        RequestContextUtils.getExpression(String.format("COALESCE(%s,%s)", 
INT_SV_COLUMN1, INT_SV_COLUMN2));
-    TransformFunction coalesceTransformFunction = 
TransformFunctionFactory.get(coalesceExpr, _enableNullDataSourceMap);
-    Assert.assertEquals(coalesceTransformFunction.getName(), "coalesce");
-    int[] expectedResults = new int[NUM_ROWS];
-    for (int i = 0; i < NUM_ROWS; i++) {
-      if (isColumn1Null(i) && isColumn2Null(i)) {
-        expectedResults[i] = CoalesceTransformFunction.NULL_INT;
-      } else if (isColumn1Null(i)) {
-        expectedResults[i] = _intSVValues[i] + INT_VALUE_SHIFT;
-      } else if (isColumn2Null(i)) {
-        expectedResults[i] = _intSVValues[i];
-      } else {
-        expectedResults[i] = _intSVValues[i];
-      }
-    }
-    testIntTransformFunction(coalesceExpr, expectedResults, 
_enableNullProjectionBlock, _enableNullDataSourceMap);
-    testIntTransformFunction(coalesceExpr, expectedResults, 
_disableNullProjectionBlock, _disableNullDataSourceMap);
-  }
+  public void testCoalesceIntColumns() {
+    TransformFunction coalesceFunc = TransformFunctionFactory.get(
+        RequestContextUtils.getExpression(String.format("COALESCE(%s,%s)", 
INT_SV_NULL_COLUMN, LONG_SV_COLUMN)),
+        _dataSourceMap);
 
-  // Test the Coalesce on two long columns where one or the other or both can 
be null.
-  @Test
-  public void testCoalesceLongColumns()
-      throws Exception {
-    ExpressionContext coalesceExpr =
-        RequestContextUtils.getExpression(String.format("COALESCE(%s,%s)", 
LONG_SV_COLUMN1, LONG_SV_COLUMN2));
-    TransformFunction coalesceTransformFunction = 
TransformFunctionFactory.get(coalesceExpr, _enableNullDataSourceMap);
-    Assert.assertEquals(coalesceTransformFunction.getName(), "coalesce");
     long[] expectedResults = new long[NUM_ROWS];
     for (int i = 0; i < NUM_ROWS; i++) {
-      if (isColumn1Null(i) && isColumn2Null(i)) {
-        expectedResults[i] = CoalesceTransformFunction.NULL_LONG;
-      } else if (isColumn1Null(i)) {
-        expectedResults[i] = _intSVValues[i] + INT_VALUE_SHIFT;
-      } else if (isColumn2Null(i)) {
-        expectedResults[i] = _intSVValues[i];
-      } else {
+      if (i % 2 == 0) {
         expectedResults[i] = _intSVValues[i];
-      }
-    }
-    testLongTransformFunction(coalesceExpr, expectedResults, 
_enableNullProjectionBlock, _enableNullDataSourceMap);
-    testLongTransformFunction(coalesceExpr, expectedResults, 
_disableNullProjectionBlock, _disableNullDataSourceMap);
-  }
-
-  // Test the Coalesce on two float columns where one or the other or both can 
be null.
-  @Test
-  public void testCoalesceFloatColumns()
-      throws Exception {
-    ExpressionContext coalesceExpr =
-        RequestContextUtils.getExpression(String.format("COALESCE(%s,%s)", 
FLOAT_SV_COLUMN1, FLOAT_SV_COLUMN2));
-    TransformFunction coalesceTransformFunction = 
TransformFunctionFactory.get(coalesceExpr, _enableNullDataSourceMap);
-    Assert.assertEquals(coalesceTransformFunction.getName(), "coalesce");
-    float[] expectedResults = new float[NUM_ROWS];
-    for (int i = 0; i < NUM_ROWS; i++) {
-      if (isColumn1Null(i) && isColumn2Null(i)) {
-        expectedResults[i] = CoalesceTransformFunction.NULL_FLOAT;
-      } else if (isColumn1Null(i)) {
-        expectedResults[i] = _floatValues[i] + FLOAT_VALUE_SHIFT;
-      } else if (isColumn2Null(i)) {
-        expectedResults[i] = _floatValues[i];
-      } else {
-        expectedResults[i] = _floatValues[i];
-      }
-    }
-    testFloatTransformFunction(coalesceExpr, expectedResults, 
_enableNullProjectionBlock, _enableNullDataSourceMap);
-    testFloatTransformFunction(coalesceExpr, expectedResults, 
_disableNullProjectionBlock, _disableNullDataSourceMap);
-  }
-
-  // Test the Coalesce on two double columns where one or the other or both 
can be null.
-  @Test
-  public void testCoalesceDoubleColumns()
-      throws Exception {
-    ExpressionContext coalesceExpr =
-        RequestContextUtils.getExpression(String.format("COALESCE(%s,%s)", 
DOUBLE_SV_COLUMN1, DOUBLE_SV_COLUMN2));
-    TransformFunction coalesceTransformFunction = 
TransformFunctionFactory.get(coalesceExpr, _enableNullDataSourceMap);
-    Assert.assertEquals(coalesceTransformFunction.getName(), "coalesce");
-    double[] expectedResults = new double[NUM_ROWS];
-    for (int i = 0; i < NUM_ROWS; i++) {
-      if (isColumn1Null(i) && isColumn2Null(i)) {
-        expectedResults[i] = CoalesceTransformFunction.NULL_DOUBLE;
-      } else if (isColumn1Null(i)) {
-        expectedResults[i] = _doubleValues[i] + DOUBLE_VALUE_SHIFT;
-      } else if (isColumn2Null(i)) {
-        expectedResults[i] = _doubleValues[i];
       } else {
-        expectedResults[i] = _doubleValues[i];
+        expectedResults[i] = _longSVValues[i];
       }
     }
-    testDoubleTransformFunction(coalesceExpr, expectedResults, 
_enableNullProjectionBlock, _enableNullDataSourceMap);
-    testDoubleTransformFunction(coalesceExpr, expectedResults, 
_disableNullProjectionBlock, _disableNullDataSourceMap);
+    testTransformFunction(coalesceFunc, expectedResults);
   }
 
-  // Test the Coalesce on two big decimal columns where one or the other or 
both can be null.
   @Test
-  public void testCoalesceBigDecimalColumns()
-      throws Exception {
-    ExpressionContext coalesceExpr = RequestContextUtils.getExpression(
-        String.format("COALESCE(%s,%s)", BIG_DECIMAL_SV_COLUMN1, 
BIG_DECIMAL_SV_COLUMN2));
-    TransformFunction coalesceTransformFunction = 
TransformFunctionFactory.get(coalesceExpr, _enableNullDataSourceMap);
-    Assert.assertEquals(coalesceTransformFunction.getName(), "coalesce");
-    BigDecimal[] expectedResults = new BigDecimal[NUM_ROWS];
+  public void testCoalesceIntColumnsAndLiterals() {
+    TransformFunction coalesceFunc = TransformFunctionFactory.get(
+        RequestContextUtils.getExpression(String.format("COALESCE(%s,%s)", 
INT_SV_NULL_COLUMN, 314)),

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.

To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


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

Reply via email to