github-actions[bot] commented on code in PR #65693:
URL: https://github.com/apache/doris/pull/65693#discussion_r3603498879
##########
be/src/format_v2/expr/cast.h:
##########
@@ -55,7 +56,7 @@ class Cast final : public VExpr {
const std::string& expr_name() const override { return _expr_name; }
Status clone_node(VExprSPtr* cloned_expr) const override {
DORIS_CHECK(cloned_expr != nullptr);
- *cloned_expr = Cast::create_shared(_data_type);
+ *cloned_expr = Cast::create_shared(_data_type, _lossless_decimal_cast);
Review Comment:
[P1] Preserve the selected-row safety of this localized cast — The source
`VCastExpr` always reports `is_safe_to_execute_on_selected_rows() == false`,
but `format::Cast` inherits the deterministic-child default. Hudi/Paimon native
readers replace the source with this class, so Parquet can run an earlier
single-column predicate on the full batch, compact it, and evaluate a strict
marked comparison only on survivors. For `id=1,s='bad'` plus `WHERE id > 1 AND
d = s`, that suppresses the strict parse error that the original conjunct would
raise. Please override the safety method to return false (or preserve it during
cloning) and add the selected-row differential test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExprToConnectorExpressionConverter.java:
##########
@@ -146,6 +150,17 @@ public static ConnectorExpression
convertConjuncts(List<Expr> conjuncts) {
return new ConnectorAnd(converted);
}
+ public static boolean containsLosslessDecimalCast(Expr expr) {
+ List<CastExpr> castExprs = new ArrayList<>();
+ expr.collect(CastExpr.class, castExprs);
+ return castExprs.stream().anyMatch(CastExpr::isLosslessDecimalCast);
+ }
+
+ public static boolean canPushDownLimit(List<Expr> conjuncts) {
Review Comment:
[P1] Use this LIMIT guard in the legacy Trino scan too —
`TrinoConnectorPredicateConverter` cannot convert `d = LosslessCast(s)` when
both sides are slots, so it leaves `TupleDomain.all()` and Doris retains the
comparison as a local residual. `TrinoConnectorScanNode.applyPushDown`,
however, still calls `connectorMetadata.applyLimit` whenever the scan has a
limit. With `(100,'100.4')` before `(100,'100')` and `WHERE d = s LIMIT 1`,
Trino can return only the first row; the residual rejects it and the exact row
is lost. Please gate Trino `applyLimit` with this helper and add a scan-level
regression.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExprToConnectorExpressionConverter.java:
##########
@@ -106,6 +106,10 @@ public static ConnectorExpression convert(Expr expr) {
} else if (expr instanceof LiteralExpr) {
return convertLiteral((LiteralExpr) expr);
} else if (expr instanceof CastExpr) {
+ if (((CastExpr) expr).isLosslessDecimalCast()) {
Review Comment:
[P1] Apply this guard to the catalog-specific predicate converters — Paimon,
Iceberg, LakeSoul, and HMS do not call this converter; their legacy converters
unwrap every `CastExpr` without checking the marker. For a Paimon VARCHAR `s`,
`WHERE s = 100.0` is analyzed as a lossless decimal comparison, but
`PaimonPredicateConverter` pushes raw string equality to `"100.0"`. That prunes
`s='1e2'` or `s='100.00'`, both of which the local lossless cast would match,
and the residual cannot recover them. Please reject marked casts before each
specialized file/partition/reader pushdown and add a noncanonical-string
connector regression.
--
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]
