Jackie-Jiang commented on code in PR #20070:
URL: https://github.com/apache/pinot/pull/20070#discussion_r4499093596
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rex/PinotRexExecutor.java:
##########
@@ -68,14 +71,29 @@ public void reduce(RexBuilder rexBuilder, List<RexNode>
constExps, List<RexNode>
// Calcite treats reduction as all-or-nothing. Delegate the entire batch
if any expression is unsupported, so a
// failure in the fallback cannot leave the batch partially reduced.
for (RexNode expression : constExps) {
- if (!isSupportedStringCast(expression)) {
+ if (!isSupportedStringCast(expression) && !isLiteralArray(expression)
+ && !isNumericToApproximateCast(expression)) {
_fallback.reduce(rexBuilder, constExps, reducedValues);
return;
}
}
List<RexNode> literals = new ArrayList<>(constExps.size());
try {
for (RexNode expression : constExps) {
+ if (isLiteralArray(expression)) {
+ // Already in its final form: the fallback would compile a class per
query just to rebuild it, which takes
+ // hundreds of milliseconds for a query vector of hundreds of
elements
+ literals.add(expression);
+ continue;
+ }
+ if (isNumericToApproximateCast(expression)) {
+ // CAST(<numeric literal> AS DOUBLE/FLOAT/REAL), e.g. every element
of a query vector: round the value the
+ // way the cast would, without generating code
+ // Like the string path, keep a nullable target's type with a cast
+ literals.add(rexBuilder.makeLiteral(new
BigDecimal(approximateValue((RexCall) expression)),
+ expression.getType(), true));
Review Comment:
Passing the computed `double` directly to `makeLiteral` avoids an
unnecessary conversion and preserves negative zero. `new BigDecimal(-0.0)`
loses the sign when a tiny negative exact decimal underflows. For example:
```sql
SELECT ATAN2(CAST(CAST('-1e-50' AS DECIMAL(51,50)) AS REAL), -1)
```
An isolated SQL-planning comparison produces `-3.141592653589793` with the
base executor and `3.141592653589793` with this version. This is a narrow edge
case for vector workloads, so I am treating it as non-blocking. The simpler
construction is:
```java
literals.add(rexBuilder.makeLiteral(approximateValue((RexCall) expression),
expression.getType(), true));
```
A negative-underflow regression case would preserve this behavior.
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rex/PinotRexExecutor.java:
##########
@@ -105,6 +123,50 @@ Function1<DataContext, Object[]> getCast(RexBuilder
builder, RelDataType source,
});
}
+ /// Returns whether the expression casts a non-null exact numeric literal
(integer or DECIMAL) to an approximate type
+ /// (DOUBLE, FLOAT, REAL) and the result is finite. Overflow to infinity is
left to the fallback, which decides how to
+ /// represent it. Approximate sources are left to the fallback too: reading
them as a BigDecimal rounds to 16 digits,
+ /// and NaN or infinity cannot be read that way at all.
+ @VisibleForTesting
+ static boolean isNumericToApproximateCast(RexNode expression) {
+ if (!(expression instanceof RexCall)) {
+ return false;
+ }
+ RexCall call = (RexCall) expression;
+ if (call.getOperator() != SqlStdOperatorTable.CAST ||
call.getOperands().size() != 1
+ || !SqlTypeUtil.isApproximateNumeric(call.getType())) {
+ return false;
+ }
+ RexNode operand = call.getOperands().get(0);
+ return operand instanceof RexLiteral && !((RexLiteral) operand).isNull()
+ && SqlTypeUtil.isExactNumeric(operand.getType()) &&
Double.isFinite(approximateValue(call));
Review Comment:
With the direct-double literal construction suggested above, the
finite-value check can also be removed: Calcite 1.42 can represent the
infinities produced by exact-numeric casts. Keep the exact-source restriction,
but let literal construction handle the converted value. This simplifies the
overflow special case and reduces conversion from three times per supported
expression (both predicate calls plus construction) to once. An isolated
comparison of that simplification against Calcite matched all 30 exact-DECIMAL
cases, including positive/negative overflow and underflow across REAL, FLOAT,
and DOUBLE.
##########
pinot-query-planner/src/test/java/org/apache/pinot/calcite/rex/PinotRexExecutorTest.java:
##########
@@ -278,6 +278,83 @@ public void
testWhitespaceAndInvalidInputsMatchCalcite(String value) {
}
}
+ @DataProvider
+ public Object[][] numericLiterals() {
+ List<Object[]> cases = new ArrayList<>();
+ for (SqlTypeName target : List.of(SqlTypeName.FLOAT, SqlTypeName.REAL,
SqlTypeName.DOUBLE)) {
+ for (String value : new String[]{"0", "1", "-7", "0.1", "-2.5",
"0.123456789123", "3.4028235E38", "1E39",
+ "1E308"}) {
+ cases.add(new Object[]{target, value});
+ }
+ }
+ return cases.toArray(new Object[0][]);
+ }
+
+ @Test(dataProvider = "numericLiterals")
+ public void testNumericToApproximateCastsMatchCalcite(SqlTypeName
targetType, String value) {
+ LiteralRexBuilder builder = new LiteralRexBuilder();
+ RelDataType target = builder.getTypeFactory().createSqlType(targetType);
+ // Large values are DOUBLE literals in SQL, since they do not fit the
exact (DECIMAL) literal precision
+ BigDecimal number = new BigDecimal(value);
+ RexNode literal = value.contains("E") ? builder.makeApproxLiteral(number)
: builder.makeExactLiteral(number);
+ RexNode expression = builder.makeAbstractCast(target, literal, false);
+ List<RexNode> input = List.of(expression);
+ CountingFallback fallback = new CountingFallback(CALCITE_EXECUTOR);
+ List<RexNode> expected = reduce(CALCITE_EXECUTOR, builder, input);
+ List<RexNode> actual = reduce(new PinotRexExecutor(fallback), builder,
input);
+ assertEquals(actual, expected);
+ assertEquals(actual.get(0).getType(), expected.get(0).getType());
+ // Approximate sources, and casts that overflow to infinity, are left to
the fallback
+ double converted = targetType == SqlTypeName.REAL ?
Float.parseFloat(value) : Double.parseDouble(value);
+ assertEquals(fallback._calls.get(), value.contains("E") ||
!Double.isFinite(converted) ? 1 : 0);
+ }
+
+ @Test
+ public void testApproximateSourceCastsUseFallback() {
+ LiteralRexBuilder builder = new LiteralRexBuilder();
+ RelDataType doubleType =
builder.getTypeFactory().createSqlType(SqlTypeName.DOUBLE);
+ // 17 significant digits: rounding to a 16-digit BigDecimal would fold
this to 0.3
+ RexNode literal = builder.makeApproxLiteral(new
BigDecimal("0.30000000000000004"));
+ RexNode expression = builder.makeAbstractCast(doubleType, literal, false);
+ List<RexNode> input = List.of(expression);
+ CountingFallback fallback = new CountingFallback(CALCITE_EXECUTOR);
+ List<RexNode> actual = reduce(new PinotRexExecutor(fallback), builder,
input);
+ assertEquals(fallback._calls.get(), 1);
+ assertEquals(actual, reduce(CALCITE_EXECUTOR, builder, input));
+ }
+
+ @Test
+ public void testNumericToApproximateCastKeepsNullableTargetType() {
+ LiteralRexBuilder builder = new LiteralRexBuilder();
+ RelDataType nullableDouble =
builder.getTypeFactory().createTypeWithNullability(
+ builder.getTypeFactory().createSqlType(SqlTypeName.DOUBLE), true);
+ RexNode expression = builder.makeAbstractCast(nullableDouble,
builder.makeExactLiteral(new BigDecimal("0.5")),
+ false);
+ List<RexNode> input = List.of(expression);
+ List<RexNode> actual = reduce(new PinotRexExecutor(FAILING_FALLBACK),
builder, input);
+ assertEquals(actual.get(0).getType(), expression.getType());
+ assertEquals(actual, reduce(CALCITE_EXECUTOR, builder, input));
+ }
+
+ @Test
+ public void testQueryVectorIsReducedWithoutFallback() {
+ LiteralRexBuilder builder = new LiteralRexBuilder();
+ RelDataType real =
builder.getTypeFactory().createSqlType(SqlTypeName.REAL);
+ List<RexNode> elements = new ArrayList<>();
+ List<RexNode> casts = new ArrayList<>();
+ for (int i = 0; i < 768; i++) {
+ RexNode cast = builder.makeAbstractCast(real,
builder.makeExactLiteral(new BigDecimal("0.0" + i)), false);
+ casts.add(cast);
+ elements.add(reduce(CALCITE_EXECUTOR, builder, List.of(cast)).get(0));
Review Comment:
Build the full `casts` list first, then reduce the reference batch once:
```java
List<RexNode> elements = reduce(CALCITE_EXECUTOR, builder, casts);
```
This preserves the 768-element coverage and equality assertions while
avoiding 768 separate Janino compilations during the test. An isolated batch
probe produced all 768 literals and matched the optimized executor.
##########
pinot-query-planner/src/test/java/org/apache/pinot/calcite/rex/PinotRexExecutorTest.java:
##########
@@ -278,6 +278,83 @@ public void
testWhitespaceAndInvalidInputsMatchCalcite(String value) {
}
}
+ @DataProvider
+ public Object[][] numericLiterals() {
+ List<Object[]> cases = new ArrayList<>();
+ for (SqlTypeName target : List.of(SqlTypeName.FLOAT, SqlTypeName.REAL,
SqlTypeName.DOUBLE)) {
+ for (String value : new String[]{"0", "1", "-7", "0.1", "-2.5",
"0.123456789123", "3.4028235E38", "1E39",
+ "1E308"}) {
+ cases.add(new Object[]{target, value});
+ }
+ }
+ return cases.toArray(new Object[0][]);
+ }
+
+ @Test(dataProvider = "numericLiterals")
+ public void testNumericToApproximateCastsMatchCalcite(SqlTypeName
targetType, String value) {
+ LiteralRexBuilder builder = new LiteralRexBuilder();
+ RelDataType target = builder.getTypeFactory().createSqlType(targetType);
+ // Large values are DOUBLE literals in SQL, since they do not fit the
exact (DECIMAL) literal precision
+ BigDecimal number = new BigDecimal(value);
+ RexNode literal = value.contains("E") ? builder.makeApproxLiteral(number)
: builder.makeExactLiteral(number);
Review Comment:
Make the source literal kind explicit in the test data instead of selecting
it with `value.contains("E")`. All the overflow-sized cases currently become
approximate sources, so they take the approximate-source fallback before
exercising the exact-source overflow behavior. Include explicitly typed DECIMAL
sources such as `1E39 -> REAL` and `1E400 -> DOUBLE/FLOAT`, plus
negative-underflow cases. Use `makeExactLiteral(value, decimalType)` for those
large exact values. The comment about magnitude is also misleading: these
values fit Pinot's DECIMAL precision limit; exponent notation is what makes the
SQL literal approximate.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
