github-actions[bot] commented on code in PR #64621:
URL: https://github.com/apache/doris/pull/64621#discussion_r3450831356
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/InsertUtils.java:
##########
@@ -311,6 +311,17 @@ private static Plan normalizePlanWithoutLock(LogicalPlan
plan, TableIf table,
// check the necessary conditions for partial updates
OlapTable olapTable = (OlapTable) table;
+ ConnectContext connectContext = ConnectContext.get();
+ if (connectContext != null
+ && olapTable.hasExpressionDefaultValue()
Review Comment:
This guard is firing before the existing code determines whether the sink is
actually going to remain a partial update. At this point every INSERT inherits
`enable_unique_key_partial_update` from the session, but the code below can
still turn it back off for cases such as no target column list, all columns
present, non-MoW/cluster-key tables, or missing only an auto-increment key.
With this ordering, a normal full insert like `SET
enable_unique_key_partial_update=true; INSERT INTO tbl_expr_default_mow VALUES
(...)` is rejected on an expression-default table even though line 333 would
have downgraded it to a normal upsert. Please move this check after the
partial-update eligibility/downgrade logic and only throw when
`isPartialUpdate()` is still true.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/CreateRoutineLoadInfo.java:
##########
@@ -440,6 +440,17 @@ private void checkDBTable(ConnectContext ctx) throws
AnalysisException {
if (isPartialUpdate && !((OlapTable)
table).getEnableUniqueKeyMergeOnWrite()) {
throw new AnalysisException("load by PARTIAL_COLUMNS is only
supported in unique table MoW");
}
+
+ if ((isPartialUpdate || uniqueKeyUpdateMode !=
TUniqueKeyUpdateMode.UPSERT)
Review Comment:
This only protects routine-load creation. The same unsafe partial-update
modes are still reachable through the other load planners: stream load
normalizes `UPDATE_FIXED_COLUMNS`/`UPDATE_FLEXIBLE_COLUMNS` in
`NereidsStreamLoadPlanner.plan()`, broker load plans fixed partial updates in
`NereidsLoadingTaskPlanner.plan()`, and an existing routine-load job can be
altered into a partial-update mode later. Those paths do not check
`hasExpressionDefaultValue()` or
`allow_partial_update_with_expression_default`, so they can still omit an
expression-default column and fall back to the DDL-time folded default value.
Please put the guard in the shared load planning paths as well, after any
downgrade to UPSERT, so all partial load/update modes are covered consistently.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/ColumnDef.java:
##########
@@ -314,6 +329,11 @@ public static void validateDefaultValue(Type type, String
defaultValue, DefaultV
Preconditions.checkArgument(type.isScalarType());
ScalarType scalarType = (ScalarType) type;
Review Comment:
This early return skips the existing bounded string validation below. A
literal default for `VARCHAR(3)` is rejected when it is longer than the target
length, but an expression default such as `DEFAULT concat('abcdef')` goes
through `validateDefaultValueExpressionSql()`, then
`computeRealDefaultValueForExpressionSql()` only casts/folds it. Because
`castIfNotSameType()` returns string-like expressions unchanged for string-like
targets, the folded value can be longer than the column allows. Please run the
folded expression literal through the same type-specific default validation, or
use a length-aware strict cast/check, and add a negative test for bounded
`CHAR`/`VARCHAR`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -4190,6 +4190,8 @@ public ColumnDefinition visitColumnDef(ColumnDefContext
ctx) {
defaultValue = Optional.of(DefaultValue.E_NUM_DEFAULT_VALUE);
} else if (ctx.BITMAP_EMPTY() != null) {
defaultValue =
Optional.of(DefaultValue.BITMAP_EMPTY_DEFAULT_VALUE);
+ } else if (ctx.defaultExpr != null) {
+ defaultValue =
Optional.of(DefaultValue.expressionSqlDefaultValue(ctx.defaultExpr.getText()));
Review Comment:
Please do not persist expression defaults with `ctx.defaultExpr.getText()`.
ANTLR's `getText()` concatenates tokens without hidden-channel whitespace, so
whitespace-sensitive expressions can be changed before they are validated and
folded. For example, `DEFAULT CAST('1' AS INT)` is recorded as something like
`CAST('1'ASINT)`, which no longer has the required `AS INT` token sequence when
`validateDefaultValueExpressionSql()` reparses it. This builder already has
`getOriginSql(ParserRuleContext)` for preserving the original source interval;
using that here would keep valid defaults such as `CAST`, `CASE`, and interval
expressions round-trippable.
##########
regression-test/suites/nereids_syntax_p0/default_value_expression.groovy:
##########
@@ -0,0 +1,106 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("nereids_default_value_expression", "p0") {
+ sql "SET enable_nereids_planner=true"
+ sql "SET enable_fallback_to_original_planner=false"
+
+ def tbl = "tbl_default_expr"
+
+ try {
+ sql "DROP TABLE IF EXISTS ${tbl}"
+ sql """
+ CREATE TABLE ${tbl} (
+ id INT NOT NULL,
+ d DATEV2 NOT NULL DEFAULT to_date(now()),
+ dt DATETIMEV2(3) NOT NULL DEFAULT now(3),
+ s STRING NOT NULL DEFAULT concat('a-', DATE_FORMAT(now(), '%M
%e, %Y'))
+ )
+ DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES ("replication_allocation" = "tag.location.default: 1");
+ """
+
+ sql "INSERT INTO ${tbl}(id) VALUES (1)"
+ sql "sync"
+ qt_default_expr_select "SELECT id, d = CURRENT_DATE, dt IS NOT NULL, s
= concat('a-', DATE_FORMAT(now(), '%M %e, %Y')) FROM ${tbl} ORDER BY id"
+ } finally {
Review Comment:
Please align this suite with the Doris regression-test rules from
`AGENTS.md`: ordinary test table names should be hardcoded instead of assigned
through `def`, and tables should be dropped before use rather than in `finally`
after the assertions. Dropping at the end removes the table state that is
useful when a failure needs to be debugged. This suite already has the initial
`DROP TABLE IF EXISTS`; please remove the final drops and use fixed table names
directly in the SQL.
--
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]
