gner56 opened a new pull request, #70511:
URL: https://github.com/apache/doris/pull/70511
…runing
`PluginDrivenScanNode.buildRemainingFilter` decides which conjuncts a
connector receives for scan planning. The neutral converter,
`ExprToConnectorExpressionConverter`, unwraps every `CastExpr`, so a connector
that returns `true` from `supportsCastPredicatePushdown` sees `CAST(col AS T)
<op> literal` as `col <op> literal`. Iceberg returns `true` and hands the
result to `TableScan.filter`, which prunes manifests and data files at the
source.
Issue Number: close #70503
Related PR: #67933 (the same row loss fixed in the Paimon converter on
branch-4.x), #55804 (same CAST stripping, string column cast to datetime; not
fixed by this PR)
PR Type: Bugfix
Problem Summary:
A predicate comparing an integer column as a string, such as `CAST(id AS
VARCHAR) >= '100'`, silently returns too few rows on Iceberg tables. The CAST
is stripped and the string literal is bound to the column's type, so Iceberg
prunes on `id >= 100`; string and numeric order disagree (`'15' >= '100'`, but
`15 < 100`), so a data file holding only `15` is skipped and its rows are never
read. This keeps any conjunct that casts a non-string column to a string type
in Doris, next to the existing `containsTimestampTzCast` check, so Iceberg and
every other connector accepting CASTs no longer prune on it. Results become
correct; such predicates no longer prune files.
- **Frequency:** Every query on an Iceberg table that compares a non-string
column cast to a string with a range or inequality, when the table's files are
laid out so the numeric bound excludes a whole file (common once files are
compacted or sorted by that column). No error is raised.
- **Root cause:** `ExprToConnectorExpressionConverter.convert` returns
`convert(expr.getChild(0))` for every `CastExpr`, and
`IcebergPredicateConverter.extractIcebergLiteral` parses the string literal
into the column's type (`Integer.parseInt` for INTEGER). Doris still evaluates
the original predicate on the rows it reads, so only rows in pruned files are
lost. The `supportsCastPredicatePushdown` javadoc in `IcebergConnectorMetadata`
names this risk as accepted, with no defect observed.
- **Why existing tests missed it:** `PluginDrivenScanNodeCastPredicateTest`
covers only a connector that rejects CASTs (Paimon); with CASTs accepted it
asserts the conjunct is pushed. No Iceberg suite casts an integer column to a
string with values split across files.
- **Fix:** `containsColumnCastToString` matches a `CastExpr` to a string
type whose child is a non-string `SlotRef`; `buildRemainingFilter` keeps such
conjuncts local for every connector. A cast of a constant (e.g. `varchar_col >
date '2023-03-07'`, where Nereids casts the date literal) still compares in
string order on both sides and keeps being pushed, as
`test_iceberg_predicate_conversion` q01 expects. Casts to non-string types keep
being pushed as before.
- **Other paths and branches:** `buildFilterConstraint` / `applyFilter` is
not gated; Iceberg does not prune from it, so it is left as is. branch-4.1 and
branch-4.2 have the same bug in `IcebergUtils.convertDorisExprToSlotRef`, which
unwraps any `CastExpr` around a `SlotRef`; they need a separate backport.
- Reproduced the bug manually on Doris 4.1.0 with a local Iceberg (hadoop)
catalog: two INSERTs (`15`, `150`) and `CAST(id AS VARCHAR) >= '100' AND
CAST(id AS VARCHAR) < '200'` return only `150`, with `icebergPredicatePushdown`
showing `ref(name="id") >= 100` and one of two splits scanned.
- Added `columnCastToStringStaysInDorisWhenTheConnectorAcceptsCasts` and
`otherCastsArePushedWhenTheConnectorAcceptsCasts` to
`PluginDrivenScanNodeCastPredicateTest`, and the regression suite
`test_iceberg_cast_to_string_predicate`.
- Not run locally: the FE build, the unit tests and the regression suite.
The suite's `.out` file is not included, since it must be generated by running
the suite; it needs `run external` in CI.
- Two unit cases depend on Nereids keeping the CAST as a `CastExpr`:
`cast(id as varchar) >= '15' or id = 1` (asserted as a precondition) and
`cast(id as bigint) > 5` (passes either way). If Nereids rewrites the first, it
needs a different shape.
- Matching only casts of a column, not of constants, is deliberate; see Fix
above.
Fix Iceberg queries that compare a non-string column cast to a string (e.g.
`CAST(int_col AS VARCHAR) >= '100'`) silently returning too few rows.
- Behavior changed: No
- Does this need documentation: No
- Pre-submission status: Partially verified: the fix and tests are not built
or run locally; the bug was reproduced manually on 4.1.0.
- Tool: Claude Code (Claude)
- Generated: the code change, the unit and regression tests, and this
description
- Human review: EDIT-ME
### Background
<!-- How the feature or module this PR touches works today, in two to four
plain sentences, for a reviewer who doesn't know it. Link the user doc if there
is one. Leave the problem and the fix to Problem Summary. -->
### What problem does this PR solve?
<!-- Issue Number: "close #N" if this PR resolves the issue, "#N" if it only
relates to it, "None" if there is none. A Feature needs an issue. -->
<!-- Related PR: "#N (relation)" for each PR a reviewer should know about,
such as "#123 (introduced the bug)", separated by commas. Write "None" if no
related PR is known. -->
<!-- PR Type: Bugfix, Performance, Feature, Enhancement, Refactor, Test,
Build/CI/Chore, Revert, Release/Dependency, or Other. -->
Issue Number:
Related PR:
PR Type:
Problem Summary:
<!-- Two to five sentences: the problem or need and who is affected; the
approach in one sentence; when the diff is larger than that suggests, what else
it had to handle, or that most of it is tests; and what existing users see
after an upgrade. -->
### Details
<!--
For a diff of more than about 300 changed lines, start with "- **Change
map:**": how many lines are production code, tests, generated files, and docs,
then the code in parts, each with the problem it handles and its size.
Then answer each field of your PR type on its own line, as "- **Field:**
answer", in this order:
Bugfix: Frequency, Root cause, Why existing tests missed it, Fix, Other
paths and branches
Performance: Why it was slow, Optimization, Results, Applicability and cost
Feature: Scope, Design, Usage, Default behavior, Compatibility
Enhancement: Before and after, Configs and defaults, Impact on existing
users
Refactor: Moves vs. logic changes, Behavior change, Compatibility,
Performance impact, Rollback
Test, adding tests: Scenarios, What existing tests miss, Fails when broken
Test, fixing a test: Failure, Cause, Stability evidence
Build/CI/Chore: Affected, Developer and release impact, License and
third-party, Rollback
Revert: Why not fix forward, Original author notified, Follow-up
Release/Dependency: Versions, Upstream changes, Compatibility, Rollback
Other: Why Other
"None" is a valid answer when it is true. Don't restate the diff.
-->
### Testing
<!-- The tests, suites, or commands that ran, what each shows, and where it
ran (locally or in CI). For a fix, the test that fails before it and passes
after. -->
### Notes for Reviewers
<!-- Optional; delete this section if there is nothing to say. Why each
non-obvious edit was made, what you are unsure about, and where to start
reading if Details has no Change map. -->
### Release note
<!-- One paragraph on what users will notice, or None. -->
### Check List (For Author)
<!-- Behavior changed: "No", or "Yes (see Details)" with a Behavior change
field in Details. -->
<!-- Does this need documentation: "No", or "Yes" with the
apache/doris-website PR or the plan. -->
<!-- Pre-submission status: "Ready"; "Partially verified: " and what is not
verified; or "Not ready: " and why, for a Draft PR. -->
- Behavior changed:
- Does this need documentation:
- Pre-submission status:
### AI Disclosure
<!-- Tool: the agent and model, or None. Generated: the parts it wrote
(code, tests, this description), or None. Human review: what you reviewed of
the generated parts, or "Not applicable" without AI tools. You may add "-
Prompts:" to summarize how you directed the agent. -->
- Tool:
- Generated:
- Human review:
--
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]
