From af3c4ae30fa83fda107b0df49e1eb6778e6aeee7 Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Sat, 5 Sep 2026 03:25:47 +0800 Subject: [PATCH] [common] Never compare null literals when merging range predicates Between.optimize ordered the two bounds it merges into a BETWEEN with compareLiteral, which throws NullPointerException on a null literal. A Spark procedure where-clause can carry one, since resolveFilter does not run NullPropagation. Leave a pair with a null bound unmerged and let the evaluation layer answer false for it, as LeafBinaryFunction already does, and guard compareLiteral so the next caller gets a message instead of an NPE. --- .../org/apache/paimon/predicate/Between.java | 32 +++++++++++++-- .../apache/paimon/predicate/CompareUtils.java | 5 +++ .../apache/paimon/predicate/BetweenTest.java | 39 +++++++++++++++++++ 3 files changed, 73 insertions(+), 3 deletions(-) diff --git a/paimon-common/src/main/java/org/apache/paimon/predicate/Between.java b/paimon-common/src/main/java/org/apache/paimon/predicate/Between.java index 30d317b22486..dce11132c60a 100644 --- a/paimon-common/src/main/java/org/apache/paimon/predicate/Between.java +++ b/paimon-common/src/main/java/org/apache/paimon/predicate/Between.java @@ -27,6 +27,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import static org.apache.paimon.predicate.CompareUtils.compareLiteral; @@ -133,7 +134,7 @@ private static List mergeLessAndGreaterToBetween( if (leafPredicate.function() == LessOrEqual.INSTANCE) { if (lessOrEqual == null) { lessOrEqual = leafPredicate; - } else { + } else if (!hasNullLiteral(lessOrEqual) && !hasNullLiteral(leafPredicate)) { lessOrEqual = compareLiteral( type, @@ -142,11 +143,14 @@ private static List mergeLessAndGreaterToBetween( < 0 ? lessOrEqual : leafPredicate; + } else { + // A null bound matches nothing; keep the null-bearing predicate. + lessOrEqual = hasNullLiteral(lessOrEqual) ? lessOrEqual : leafPredicate; } } else if (leafPredicate.function() == GreaterOrEqual.INSTANCE) { if (greaterOrEqual == null) { greaterOrEqual = leafPredicate; - } else { + } else if (!hasNullLiteral(greaterOrEqual) && !hasNullLiteral(leafPredicate)) { greaterOrEqual = compareLiteral( type, @@ -155,6 +159,9 @@ private static List mergeLessAndGreaterToBetween( > 0 ? greaterOrEqual : leafPredicate; + } else { + greaterOrEqual = + hasNullLiteral(greaterOrEqual) ? greaterOrEqual : leafPredicate; } } else { result.add(leafPredicate); @@ -166,7 +173,12 @@ private static List mergeLessAndGreaterToBetween( // Determine which is the lower bound and which is the upper bound Object lowerBound = greaterOrEqual.literals().get(0); Object upperBound = lessOrEqual.literals().get(0); - if (compareLiteral(type, lowerBound, upperBound) >= 0) { + if (lowerBound == null || upperBound == null) { + // A null bound makes the conjunction match nothing; never compare + // nulls (SQL null literals are unordered). + result.add(lessOrEqual); + result.add(greaterOrEqual); + } else if (compareLiteral(type, lowerBound, upperBound) >= 0) { // No valid intersection, keep all original predicates result.add(lessOrEqual); result.add(greaterOrEqual); @@ -190,6 +202,10 @@ private static List mergeLessAndGreaterToBetween( return result; } + private static boolean hasNullLiteral(LeafPredicate predicate) { + return predicate.literals().stream().anyMatch(Objects::isNull); + } + private static List mergeMultipleBetweens( FieldTransform field, List predicates) { List results = new ArrayList<>(); @@ -211,10 +227,15 @@ private static List mergeMultipleBetweens( Object maxLower = null; Object minUpper = null; + boolean anyNullLiteral = false; for (LeafPredicate between : betweens) { Object lower = between.literals().get(0); Object upper = between.literals().get(1); + if (lower == null || upper == null) { + anyNullLiteral = true; + continue; + } if (maxLower == null || compareLiteral(fieldType, lower, maxLower) > 0) { maxLower = lower; } @@ -222,6 +243,11 @@ private static List mergeMultipleBetweens( minUpper = upper; } } + if (anyNullLiteral) { + // A null bound makes the conjunction match nothing; leave the predicates + // unmerged instead of comparing nulls. + return predicates; + } // Check if intersection is valid if (maxLower != null diff --git a/paimon-common/src/main/java/org/apache/paimon/predicate/CompareUtils.java b/paimon-common/src/main/java/org/apache/paimon/predicate/CompareUtils.java index 4d7fb056bb44..1d724aafadca 100644 --- a/paimon-common/src/main/java/org/apache/paimon/predicate/CompareUtils.java +++ b/paimon-common/src/main/java/org/apache/paimon/predicate/CompareUtils.java @@ -27,6 +27,11 @@ public class CompareUtils { private CompareUtils() {} public static int compareLiteral(DataType type, Object v1, Object v2) { + // SQL null literals are unordered, so a caller that merges range predicates has to + // handle a null bound rather than ask for its order. + if (v1 == null || v2 == null) { + throw new IllegalArgumentException("Null literal cannot be compared for type: " + type); + } if (v1 instanceof Comparable) { return ((Comparable) v1).compareTo(v2); } else if (v1 instanceof byte[]) { diff --git a/paimon-common/src/test/java/org/apache/paimon/predicate/BetweenTest.java b/paimon-common/src/test/java/org/apache/paimon/predicate/BetweenTest.java index f5692d85f521..20e3966ab28c 100644 --- a/paimon-common/src/test/java/org/apache/paimon/predicate/BetweenTest.java +++ b/paimon-common/src/test/java/org/apache/paimon/predicate/BetweenTest.java @@ -18,6 +18,7 @@ package org.apache.paimon.predicate; +import org.apache.paimon.data.GenericRow; import org.apache.paimon.types.IntType; import org.apache.paimon.types.RowType; @@ -47,6 +48,44 @@ public void testOneLessOrEqualNotRewrite() { assertThat(compoundResult.children().get(0)).isEqualTo(isNotNull); } + @Test + public void testNullLiteralBoundsDoNotCrash() { + PredicateBuilder builder = new PredicateBuilder(RowType.of(new IntType())); + // x <= NULL AND x >= 1: Flink pushdown keeps null literals; optimize() + // previously crashed comparing null (Unsupported type / NPE). + Predicate lteNull = builder.lessOrEqual(0, null); + Predicate gte = builder.greaterOrEqual(0, 1); + Predicate and = PredicateBuilder.and(Arrays.asList(lteNull, gte)); + assertThat(and).isNotNull(); + + // Two <= bounds where one is null: keeps the null-bearing predicate. + Predicate lte10 = builder.lessOrEqual(0, 10); + Predicate andNulls = PredicateBuilder.and(Arrays.asList(lteNull, lte10, gte)); + assertThat(andNulls).isNotNull(); + + // Two >= bounds where one is null. + Predicate gteNull = builder.greaterOrEqual(0, null); + Predicate andGteNulls = + PredicateBuilder.and(Arrays.asList(gteNull, builder.greaterOrEqual(0, 5), lte10)); + assertThat(andGteNulls).isNotNull(); + + // Two BETWEENs where one has a null bound stay unmerged: dropping the + // null-bound BETWEEN would wrongly match rows 1..4. + Predicate betweenNull = builder.between(0, null, 5); + Predicate between = builder.between(0, 1, 4); + Predicate andBetweens = PredicateBuilder.and(Arrays.asList(betweenNull, between)); + assertThat(andBetweens).isInstanceOf(CompoundPredicate.class); + CompoundPredicate compound = (CompoundPredicate) andBetweens; + assertThat(compound.function()).isInstanceOf(And.class); + assertThat(compound.children()).hasSize(2); + + // The null-bearing predicate retained by the merge still evaluates to false. + Predicate merged = PredicateBuilder.and(Arrays.asList(lteNull, gte)); + GenericRow row = new GenericRow(1); + row.setField(0, 3); + assertThat(merged.test(row)).isFalse(); + } + @Test public void testTryRewriteBetweenPredicateBasic() { // Test basic case: AND(a>=1, a<=10, a is not null) should be rewritten to BETWEEN