Mryange commented on PR #62582:
URL: https://github.com/apache/doris/pull/62582#issuecomment-4387152611
> Automated review conclusion: request changes.
>
> I did not add new inline comments because the blocking issues I verified
are already covered by existing inline review threads and should not be
duplicated. The unresolved blockers are:
`create_column_const_with_default_value()` now materializes column defaults
instead of the removed type-level defaults, which changes Array/Map/JSONB
default values in shared non-storage call sites; fixed partial update and
flexible partial update also persist the new empty/NULL complex defaults for
insert-after-delete rows instead of the previous synthetic defaults; and the
new Variant default unit-test expectation was already noted as inconsistent
with `ColumnVariant::insert_default()`/`ColumnVariant::get()`. The current PR
also reports BE UT (macOS) failed.
>
> Critical checkpoint conclusions:
>
> * Goal and proof: the PR aims to remove `IDataType::get_default()` and use
column default materialization consistently, but the implementation is not
behavior-preserving while the PR states no behavior change, and test/check
status does not prove correctness.
> * Scope/minimality: the API removal is broad but mechanically focused;
however, replacing shared helper/storage semantics without compatibility or
behavior acknowledgement is not safe.
> * Concurrency/lifecycle/config: no new concurrency, lifecycle, or
configuration concerns were found in the changed code.
> * Compatibility/data correctness: there is
user-visible/default-materialization behavior change for complex types and MoW
partial-update reinsert paths; this remains the main blocker.
> * Parallel paths: fixed partial update, flexible partial update, and
shared const-default helper paths were reviewed; the same semantic change
exists across them and has already been commented.
> * Testing: a new BE unit test was added and a regression `.out` changed,
but the PR checklist says no tests are needed, the current checks show BE UT
failure, and the regression diff itself demonstrates changed behavior.
> * Observability/performance/memory: no additional observability,
performance, or memory-tracking issues were identified for this refactor.
>
> User focus points: no additional user-provided review focus was supplied.
create_column_const_with_default_value 没有问题。
Array/Map/JSONB 的默认值变更是合理的。
请重新review。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
