Skip to content

[fix](be) Reject lossy file predicate literal conversion#65490

Closed
Gabriel39 wants to merge 2 commits into
apache:masterfrom
Gabriel39:codex/fix-filescanner-literal-localization
Closed

[fix](be) Reject lossy file predicate literal conversion#65490
Gabriel39 wants to merge 2 commits into
apache:masterfrom
Gabriel39:codex/fix-filescanner-literal-localization

Conversation

@Gabriel39

Copy link
Copy Markdown
Contributor

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.5 for an INT file column to value < 1 incorrectly rejects a file value of 1.

This change requires literal conversion to round-trip exactly through the file type before localization. Exact conversions such as BIGINT 1 -> INT 1 -> BIGINT 1 continue 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)

  • Test: Unit Test
    • ColumnMapperCastTest.ColumnMapperRejectsLossyBinaryLiteralConversion
    • ColumnMapperCastTest.ColumnMapperRejectsLossyInPredicateLiteralConversion
    • Existing safe binary and IN literal conversion tests
  • Behavior changed: Yes. Lossy literal conversions now fall back to casting the file slot.
  • Does this need documentation: No

Validation on the designated Linux build host:

  • build-support/check-format.sh
  • Targeted BE unit tests: 4 tests passed
  • run-clang-tidy.sh was attempted; it is blocked by pre-existing diagnostics in the file and toolchain/header diagnostics unrelated to the changed lines.

### 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
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39 Gabriel39 marked this pull request as ready for review July 12, 2026 04:58
@Gabriel39 Gabriel39 requested a review from yiguolei as a code owner July 12, 2026 04:58
@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread be/src/format_v2/column_mapper.cpp
### 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
@Gabriel39

Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.cpp and column_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, and thirdparty/installed/bin/protoc; the available clang-format is 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.

@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 83.56% (61/73) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 56.91% (23549/41377)
Line Coverage 40.58% (230275/567513)
Region Coverage 36.50% (182034/498780)
Branch Coverage 37.56% (81261/216323)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 89.04% (65/73) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 74.68% (30150/40372)
Line Coverage 58.70% (331065/563951)
Region Coverage 55.47% (277729/500676)
Branch Coverage 56.74% (122786/216401)

Gabriel39 added a commit to Gabriel39/incubator-doris that referenced this pull request Jul 12, 2026
### 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
@Gabriel39 Gabriel39 closed this Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants