[fix](fe) Reject unsupported casts during constant folding - #66785
[fix](fe) Reject unsupported casts during constant folding#66785Mryange wants to merge 2 commits into
Conversation
### What problem does this PR solve? Issue Number: N/A Problem Summary: Constant folding evaluated literal casts before validating whether their source and target types were compatible. This allowed an unsupported BIGINT to TIMESTAMPTZ cast to be folded into an incorrect timestamp literal while the same cast on a column was rejected. Reuse CheckCast during FE constant folding so unsupported casts remain for the normal validation rule to reject consistently. ### Release note Reject unsupported constant casts from BIGINT to TIMESTAMPTZ consistently with non-constant casts. ### Check List (For Author) - Test: Added unit and regression coverage; passed FE Checkstyle. Unit and regression tests were not run. - Behavior changed: Yes. Unsupported constant casts are rejected instead of being folded. - Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
Codex automated review failed and did not complete. Error: This content was flagged for possible cybersecurity risk. If this seems wrong, try rephrasing your request. To get authorized for security work, join the Trusted Access for Cyber program: https://chatgpt.com/cyber Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
There was a problem hiding this comment.
Automated review conclusion: changes requested.
The projected SELECT fix is directionally correct, but the stated goal of a consistent analysis error is not reached across all reachable plan shapes. The attached blocking finding shows that invalid constant casts can be discarded before CheckCast, while default fast INSERT ... VALUES never runs that validator.
Critical checkpoint conclusions:
- Goal and correctness: The guard fixes the ordinary FE-folded projection, and the added tests prove that local path, but the reachable ORDER BY, GROUP BY, and fast-insert paths leave the goal incomplete.
- Scope and conditional design: The patch is small and reuses the existing legality table, but returning a residual cast is only safe when every consumer is guaranteed to validate it; that invariant does not hold.
- Parallel paths and configuration: Strict/non-strict casting,
TryCast, BE folding, fold-disabled execution, nested casts, and recursive complex casts were checked. The normal variants preserve the cast until validation; default fast values analysis is the surviving parallel-path gap. No new configuration was added. - Tests and results: The unit checks and expected-error regression are coherent, but they omit the three concrete failing plan shapes in the inline finding. No local build or test was run, as required by the review runner. CI shows successful CheckStyle and general repository checks, but no relevant FE unit/regression result was present.
- Concurrency and lifecycle: Not involved; this is a stateless expression rewrite and introduces no lock, thread, resource, or initialization lifecycle.
- Compatibility and protocol: No function symbol, storage format, persisted state, or FE-BE variable/protocol change is introduced.
- Transactions and data writes: No transaction or persistence mechanism is changed. The fast-insert issue is validation timing before execution, not a new atomicity mechanism.
- Performance and observability: The legality lookup is bounded and no material performance issue was found; this analysis-time validation change does not require new logs or metrics.
- User focus: No additional user-provided focus was supplied.
- Other issues: No additional non-duplicate defect remained after three rounds; the final round converged.
| Expression child = cast.child(); | ||
| DataType dataType = cast.getDataType(); | ||
| if (!CheckCast.check(child.getDataType(), dataType, SessionVariable.enableStrictCast())) { | ||
| return cast; |
There was a problem hiding this comment.
This early return relies on a later CheckCast, but validation currently happens after rules that can erase constant expressions. For example:
Sort(CAST(CAST(20240229112233 AS BIGINT) AS TIMESTAMPTZ(6)))
is processed by EliminateOrderByConstant before expression normalization; because the invalid deterministic cast is still isConstant(), the sort key is removed and CheckCast never sees it. Similarly, NormalizeAggregate.eliminateGroupByConstant folds a GROUP BY-only occurrence, treats the unchanged cast as constant, and removes it before validation. Thus the corresponding ORDER BY and GROUP BY queries can be accepted while the projected expression in the new regression errors. The default fast INSERT ... VALUES analyzer also invokes this folder without any CheckCast and skips normal rewrite, so that path fails only during BE execution. Please enforce cast legality before constant-expression elimination/translation, and add regressions for these paths.
### What problem does this PR solve?
Issue Number: N/A
Problem Summary: FE constant folding could evaluate unsupported constant casts before cast validation. Throw a Nereids AnalysisException when CheckCast rejects the source and target types so special planning paths cannot accept the invalid expression.
### Release note
Reject unsupported constant casts consistently with non-constant casts.
### Check List (For Author)
- Test: Unit Test
- FoldConstantTest and CheckCastTest: 51 tests passed
- Behavior changed: Yes. Unsupported constant casts fail during FE constant folding.
- Does this need documentation: No
|
/review |
There was a problem hiding this comment.
Automated review conclusion: changes requested.
The current head improves the previous patch: throwing from FoldConstantRuleOnFE.visitCast now rejects the ordinary projected constant, the direct GROUP BY key, and default/batch VALUES analysis. However, the stated goal of consistent rejection is still incomplete because validation remains inside the folder instead of preceding every rule that may erase an expression.
The existing blocking thread remains applicable: EliminateOrderByConstant runs before expression normalization and drops an unsupported constant sort key solely because isConstant() is true, so neither the new throw nor the later CheckCast executes. A nested GROUP BY form such as FALSE AND is_null(CAST(BIGINT literal AS TIMESTAMPTZ(6))) has the same root problem: visitor-mode visitAnd returns on FALSE without visiting the later cast, after which NormalizeAggregate.eliminateGroupByConstant removes the key and bottom expression. This is the same validation-order invariant, location family, effect, and requested fix already captured in #66785 (comment), so I did not create a duplicate inline thread.
Critical checkpoint conclusions:
- Goal and correctness: Partially achieved. Direct folding now rejects the target unsupported cast, but ORDER BY-only and nested short-circuit GROUP BY expressions can still be accepted after the invalid cast is discarded before validation.
- Scope and conditional design: The production edit is small and reuses the existing whitelist, but the legality invariant is attached to an optional optimization traversal. Cast legality needs an independent stage guaranteed to run before constant-expression elimination/translation.
- Parallel paths and configuration: Normal projection, direct GROUP BY, default and batch VALUES, visitor and pattern traversal, strict/non-strict modes,
debug_skip_fold_constant, BE folding,TRY_CAST, nested casts, NULL, and recursive complex casts were traced. No distinct additional defect survived duplicate suppression, and no configuration item was added. - Special conditions: Bottom-up pattern traversal visits nested casts, while the deep visitor may short-circuit
And/Or; this explains the nested GROUP BY survivor above. Generic exception swallowing inFoldConstantRule.evaluatedid not yield another substantiated user-visible escape in the inspected callers. - Tests and results: The two unit additions and projected-query expected-error regression are deterministic and coherent, but they do not cover the still-failing ORDER BY-only or nested GROUP BY plan shapes. No local build or test was run because the review runner explicitly prohibits builds. Live CI shows CheckStyle and general repository checks passing, but no relevant FE unit/regression result is visible.
- Concurrency and lifecycle: Not involved. The rewrite is stateless and adds no locks, threads, shared mutable state, resource ownership, or initialization lifecycle.
- Compatibility and protocol: No function symbol, storage format, persisted state, rolling-upgrade contract, or FE-BE variable/protocol change is introduced.
- Transactions and data writes: No transaction, EditLog, or atomicity mechanism is changed. VALUES analysis was traced only as a parallel validation path and now rejects the direct unsupported cast at this head.
- Performance and observability: The added whitelist lookup is bounded and analysis-only; no material CPU, memory, or allocation issue was found. This validation change does not require new logs, metrics, or tracing.
- User focus: No additional user-provided review focus was supplied.
- Other issues and completion: Both the full-scope and risk-focused Round 1 reviews returned
NO_NEW_VALUABLE_FINDINGS, and every initial risk was independently verified, dismissed with evidence, or deduplicated. The review converged after Round 1 with the existing blocker still unresolved.
|
/review 重新review下,我觉得 ORDER BY / GROUP BY 短路路径都消除了,没有检查是没有问题的。 |
There was a problem hiding this comment.
Automated re-review conclusion: no blocking code finding (comment-only; zero inline comments).
This focused rerun supersedes the earlier automated blocking opinion on the current head. The patch correctly applies the established CheckCast policy before the FE constant evaluator can manufacture an invalid TIMESTAMPTZ literal, and the final review found no remaining distinct correctness defect.
Critical checkpoint conclusions:
- Goal and correctness: The projected constant, direct GROUP BY key, and folding-enabled default/batch VALUES paths reach the shared folder and now reject the unsupported BIGINT-to-TIMESTAMPTZ cast before invalid evaluation. The normal INSERT path retains its independent analysis validation.
- User focus: I agree that an ordinary ORDER BY-only constant and a visitor-short-circuited nested GROUP BY branch may be removed without validation when no output, predicate, cardinality, ordering, or side-effect consumer survives. That is not a result-correctness issue. Direct GROUP BY is different:
NormalizeAggregatedeliberately invokes the shared folder, and the current head now rejects the unsupported conversion. The earlier validation-order thread is therefore not a remaining blocker for this patch. - Scope and special paths: The change is small and reuses the normal cast-compatibility predicate and error contract. Strict and non-strict modes, TRY_CAST, literal NULL, recursive complex casts, synthetic result-type restoration, AggState conversion, SQL-cache folding, fold-disabled behavior, and FE/BE fallback were rechecked. No legitimate cast was shown to be rejected by the new all-literal boundary.
- Tests and results: The unit additions cover the compatibility predicate in both strictness modes and the direct-folder exception; the regression covers the reported projected query with the required expected-error form. No local build or test was run because this review runner prohibits it. Live CheckStyle and lightweight repository checks pass, but no relevant FE unit/regression result is visible. The head commit reports 51 unit tests; that claim was not independently verified here and does not establish that the added regression ran.
- Concurrency, lifecycle, and configuration: No threads, locks, shared mutable state, resource lifecycle, static-initialization dependency, or new configuration item is involved. Existing session settings are read at analysis time as before.
- Compatibility, protocol, persistence, and data writes: No function symbol, storage/serialization format, persisted/EditLog state, transaction, data-visibility mechanism, cloud path, or FE-BE protocol/variable changes. Mixed FEs differ only in the intended handling of this invalid query.
- Performance and observability: The bounded analysis-time type-policy lookup is not a material CPU, allocation, memory, or runtime hot-path concern. The source/target-type analysis error is sufficient observability.
- Non-blocking metadata follow-up: Please update the live PR description. It still says
Release note: Noneand leaves Test and Behavior changed unchecked, despite the user-visible rejection behavior and the head commit's unit-test report. Mark only testing actually performed; the available evidence does not show that the added regression suite ran. - Completion: Three review rounds completed. The final full-scope and risk-focused reviewers both returned
NO_NEW_VALUABLE_FINDINGS; zero accepted inline comments remain.
Problem Summary: FE constant folding could evaluate an unsupported
BIGINTtoTIMESTAMPTZcast before cast validation, producing an incorrect timestamp. This change validates constant casts first and throws an analysis error for unsupported type combinations.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)