xiangfu0 commented on code in PR #19948:
URL: https://github.com/apache/pinot/pull/19948#discussion_r4344123273
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotEvaluateLiteralRule.java:
##########
@@ -386,22 +393,44 @@ private static boolean isUnsignedIntegerType(SqlTypeName
sqlTypeName) {
}
}
+ /// Returns the value that the runtime passes to a function for the literal
(see `FunctionOperand`): the value from
+ /// [RexExpressionUtils#fromRexLiteral], converted with
[ColumnDataType#toExternal]. For example, an INTEGER literal
+ /// becomes an `Integer` (Calcite stores a `BigDecimal`), and a TIMESTAMP
literal becomes a `Timestamp`.
@Nullable
- private static Object getLiteralValue(RexLiteral rexLiteral) {
- Object value = rexLiteral.getValue();
- if (value instanceof NlsString) {
- // STRING
- return ((NlsString) value).getValue();
- } else if (value instanceof GregorianCalendar) {
- // TIMESTAMP
- return ((GregorianCalendar) value).getTimeInMillis();
- } else if (value instanceof ByteString) {
- // BYTES
- return ((ByteString) value).getBytes();
- } else if (value instanceof TimeUnitRange) {
- return ((TimeUnitRange) value).name();
- } else {
- return value;
+ private static Object getArgumentValue(RexLiteral rexLiteral, ColumnDataType
argumentType) {
+ if (argumentType == ColumnDataType.OBJECT && rexLiteral.getTypeName() !=
SqlTypeName.SYMBOL) {
+ // The runtime cannot represent this literal (for example an INTERVAL),
so keep the value that Calcite stores
+ return rexLiteral.getValue();
+ }
+ if (canExceedArgumentType(rexLiteral, argumentType)) {
+ // Keep the exact value, which the conversion to INT or LONG can wrap
around
+ return rexLiteral.getValue();
+ }
+ RexExpression.Literal literal =
RexExpressionUtils.fromRexLiteral(rexLiteral);
+ Object value = literal.getValue();
+ return value != null ? literal.getDataType().toExternal(value) : null;
+ }
+
+ /// Returns whether the literal is a DECIMAL whose type can hold numbers
beyond the range of its INT or LONG argument
+ /// type. The planner gives DECIMAL(p, 0) the type INT or LONG by its
precision only, for example INT to
+ /// DECIMAL(10, 0), and LONG to an integer literal beyond the BIGINT range.
The decision depends on the type, not on
+ /// the value, so that the literals of one type (for example the elements of
an array) get the same Java class.
+ private static boolean canExceedArgumentType(RexLiteral rexLiteral,
ColumnDataType argumentType) {
Review Comment:
Minor / tradeoff worth a note: because this keeps the raw `BigDecimal` for
any scale-0 DECIMAL whose *type* exceeds the target range, a value that
actually fits still diverges from the runtime. E.g. `toThetaSketch(CAST(5 AS
DECIMAL(10,0)))` folds the `BigDecimal` (sketched as the string "5"), while the
same `5` as a plain INT literal sketches the number — the exact class of
mismatch this PR fixes.
I think keeping the exact value is the right choice (avoiding wraparound
matters more, and a value-aware check would break the array-uniformity
invariant you rely on), so no change needed — just flagging it so the narrow
re-exposure is a conscious decision. A one-line comment here noting "value may
still differ from runtime for a fitting value, but never wraps" would help the
next reader.
##########
pinot-query-runtime/src/test/resources/queries/LiteralExpressions.json:
##########
@@ -36,5 +36,38 @@
]
}
]
+ },
+ "literal_function_evaluation": {
+ "comments": "The planner evaluates a function call on literals. It must
pass the literals as the Java values that the runtime passes for the same
values, e.g. an Integer for an INT literal rather than the BigDecimal that
Calcite stores, or functions that dispatch on the value class return other
results. Each query compares a call on a literal with the same call on column
values, which runs above the leaf stage because of the GROUP BY.",
+ "tables": {
+ "tbl": {
+ "schema": [
+ {"name": "intCol", "type": "INT"},
+ {"name": "longCol", "type": "LONG"},
+ {"name": "doubleCol", "type": "DOUBLE"}
+ ],
+ "inputs": [
+ [1, 10000000001, 1.25],
+ [5, 10000000005, 1.5]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "description": "Sketches of an INT literal",
+ "sql": "SELECT intCol, toBase64(toThetaSketch(5)) =
toBase64(toThetaSketch(intCol)), toBase64(toHLL(5)) = toBase64(toHLL(intCol)),
toBase64(toCpcSketch(5)) = toBase64(toCpcSketch(intCol)), toBase64(toULL(5)) =
toBase64(toULL(intCol)), toBase64(toIntegerSumTupleSketch(5, 1)) =
toBase64(toIntegerSumTupleSketch(intCol, 1)) FROM (SELECT intCol FROM {tbl}
GROUP BY intCol) t",
+ "h2Sql": "SELECT intCol, intCol = 5, intCol = 5, intCol = 5, intCol =
5, intCol = 5 FROM (SELECT intCol FROM {tbl} GROUP BY intCol) t"
+ },
+ {
+ "description": "Sketch of a BIGINT literal",
+ "sql": "SELECT longCol, toBase64(toThetaSketch(10000000005)) =
toBase64(toThetaSketch(longCol)) FROM (SELECT longCol FROM {tbl} GROUP BY
longCol) t",
+ "h2Sql": "SELECT longCol, longCol = 10000000005 FROM (SELECT longCol
FROM {tbl} GROUP BY longCol) t"
+ },
+ {
+ "description": "Sketch of a DECIMAL literal, which the runtime reads
as a DOUBLE",
Review Comment:
Minor test-coverage gap: the PR description calls out two observable
behavior changes that don't have a dedicated regression test —
`jsonFormat(1.50)` now yielding `'1.5'` (scale stripped, since the arg arrives
as a `Double`), and `toThetaSketch(TIMESTAMP '...')` now failing at plan time.
The sketch-parity tests here are great, but a small case pinning the
`1.50`->`1.5` output (and maybe asserting the TIMESTAMP-sketch error) would
guard those documented changes against silent regressions.
--
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]
