[fix](insert) enable_insert_strict should not affect semantic of enable_strict_cast#63794
Open
jacktengg wants to merge 1 commit into
Open
[fix](insert) enable_insert_strict should not affect semantic of enable_strict_cast#63794jacktengg wants to merge 1 commit into
jacktengg wants to merge 1 commit into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31650 ms |
Contributor
TPC-DS: Total hot run time: 171760 ms |
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
FE Regression Coverage ReportIncrement line coverage |
…le_strict_cast
Problem Summary:
`enable_insert_strict` and `enable_strict_cast` are two independent session
variables: the former controls how many error rows a load/insert tolerates
(row filtering and `insert_max_filter_ratio`), while the latter controls the
strictness of cast/conversion semantics (e.g. whether out-of-range or
overflowing casts fail or produce NULL). They were wrongly coupled:
- In FE, `SessionVariable.enableStrictCast()` returned `enableInsertStrict`
instead of `enableStrictCast` whenever the current statement was an insert.
As a result, toggling `enable_insert_strict` silently changed cast semantics
for insert statements, and `enable_strict_cast` had no effect on inserts.
- In BE, `OlapTableBlockConvertor` truncated over-length string columns with a
`substring` only when `enable_insert_strict` was false, so string truncation
behavior was driven by the load-tolerance flag rather than by the cast/plan
decided in FE.
This commit decouples the two variables:
- FE `SessionVariable.enableStrictCast()` now always returns `enableStrictCast`,
regardless of whether the statement is an insert.
- FE `BindSink` drives string truncation off the cast semantics (via
`truncateString`, which depends on `enableStrictCast()`), and also handles
unbounded source string types whose length is reported as -1 (e.g.
`string`/`text`): such sources are always longer than a bounded char/varchar
target and are now correctly truncated with `substring`.
- BE `OlapTableBlockConvertor` no longer performs `enable_insert_strict`-gated
substring truncation. FE is now responsible for planning the `substring`, and
BE always validates that each row's byte length does not exceed the schema
length (since schema length is in bytes while substring works on chars).
After the fix, `enable_strict_cast` controls cast/truncation semantics for
inserts as documented, and `enable_insert_strict` only controls error-row
filtering, independent of each other.
Fixed an issue where `enable_insert_strict` incorrectly changed the semantics
of `enable_strict_cast` for INSERT statements. The two session variables are
now independent.
- Test: Regression test
- Updated and added regression tests under `regression-test/suites/load_p0`
and `nereids_function_p0` to cover the decoupled behavior of
`enable_insert_strict` and `enable_strict_cast`.
- Behavior changed: Yes (`enable_insert_strict` no longer affects cast
strictness; `enable_strict_cast` now applies to INSERT statements)
jacktengg
force-pushed
the
fix-insert-strict
branch
from
0d84131 to
bb5943a
Compare
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code review completed. I did not find blocking correctness issues in the PR diff.
Critical checkpoint conclusions:
- Goal and tests: The PR decouples insert row-filter strictness from strict cast semantics, moves string truncation planning to FE for inserts, and adds regression coverage for values/select/TVF cases.
- Scope: The code changes are focused on FE insert coercion, BE load validation, session variable lookup, and matching regression tests.
- Concurrency/lifecycle: No new concurrency, lifecycle, or static initialization concerns found.
- Configuration/compatibility: No new configuration item or persisted format/protocol change found.
- Parallel paths: Nereids insert and load paths were considered; stream/TVF tests were updated for the changed validation behavior.
- Error handling/data correctness: BE still validates byte length after any FE substring, preserving byte-length enforcement for multi-byte strings.
- Test coverage: Regression tests cover the main combinations of enable_insert_strict, enable_strict_cast, enable_insert_value_auto_cast, and insert_max_filter_ratio. I did not run the suite locally in this review environment.
- Observability/performance: No new logging or hot-path performance issue found beyond the expected removal of BE-side substring execution.
User focus: No additional user-provided review focus was specified.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31027 ms |
Contributor
TPC-DS: Total hot run time: 171986 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)