[fix](parse) No longer throws exceptions when parse datetime failed in from_olap_string#63035
Merged
zclllyybb merged 1 commit intoapache:masterfrom May 8, 2026
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review 额外关注一下异常测试是否充足 |
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review completed for the actual PR diff (SerDe files and OlapTypeTest). I did not find blocking issues.
Critical checkpoints:
- Goal/test coverage: the PR consistently changes supported scalar
from_olap_string/from_fe_string/from_zonemap_stringparse failures to return type defaults, with BE unit coverage across numeric/date/time/decimal/IP/bitmap/HLL/quantile/string paths. - Scope: the actual GitHub PR diff is focused on BE SerDe behavior and tests.
- Concurrency/lifecycle: not applicable; no shared mutable state or lifecycle changes.
- Compatibility/persistence: no serialized layout/protocol change; behavior change is explicitly called out in the PR.
- Parallel code paths: default
from_fe_string/from_zonemap_stringdelegate tofrom_olap_string, and changed overridden SerDes are covered. - Negative/exception tests: focus point checked. The new invalid-input test covers broad parse-failure cases; I did not find an additional missing exception-test issue for this PR.
- Performance/observability: no meaningful hot-path regression or observability gap identified.
yiguolei
reviewed
May 7, 2026
yiguolei
reviewed
May 7, 2026
yiguolei
reviewed
May 7, 2026
yiguolei
reviewed
May 7, 2026
yiguolei
reviewed
May 7, 2026
yiguolei
reviewed
May 7, 2026
yiguolei
reviewed
May 7, 2026
zclllyybb
force-pushed
the
default_zonemap_dt
branch
2 times, most recently
from
091f1fd to
d66adef
Compare
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: DateV1/DateTimeV1 Field string deserialization returned an error for invalid OLAP strings while DateV2/DateTimeV2 already fell back to the type minimum for the same storage string paths. This change limits the fallback behavior to datelike types and keeps non-datelike parse failures as errors.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- ./run-be-ut.sh --run --filter=OlapTypeTest.*
- Behavior changed: Yes. Invalid DateV1/DateTimeV1 values passed through from_olap_string/from_fe_string/from_zonemap_string now produce the datelike minimum, matching DateV2/DateTimeV2; non-datelike invalid strings still return errors.
- Does this need documentation: No
zclllyybb
force-pushed
the
default_zonemap_dt
branch
from
d66adef to
2f194d5
Compare
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Summary: I found one blocking correctness issue. The remaining fallback is documented as a zonemap/partial-update workaround, but the PR also changes and tests from_fe_string to accept invalid datelike input. That path is used for FE-provided delete conditions, schema-change defaults, and default-value column iteration, so invalid user/configured DATE/DATETIME strings can silently become FIRST_DAY/MIN_DATE_V2 instead of failing.
Critical checkpoints:
- Goal/test: The PR aims to tolerate invalid datelike zonemap strings; tests cover fallback behavior, but they also assert fallback for from_fe_string, which is outside that goal.
- Scope: The change is not sufficiently focused because the fallback is implemented in from_olap_string, the shared backend for both from_zonemap_string and from_fe_string.
- Concurrency/lifecycle/config: No new concurrency, special lifecycle, or configuration concerns found.
- Compatibility/storage: No serialized format change found; existing corrupted/placeholder zonemap strings can be read, but FE string parsing semantics are broadened incorrectly.
- Parallel paths: DateV1/DateTimeV1/DateV2/DateTimeV2 are handled consistently, but the FE-vs-zonemap caller distinction is not preserved.
- Tests: Added BE UT coverage exists, but it codifies the incorrect from_fe_string behavior and lacks a negative assertion that FE/user strings still fail.
- Observability/performance/transactions: No new observability, performance, or transaction issues identified in this small SerDe change.
User focus: No additional user-provided review focus was supplied.
yiguolei
approved these changes
May 7, 2026
Contributor
Author
|
run buildall |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
Author
|
run beut |
zclllyybb
changed the title
from_olap_stringfrom_olap_string
Contributor
|
PR approved by anyone and no changes requested. |
github-actions Bot
pushed a commit
that referenced
this pull request
May 8, 2026
…n `from_olap_string` (#63035) Problem Summary: SerDe from_olap_string, from_fe_string, and from_zonemap_string handled invalid scalar strings inconsistently: some supported types returned errors while others filled default fields. This change makes supported string deserialization paths fill type defaults consistently on parse failure. ATTN: We used to write some values in zonemap that were considered unreasonable after upgrading to later versions, as these values were actually not used (mostly just placeholders in partial updates or something). The only reasonable approach was to ignore them.
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
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.
Problem Summary: SerDe from_olap_string, from_fe_string, and from_zonemap_string handled invalid scalar strings inconsistently: some supported types returned errors while others filled default fields. This change makes supported string deserialization paths fill type defaults consistently on parse failure.
ATTN: We used to write some values in zonemap that were considered unreasonable after upgrading to later versions, as these values were actually not used (mostly just placeholders in partial updates or something). The only reasonable approach was to ignore them.