Repository: polarsignals/frostdb
Pull request #509 (2023-08-30 11:32:04)

*: handle column chunks with contained nulls in BinaryScalarOperation
Previously, nulls were only handled for the logicalplan.OpEq operator. There were cases where the presence of nulls in the application of other operators (e.g. logicalplan.OpGt) would lead to an unsupported value panic in the comparison function. Indeed, null comparisons are undefined according to SQL semantics so the safest bet right now is to simple have null values pass the check and delegate the filtering to the execution engine.

There is probably some optimizations we can do here once we define the semantics we want, but this commit opts for safety first as a first step.

Closes https://github.com/polarsignals/polarsignals/issues/2508


Patch:
diff --git a/binaryscalarexpr.go b/binaryscalarexpr.go
index 588f260..b438be3 100644
--- a/binaryscalarexpr.go
+++ b/binaryscalarexpr.go
@@ -81,14 +81,15 @@ var ErrUnsupportedBinaryOperation = errors.New("unsupported binary operation")
 // returns false, it means that the operator will definitely not be satisfied
 // by any value in the column chunk.
 func BinaryScalarOperation(left parquet.ColumnChunk, right parquet.Value, operator logicalplan.Op) (bool, error) {
-	switch operator {
-	case logicalplan.OpEq:
-		numNulls := NullCount(left)
+	numNulls := NullCount(left)
+	fullOfNulls := numNulls == left.NumValues()
+	if operator == logicalplan.OpEq {
 		if right.IsNull() {
 			return numNulls > 0, nil
 		}
-		if numNulls == left.NumValues() {
-			// No non-null values, so there is definitely not a match.
+		if fullOfNulls {
+			// Right value is not null and there are no non-null values, so
+			// there is definitely not a match.
 			return false, nil
 		}
 
@@ -109,6 +110,19 @@ func BinaryScalarOperation(left parquet.ColumnChunk, right parquet.Value, operat
 		}
 
 		return true, nil
+	}
+
+	// The following operators are not optimized. If there is at least one null
+	// in the page, return true for safety since min/max values are not
+	// guaranteed to be set correctly. We can definitely improve this in the
+	// future for better performance. This is not a correctness issue because
+	// the execution engine will take care of correctly applying the operator.
+	// This applies if the right value is null as well, since it cannot be
+	// compared.
+	if numNulls > 0 || right.IsNull() {
+		return true, nil
+	}
+	switch operator {
 	case logicalplan.OpLtEq:
 		return compare(Min(left), right) <= 0, nil
 	case logicalplan.OpLt:


Test patch:
diff --git a/binaryscalarexpr_test.go b/binaryscalarexpr_test.go
index e7bab10..f43f276 100644
--- a/binaryscalarexpr_test.go
+++ b/binaryscalarexpr_test.go
@@ -57,93 +57,132 @@ func Test_MinMax_EmptyColumnChunk(t *testing.T) {
 }
 
 func TestBinaryScalarOperation(t *testing.T) {
+	const numValues = 10
 	for _, tc := range []struct {
 		name string
 		min  int
 		max  int
 		// -1 is interpreted as a null value.
-		right       int
-		nullCount   int64
-		op          logicalplan.Op
-		expectFalse bool
+		right     int
+		nullCount int64
+		op        logicalplan.Op
+		// expectSatisfies is true if the predicate should be satisfied by the
+		// column chunk.
+		expectSatisfies bool
 	}{
 		{
-			name:  "OpEqValueContained",
-			min:   1,
-			max:   10,
-			right: 5,
-			op:    logicalplan.OpEq,
+			name:            "OpEqValueContained",
+			min:             1,
+			max:             10,
+			right:           5,
+			op:              logicalplan.OpEq,
+			expectSatisfies: true,
 		},
 		{
-			name:        "OpEqValueGt",
-			min:         1,
-			max:         10,
-			right:       11,
-			op:          logicalplan.OpEq,
-			expectFalse: true,
+			name:            "OpEqValueGt",
+			min:             1,
+			max:             10,
+			right:           11,
+			op:              logicalplan.OpEq,
+			expectSatisfies: false,
 		},
 		{
-			name:        "OpEqValueLt",
-			min:         1,
-			max:         10,
-			right:       0,
-			op:          logicalplan.OpEq,
-			expectFalse: true,
+			name:            "OpEqValueLt",
+			min:             1,
+			max:             10,
+			right:           0,
+			op:              logicalplan.OpEq,
+			expectSatisfies: false,
 		},
 		{
-			name:  "OpEqMaxBound",
-			min:   1,
-			max:   10,
-			right: 10,
-			op:    logicalplan.OpEq,
+			name:            "OpEqMaxBound",
+			min:             1,
+			max:             10,
+			right:           10,
+			op:              logicalplan.OpEq,
+			expectSatisfies: true,
 		},
 		{
-			name:  "OpEqMinBound",
-			min:   1,
-			max:   10,
-			right: 1,
-			op:    logicalplan.OpEq,
+			name:            "OpEqMinBound",
+			min:             1,
+			max:             10,
+			right:           1,
+			op:              logicalplan.OpEq,
+			expectSatisfies: true,
 		},
 		{
-			name:        "OpEqNullValueNoMatch",
-			right:       -1,
-			nullCount:   0,
-			op:          logicalplan.OpEq,
-			expectFalse: true,
+			name:            "OpEqNullValueNoMatch",
+			right:           -1,
+			nullCount:       0,
+			op:              logicalplan.OpEq,
+			expectSatisfies: false,
 		},
 		{
-			name:      "OpEqNullValueMatch",
-			right:     -1,
-			nullCount: 1,
-			op:        logicalplan.OpEq,
+			name:            "OpEqNullValueMatch",
+			right:           -1,
+			nullCount:       1,
+			op:              logicalplan.OpEq,
+			expectSatisfies: true,
+		},
+		{
+			name:            "OpEqNullColumn",
+			right:           1,
+			nullCount:       1,
+			op:              logicalplan.OpEq,
+			expectSatisfies: false,
+		},
+		{
+			name:            "OpEqFullNullColumn",
+			right:           1,
+			nullCount:       10,
+			op:              logicalplan.OpEq,
+			expectSatisfies: false,
 		},
 		{
-			name:        "OpEqNullColumn",
-			right:       1,
-			nullCount:   1,
-			op:          logicalplan.OpEq,
-			expectFalse: true,
+			name:      "OpGtFullNullColumn",
+			right:     1,
+			nullCount: 10,
+			op:        logicalplan.OpGt,
+			// expectSatisfies should probably be false once we optimize this.
+			expectSatisfies: true,
 		},
 		{
-			name:        "OpEqFullNullColumn",
-			right:       1,
-			nullCount:   10,
-			op:          logicalplan.OpEq,
-			expectFalse: true,
+			name:            "OpGtNullValueMatch",
+			right:           -1,
+			nullCount:       0,
+			op:              logicalplan.OpGt,
+			expectSatisfies: true,
+		},
+		{
+			name:      "OpGtNullValueNoMatch",
+			right:     -1,
+			nullCount: 1,
+			op:        logicalplan.OpGt,
+			// expectSatisfies should probably be false once we optimize this.
+			expectSatisfies: true,
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			if tc.op == logicalplan.OpUnknown {
 				t.Fatal("test programming error: remember to set operator")
 			}
+			minV := parquet.ValueOf(tc.min)
+			maxV := parquet.ValueOf(tc.max)
+			if tc.nullCount == numValues {
+				// All values in page are null. Parquet doesn't have
+				// well-defined min/max values in this case, but setting them
+				// explicitly to null here will tickle some edge cases.
+				minV = parquet.ValueOf(nil)
+				maxV = parquet.ValueOf(nil)
+			}
 			fakeChunk := &FakeColumnChunk{
 				index: &FakeColumnIndex{
 					numPages:  1,
-					min:       parquet.ValueOf(tc.min),
-					max:       parquet.ValueOf(tc.max),
+					min:       minV,
+					max:       maxV,
 					nullCount: tc.nullCount,
 				},
-				numValues: 10,
+				numValues: numValues,
 			}
 			var v parquet.Value
 			if tc.right == -1 {
@@ -153,7 +192,7 @@ func TestBinaryScalarOperation(t *testing.T) {
 			}
 			res, err := BinaryScalarOperation(fakeChunk, v, tc.op)
 			require.NoError(t, err)
-			require.Equal(t, !tc.expectFalse, res)
+			require.Equal(t, tc.expectSatisfies, res)
 		})
 	}
 }