github-actions[bot] commented on code in PR #67209:
URL: https://github.com/apache/doris/pull/67209#discussion_r3977793865
##########
fe/fe-connector/fe-connector-trino/src/test/java/org/apache/doris/connector/trino/TrinoPredicateConverterTest.java:
##########
@@ -162,6 +163,36 @@ public void testVarcharEqEncodesAsSlice() {
CONVERTER.convert(cmp));
}
+ @Test
+ public void testVarcharDatetimeComparisonDegradesToAll() {
+ // CAST(c_str AS DATETIME) >= TIMESTAMP '2026-08-24 00:00:00' reaches
this converter as
+ // c_str >= <DATETIME literal> because fe-core unwraps CastExpr.
Encoding the datetime as a
Review Comment:
[P2] Keep the capability contract and test rationale in sync
With this PR's `supportsCastPredicatePushdown() == false` plus metadata
deferral, an ordinary CAST predicate no longer reaches this converter from
`PluginDrivenScanNode`; this direct converter case is defense-in-depth. More
importantly, the authoritative
`ConnectorPushdownOps.supportsCastPredicatePushdown()` Javadoc still explicitly
lists the Trino bridge among connectors that deliberately return `true`, the
opposite of the new runtime behavior and rationale. Please reword this test
comment and update the shared SPI contract in the same change so the
safety-critical opt-out guidance stays accurate.
##########
fe/fe-connector/fe-connector-trino/src/main/java/org/apache/doris/connector/trino/TrinoConnectorDorisMetadata.java:
##########
@@ -252,66 +249,22 @@ public Map<String, ConnectorColumnHandle>
getColumnHandles(
}
/**
- * The trino-connector bridge accepts CAST-bearing predicates ({@code
true}, the SPI default, stated here
- * rather than inherited).
- *
- * <p>This is a conscious acceptance of the risk the SPI documents, not a
claim of safety: the residual
- * predicate becomes a trino {@code Constraint} and is handed to the
embedded trino connector's own
- * {@code applyFilter}, which may turn it into source-side filtering with
that system's coercion rules. It
- * stays {@code true} because the bridge cannot tell which embedded
connector will do so, and dropping all
- * CAST-bearing conjuncts would silently de-optimize every trino
catalog.</p>
+ * CAST nodes are erased at the Doris connector-expression boundary, so
the bridge cannot prove that a
+ * domain over the raw Trino column preserves the casted Doris comparison.
Keep those predicates local.
*/
@Override
public boolean supportsCastPredicatePushdown(ConnectorSession session) {
- return true;
+ return false;
}
Review Comment:
[P1] Make the CAST gate cover `TryCastExpr`
Returning `false` only protects expressions found by
`PluginDrivenScanNode.containsCastExpr()`, but that helper calls
`expr.collect(CastExpr.class, ...)`, and the class-based `TreeNode.collect`
matches with `cl.equals(getClass())`. `TryCastExpr extends CastExpr`, so
`TRY_CAST` is missed; the connector converter then uses `instanceof CastExpr`
and unwraps it. For example, `try_cast(c_str AS INT) IS NULL` becomes raw
`c_str IS NULL`, allowing Trino to discard a malformed non-null value such as
`'abc'` even though Doris turns it into NULL and the row should match. Please
make the gate subclass-aware and add a Trino regression for malformed
`TRY_CAST(... ) IS NULL`/`<=> NULL`. This is distinct from the existing
ordinary-CAST null thread because that exact `CastExpr` path is now stripped;
the subclass is what bypasses the fix.
--
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]
