[fix](be) Reject lossy file predicate literal conversion#65490
[fix](be) Reject lossy file predicate literal conversion#65490Gabriel39 wants to merge 2 commits into
Conversation
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: FileScannerV2 localized predicate literals to the file column type whenever conversion succeeded, even when the conversion changed the literal value. For example, localizing a DOUBLE predicate value of 1.5 to an INT file column changed the boundary to 1 and could make file-level filtering silently drop matching rows. Require an exact round trip through the file type before localizing a literal; otherwise, preserve table semantics by casting the file slot. Exact conversions such as BIGINT 1 to INT 1 remain localized.
### Release note
Fix FileScannerV2 predicate localization to avoid missing rows after lossy literal conversion.
### Check List (For Author)
- Test: Unit Test
- ColumnMapperCastTest targeted literal localization tests
- Behavior changed: Yes, lossy literal conversions now fall back to casting the file slot.
- Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed PR 65490 at 2bffb2d.
I found one correctness issue in the new round-trip guard and left it inline. Static review only: this checkout is missing .worktree_initialized and thirdparty/installed, so I could not run the BE test suite locally. I did run git diff --check for the changed files, which passed.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#65490 Problem Summary: Round-trip equality alone can accept unsafe predicate localization when file values lose information during table materialization or when complex Field equality ignores nested contents. Restrict localization to scalar numeric file-to-table casts that preserve every file value, retain the exact literal round-trip check, and fall back to the table predicate for complex or unsupported casts. ### Release note Prevent file predicate localization from dropping rows for lossy schema-evolution casts. ### Check List (For Author) - Test: Unit Test - ColumnMapperCastTest (19 tests passed) - Behavior changed: Yes. Unsafe literal localization now falls back to evaluating the table-typed predicate after materialization. - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review completed for PR 65490.
I did not find any new substantiated issue to raise as an inline comment. The existing inline thread on be/src/format_v2/column_mapper.cpp:641 already covers the round-trip-only literal-localization concern, and I did not duplicate it.
Critical checkpoint conclusions:
- Goal and proof: The patch targets FileScannerV2 predicate literal localization for lossy schema-evolution casts. The current code gates literal localization through
is_lossless_file_to_table_numeric_cast()plus an exact literal round trip, and the added unit cases cover lossy scalar literals, lossy file-to-table conversion, complex literal rejection, and IN-predicate fallback. - Scope: The change is focused on
column_mapper.cppandcolumn_mapper_test.cpp, with no unrelated source churn. - Concurrency/lifecycle/config/persistence: No new concurrency, lifecycle, configuration, persistence, transaction, or FE-BE protocol surface was introduced.
- Parallel paths: Binary comparison and IN-predicate localization both use the same literal rewrite helper. Unsupported or unsafe casts fall back to a table-typed predicate over a cast file slot, while complex literal localization is skipped.
- Data correctness: For the reviewed scalar numeric paths, file-local filtering is only kept when every file value survives materialization to the table type and the literal boundary is exactly representable. Otherwise, the original table semantics are preserved.
- Tests: The added BE unit tests cover the relevant mapper decisions. I did not run tests locally because this checkout lacks
.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protoc; the availableclang-formatis version 18 rather than Doris-required v16. - Performance/observability: The patch may conservatively skip some literal pushdown for unsupported exact scalar families, but this is the safer behavior for correctness. No new observability appears necessary for this local mapper fix.
User focus: No additional user-provided focus was supplied in .code-review.4ktG6h/review_focus.txt.
Subagent conclusions: The optimizer-rewrite and tests-session-config subagents reported no new valuable findings in their initial passes. After the main ledger recorded the duplicate suppression and empty proposed final inline set, convergence round 1 ended with both subagents replying NO_NEW_VALUABLE_FINDINGS for the same current ledger/comment set.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: N/A Related PR: apache#65490 Problem Summary: Round-trip equality alone can accept unsafe predicate localization when file values lose information during table materialization or when complex Field equality ignores nested contents. Restrict localization to scalar numeric file-to-table casts that preserve every file value, retain the exact literal round-trip check, and fall back to the table predicate for complex or unsupported casts. ### Release note Prevent file predicate localization from dropping rows for lossy schema-evolution casts. ### Check List (For Author) - Test: Unit Test - ColumnMapperCastTest (19 tests passed) - Behavior changed: Yes. Unsafe literal localization now falls back to evaluating the table-typed predicate after materialization. - Does this need documentation: No
What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary:
FileScannerV2 localized predicate literals to the file column type whenever conversion succeeded, even if the conversion changed the literal value. This could make file-level filtering less selective than the original table predicate and silently drop matching rows. For example, rewriting a DOUBLE predicate
value < 1.5for an INT file column tovalue < 1incorrectly rejects a file value of1.This change requires literal conversion to round-trip exactly through the file type before localization. Exact conversions such as
BIGINT 1 -> INT 1 -> BIGINT 1continue to use the file-local predicate. Lossy conversions fall back to casting the file slot and retain the original table-typed literal.Release note
Fix FileScannerV2 predicate localization to avoid missing rows after lossy literal conversion.
Check List (For Author)
ColumnMapperCastTest.ColumnMapperRejectsLossyBinaryLiteralConversionColumnMapperCastTest.ColumnMapperRejectsLossyInPredicateLiteralConversionValidation on the designated Linux build host:
build-support/check-format.shrun-clang-tidy.shwas attempted; it is blocked by pre-existing diagnostics in the file and toolchain/header diagnostics unrelated to the changed lines.