adding null integration test #9657

walterddr · 2022-10-25T20:46:31Z

null handling was only tested against FLOAT and DOUBLE peviously

null value test for more types
add null integration test

codecov-commenter · 2022-10-25T21:50:30Z

Codecov Report

Merging #9657 (2605368) into master (3de1c54) will decrease coverage by 7.30%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #9657      +/-   ##
============================================
- Coverage     68.63%   61.33%   -7.31%     
- Complexity     4920     5156     +236     
============================================
  Files          1947     1933      -14     
  Lines        104168   103900     -268     
  Branches      15796    15770      -26     
============================================
- Hits          71496    63723    -7773     
- Misses        27556    35365    +7809     
+ Partials       5116     4812     -304

Flag	Coverage Δ
integration1	`25.88% <ø> (+0.01%)`	⬆️
unittests1	`67.43% <ø> (+0.09%)`	⬆️
unittests2	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...pinot/controller/recommender/io/ConfigManager.java	`0.00% <0.00%> (-100.00%)`	⬇️
.../org/apache/pinot/client/AggregationResultSet.java	`0.00% <0.00%> (-100.00%)`	⬇️
...ntroller/recommender/rules/impl/JsonIndexRule.java	`0.00% <0.00%> (-100.00%)`	⬇️
...routing/adaptiveserverselector/HybridSelector.java	`0.00% <0.00%> (-100.00%)`	⬇️
...troller/recommender/io/metadata/FieldMetadata.java	`0.00% <0.00%> (-100.00%)`	⬇️
...outing/adaptiveserverselector/LatencySelector.java	`0.00% <0.00%> (-100.00%)`	⬇️
...roller/recommender/rules/impl/BloomFilterRule.java	`0.00% <0.00%> (-100.00%)`	⬇️
...oller/api/resources/PinotControllerAppConfigs.java	`0.00% <0.00%> (-100.00%)`	⬇️
...ler/recommender/data/generator/BytesGenerator.java	`0.00% <0.00%> (-100.00%)`	⬇️
...er/recommender/io/metadata/SchemaWithMetaData.java	`0.00% <0.00%> (-100.00%)`	⬇️
... and 292 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jackjlli · 2022-10-25T23:07:05Z

pinot-core/src/test/java/org/apache/pinot/queries/NullEnabledQueriesTest.java

        } else if ((int) row[keyColumnIdx] == 1) {
-          assertTrue(Math.abs(((Double) row[0]) - 4 * _sumKey1) < 1e-1);
-          assertTrue(Math.abs(((Double) row[1]) - baseValue.doubleValue()) < 1e-1);
+          assertTrue(Math.abs(((Double) row[0]) - 4 * _sumKey1) < PRECISION);


I saw the value of PRECISION constant is 1 in this PR, isn't it greater than 1e-1?

yeah this was for integer and long in the future. but yes i can make it 1e-1 for now until we add other types

jackjlli · 2022-10-25T23:14:39Z

...-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java

  public void tearDown()
      throws Exception {
+    // Setting data table version back
+    DataTableBuilderFactory.setDataTableVersion(DataTableBuilderFactory.DEFAULT_VERSION);


Do we need to set the following to the setUp method of this integration test class as well?

// Setting data table version to 4 DataTableBuilderFactory.setDataTableVersion(DataTableFactory.VERSION_4);

yeah this is a hotfix, multistage test was already setting it in setup but it wasn't set back in teardown i just happen to notice

walterddr · 2022-10-26T00:45:25Z

pinot-core/src/test/java/org/apache/pinot/queries/NullEnabledQueriesTest.java

+        new Object[]{ColumnDataType.FLOAT, RANDOM.nextFloat(), true},
+        new Object[]{ColumnDataType.DOUBLE, RANDOM.nextDouble(), true},
+        new Object[]{ColumnDataType.FLOAT, RANDOM.nextFloat(), false},
+        new Object[]{ColumnDataType.DOUBLE, RANDOM.nextDouble(), false},


@nizarhejazi i was trying to add additional numeric test types here but the assert logic is a bit hard to generalized. please kindly take a look and see if you have better ideas.

walterddr · 2022-10-26T00:46:18Z

pinot-core/src/test/java/org/apache/pinot/queries/NullEnabledQueriesTest.java

+    return new Object[][]{
+        new Object[]{ColumnDataType.INT, RANDOM.nextInt(), true},
+        new Object[]{ColumnDataType.LONG, RANDOM.nextInt(), true},
+        new Object[]{ColumnDataType.BOOLEAN, RANDOM.nextInt(2), true},
+        new Object[]{ColumnDataType.STRING, RANDOM.nextInt(), true},
+        new Object[]{ColumnDataType.TIMESTAMP, RANDOM.nextInt(), true},
+        new Object[]{ColumnDataType.INT, RANDOM.nextInt(), false},
+        new Object[]{ColumnDataType.LONG, RANDOM.nextInt(), false},
+        new Object[]{ColumnDataType.BOOLEAN, RANDOM.nextInt(2), false},
+        new Object[]{ColumnDataType.STRING, RANDOM.nextInt(), false},
+        new Object[]{ColumnDataType.TIMESTAMP, RANDOM.nextInt(), false},


this list is not at all a full coverage, but good to add STRING BOOLEAN and TIMESTAMP as representatives. can add more later

@walterddr NullHandling support was tested against all data types. Please check the following files:

NullEnabledQueriesTest (Dict and non-dict Float and Double types).

BooleanNullEnabledQueriesTest (Dict and non-dict Boolean type).

BigDecimalQueriesTest (Dict and non-dict BigDecimal type).

AllNullQueriesTest (Dict and non-dict Long, Float, Double, Int, String and BigDecimal types).

Ah I see thanks for the info. I wasn't aware of the other tests

walterddr · 2022-10-26T00:46:43Z

pinot-core/src/test/java/org/apache/pinot/queries/NullEnabledQueriesTest.java

+    // TODO fix String null value handling
+    if (dataType != ColumnDataType.STRING) {


looks like this query doesn't work with STRING type. might be we can take another look @nizarhejazi

@walterddr There is a reason this query does not work w/ String, right? We cannot pass String column to aggregation functions (Min, Max, Avg, etc.) and we cannot use inequality operators w/ Strings.

Oh I see. So we are not using comparison for min/max and null checker for count. But actually fallback to the numeric handling. Is this correct?

nizarhejazi · 2022-10-26T01:39:52Z

@walterddr NullHandling support was tested against all data types. Please check the following files:

NullEnabledQueriesTest (Dict and non-dict Float and Double types).
BooleanNullEnabledQueriesTest (Dict and non-dict Boolean type).
BigDecimalQueriesTest (Dict and non-dict big decimal type).
AllNullQueriesTest (Dict and non-dict Long, Float, Double, Int, String and BigDecimal types).

walterddr · 2022-10-26T03:24:12Z

looks like as @nizarhejazi mentioned the issue is not on null handling (e.g. select out null column) but on function interpretation of null values. closing this and creating a new PR for fixing the issue

adding null integration test

2605368

walterddr marked this pull request as ready for review October 25, 2022 21:55

walterddr mentioned this pull request Oct 25, 2022

Support for nulls python-pinot-dbapi/pinot-dbapi#45

Closed

jackjlli reviewed Oct 25, 2022

View reviewed changes

walterddr commented Oct 26, 2022

View reviewed changes

walterddr closed this Oct 26, 2022

walterddr deleted the null_itcase branch December 6, 2023 16:21

		// TODO fix String null value handling
		if (dataType != ColumnDataType.STRING) {

adding null integration test #9657

adding null integration test #9657

Uh oh!

Conversation

walterddr commented Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nizarhejazi Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nizarhejazi Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nizarhejazi commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

walterddr commented Oct 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

walterddr commented Oct 25, 2022 •

edited

Loading

codecov-commenter commented Oct 25, 2022 •

edited

Loading

nizarhejazi Oct 26, 2022 •

edited

Loading

nizarhejazi Oct 26, 2022 •

edited

Loading

nizarhejazi commented Oct 26, 2022 •

edited

Loading