[fix](function) Handle case-insensitive auto partition arguments - #67121
Open
Mryange wants to merge 2 commits into
Open
[fix](function) Handle case-insensitive auto partition arguments#67121Mryange wants to merge 2 commits into
Mryange wants to merge 2 commits into
Conversation
Contributor
Author
|
/review |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review result: request changes
I found two blocking issues: FE and BE still disagree on locale-dependent keyword normalization, leaving an FE-accepted two-argument call on the BE crash path; and the added BE unit test fails before it can prove the fix, then still uses a row shape that cannot independently exercise the cases.
Critical checkpoint conclusions:
- Goal and proof: the BE change handles locale-independent ASCII case variants, but does not fully align with FE accepted-input semantics; the new test does not currently provide valid proof.
- Scope/focus: the production diff is compact and focused. Fixing FE normalization and restructuring the test are both required to complete the stated goal.
- Concurrency: the function is stateless and uses block-local data; no shared-state, lock-order, or atomic concern applies.
- Lifecycle/static initialization: no lifecycle, ownership-cycle, destructor, or cross-TU static initialization change is introduced.
- Configuration: no configuration item or dynamic-reload behavior is added.
- Compatibility: no symbol, protocol, storage-format, or persisted-data change is present. Exact comparisons preserve locale-independent FE-valid tokens and reject existing invalid prefixes; locale parity remains the compatibility gap.
- Parallel paths: Nereids literal validation, FE literal translation, BE constant/mixed execution, constant folding, list dispatch, and all six range granularities were traced. There is no separate unvalidated legacy scalar path.
- Conditional/error handling: the normalized list/range branches are otherwise consistent with FE's literal invariant, but the locale mismatch can still select range for a two-argument FE-accepted list call and index the missing third argument.
- Test coverage: existing regressions cover lowercase granularities and invalid prefixes. The new mixed-case unit test must use the non-nullable result type and independent one-row constant-literal cases; Turkish-default-locale coverage is also needed for FE/BE parity.
- Test results: the new expected results were reviewed against
check_functionand the implementation and are not executable as written. No repository builds/tests were run because this review runner explicitly prohibits them; current live checks do not include BE UT execution. - Observability: no new distributed or operational path requires logs or metrics.
- Transactions/persistence: no transaction, EditLog, failover, version-visibility, or persistence behavior is touched.
- Data writes: no data-write, atomicity, crash-recovery, or MoW behavior is involved.
- FE-BE variables: no new transmitted field is added; the defect is inconsistent normalization of the same transmitted raw literal.
- Performance/memory: two tiny once-per-block strings are immaterial; no significant allocation, MemTracker, copy, or hot-loop issue was found.
- Build hygiene: the standard include is confined to a
.cpp; no hub header, PCH, gate table, explicit instantiation, source-including test, or unity-skip rule is affected. - User focus: no additional user-provided focus was supplied.
Review completion: three permitted rounds were completed. The final complete-review and risk-focused reviewers both returned NO_NEW_VALUABLE_FINDINGS; all candidates were accepted, merged, or dismissed with evidence, so the review converged with the two inline issues below.
| [](unsigned char c) { return static_cast<char>(std::tolower(c)); }); | ||
| // partition type is list|range | ||
| if (std::strncmp(partition_type, "list", 4) == 0) { | ||
| if (partition_type == "list") { |
Contributor
There was a problem hiding this comment.
[P1] Keep FE and BE keyword normalization locale-independent. FE still validates both literals with parameterless Java String.toLowerCase(), while this byte-wise C-locale transform only canonicalizes ASCII. With a Turkish-default FE, LİST.toLowerCase() is list, so auto_partition_name('LİST', 'x') is accepted as a two-argument list call; the raw literal is transmitted unchanged, becomes lİst here, misses this branch, and the range path indexes the missing third argument. MİNUTE has the same FE-accepted/BE-unmatched split. Please use Locale.ROOT in FE validation or transmit the validated canonical token, and add Turkish-default-locale coverage.
| {{"RANGE", "MONTH", "2022-12-12 19:20:30"}, "p20221201000000"}, | ||
| {{"rAnGe", "dAy", "2022-12-12 19:20:30"}, "p20221212000000"}, | ||
| }; | ||
| ASSERT_TRUE(check_function<DataTypeString, true>("auto_partition_name", range_input_types, |
Contributor
There was a problem hiding this comment.
[P1] Make these checks use the function's real result and literal shapes. Both calls request Nullable(String) through <DataTypeString, true>, but auto_partition_name declares non-nullable String, so the function builder rejects them before execution. With that corrected, the plain input descriptors still create two-row non-const columns while the implementation reads dispatch and range granularity only from row 0. Both range rows therefore execute MONTH, making the second actual result p20221201000000 rather than p20221212000000; LiSt/rAnGe/dAy in row 2 never independently control the paths they claim to test. Please use non-nullable check_function<DataTypeString> calls and split each casing into a separate one-row invocation, marking production-literal arguments constant.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16815 ms |
Contributor
TPC-DS: Total hot run time: 81635 ms |
Contributor
ClickBench: Total hot run time: 14.66 s |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16674 ms |
Contributor
TPC-DS: Total hot run time: 82389 ms |
Contributor
ClickBench: Total hot run time: 14.52 s |
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
3 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.
AUTO_PARTITION_NAMEaccepted case-insensitivelistandrangearguments in FE, but BE compared them case-sensitively. UppercaseLISTwas incorrectly routed to the range implementation and could access a missing third argument, causing a BE core. The BE now normalizes partition type and range granularity arguments before dispatch, with unit coverage for mixed-case inputs.