github-actions[bot] commented on code in PR #67209:
URL: https://github.com/apache/doris/pull/67209#discussion_r3872561398
##########
fe/fe-connector/fe-connector-trino/src/main/java/org/apache/doris/connector/trino/TrinoPredicateConverter.java:
##########
@@ -305,4 +306,20 @@ private BigDecimal toBigDecimal(Object value) {
}
return new BigDecimal(String.valueOf(value));
}
+
+ private Object convertStringLiteralValue(ConnectorLiteral literal) {
+ if (literal.isNull()) {
+ return null;
Review Comment:
[P1] Preserve CAST semantics for null checks too
This early return bypasses the literal-type compatibility check. Because
`ExprToConnectorExpressionConverter` unwraps `CastExpr`, `CAST(c_str AS
DATETIME) <=> NULL` reaches this code as the raw VARCHAR column plus a null
literal, and the `EQ_FOR_NULL` branch pushes `Domain.onlyNull(VARCHAR)`. The
parallel `CAST(c_str AS DATETIME) IS NULL` path is likewise unwrapped and
`convertIsNull()` pushes the same raw domain. Both are narrower than Doris
semantics: malformed non-null strings such as `'a'` cast to a NULL DATETIME, so
those rows satisfy the null check but remote raw-column `IS NULL` removes them
before BE can re-evaluate. The new null test models only `c_str <=> NULL`,
where this domain is valid. Please preserve enough cast/type information to
reject both casted null-check forms and add a malformed non-null string case.
##########
fe/fe-connector/fe-connector-trino/src/main/java/org/apache/doris/connector/trino/TrinoPredicateConverter.java:
##########
@@ -277,7 +278,7 @@ private Object convertLiteralValue(Type type,
ConnectorExpression expr) {
case "CharType":
case "VarbinaryType":
case "VarcharType":
- return Slices.utf8Slice(String.valueOf(value));
+ return convertStringLiteralValue(literal);
case "DateType": {
Review Comment:
[P1] Apply compatibility checks to every raw target type
This check protects only the string-like target branch, but
`ExprToConnectorExpressionConverter` erases every `CastExpr`. Other target
branches can still reinterpret the analyzed literal according to the raw source
type and push a narrowing domain. For example, `CAST(d AS BIGINT) = 20200101`
on a Trino DATE column becomes raw DATE `= 20200101` (interpreted as epoch
days), so the actual 2020-01-01 row is pruned. Likewise, `CAST(double_col AS
INT) = 1` pushes raw DOUBLE `= 1.0` and drops 1.5 even though Doris' cast
matches. This is wrong without LIMIT, and the retained BE residual cannot
restore rows. Please preserve cast provenance or reject every raw-domain
translation not proven equivalent, with non-string comparison and IN tests.
##########
fe/fe-connector/fe-connector-trino/src/main/java/org/apache/doris/connector/trino/TrinoPredicateConverter.java:
##########
@@ -305,4 +306,20 @@ private BigDecimal toBigDecimal(Object value) {
}
return new BigDecimal(String.valueOf(value));
}
+
+ private Object convertStringLiteralValue(ConnectorLiteral literal) {
+ if (literal.isNull()) {
+ return null;
+ }
+ String literalType =
literal.getType().getTypeName().toUpperCase(Locale.ROOT);
+ switch (literalType) {
+ case "CHAR":
+ case "VARCHAR":
+ case "STRING":
+ return Slices.utf8Slice((String) literal.getValue());
+ default:
+ throw new UnsupportedOperationException(
Review Comment:
[P2] Avoid WARNs for this expected fallback
This exception is the intended path for the valid CAST queries fixed by the
PR, but `convert()`/`convertAnd()` log caught conversion exceptions at WARN,
and the converter runs once in metadata `applyFilter` and again in `planScan`.
One rejected predicate therefore produces two warnings (q42's two range
children produce four) even though pushdown merely degraded safely. Please
represent the expected unsupported result explicitly, or log only that case at
DEBUG; keep unexpected implementation failures on the WARN/error path.
##########
fe/fe-connector/fe-connector-trino/src/main/java/org/apache/doris/connector/trino/TrinoPredicateConverter.java:
##########
@@ -305,4 +306,20 @@ private BigDecimal toBigDecimal(Object value) {
}
return new BigDecimal(String.valueOf(value));
}
+
+ private Object convertStringLiteralValue(ConnectorLiteral literal) {
+ if (literal.isNull()) {
+ return null;
+ }
+ String literalType =
literal.getType().getTypeName().toUpperCase(Locale.ROOT);
+ switch (literalType) {
+ case "CHAR":
+ case "VARCHAR":
+ case "STRING":
+ return Slices.utf8Slice((String) literal.getValue());
+ default:
+ throw new UnsupportedOperationException(
+ "Cannot convert Doris literal type " + literalType + "
to a Trino string type");
Review Comment:
[P1] Require exact predicate enforcement before source LIMIT
This branch exposes a wider-domain fallback, but
`TrinoScanPlanProvider.planScan()` still calls the embedded connector's
`applyLimit()` whenever the scan carries a limit. `WHERE CAST(c_str AS
DATETIME) >= ... LIMIT 10` can therefore limit an unconstrained scan before
Doris evaluates the CAST; partial AND has the same problem. Exactness cannot be
inferred merely from a non-`all()` domain either: cast erasure turns
`CAST(c_str AS DATETIME) IS NOT NULL` into raw VARCHAR `notNull`, so malformed
strings can consume the first ten remote rows while later valid strings should
match. `effectiveSourceLimit()` already suppresses this ordering hazard when
the engine knows a predicate was stripped, but Trino opts into CAST pushdown
and hides both converter-local fallback and widened translations. Please apply
a source limit only when every retained predicate is proven to be enforced
exactly before it, and add limit-capable tests for fallback, partial AND, and
casted `IS NOT NULL`.
--
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]
