github-actions[bot] commented on code in PR #67758:
URL: https://github.com/apache/doris/pull/67758#discussion_r4023462634
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSink.java:
##########
@@ -295,75 +294,63 @@ private Plan
bindOlapTableSink(MatchingContext<UnboundTableSink<Plan>> ctx) {
exprTranslator.createSyncMvWhereClause(), targetTableSlots);
}
- private LogicalProject<?> getOutputProjectByCoercion(List<Column>
tableSchema, LogicalPlan child,
- Map<String,
NamedExpression> columnToOutput) {
- List<NamedExpression> fullOutputExprs =
Utils.fastToImmutableList(columnToOutput.values());
+ private LogicalProject<?> getOutputProject(List<Column> tableSchema,
LogicalPlan child,
+ Map<String, NamedExpression>
columnToOutput) {
if (child instanceof LogicalOneRowRelation) {
- // remove default value slot in one row relation
+ // Remove default value slots after supplying the target columns'
defaults.
child = ((LogicalOneRowRelation)
child).withProjects(((LogicalOneRowRelation) child)
.getProjects().stream()
.filter(p -> !(p instanceof DefaultValueSlot))
.collect(ImmutableList.toImmutableList()));
}
- LogicalProject<?> fullOutputProject = new
LogicalProject<>(fullOutputExprs, child);
+ List<NamedExpression> outputExprs = Lists.newArrayList();
+ for (Column column : tableSchema) {
+ NamedExpression expression = columnToOutput.get(column.getName());
+ // Partial updates omit columns that will be filled by
SegmentWriter.
+ if (expression != null) {
+ outputExprs.add(expression);
+ }
+ }
+ return new LogicalProject<>(outputExprs, child);
+ }
- // add cast project
- List<NamedExpression> castExprs = Lists.newArrayList();
+ private boolean shouldTruncateString() {
ConnectContext connCtx = ConnectContext.get();
- final boolean truncateString = needTruncateStringWhenInsert
+ return needTruncateStringWhenInsert
&& (connCtx == null ||
connCtx.getSessionVariable().enableInsertValueAutoCast)
&& !SessionVariable.enableStrictCast();
- for (int i = 0; i < tableSchema.size(); ++i) {
- Column col = tableSchema.get(i);
- NamedExpression expr = columnToOutput.get(col.getName()); //
relative outputExpr
- if (expr == null) {
- // If `expr` is null, it means that the current load is a
partial update
- // and `col` should not be contained in the output of the sink
node so
- // we skip it.
- continue;
- }
- expr = expr.toSlot();
- DataType inputType = expr.getDataType();
- DataType targetType =
DataType.fromCatalogType(tableSchema.get(i).getType());
- Expression castExpr = expr;
- // TODO move string like type logic into
TypeCoercionUtils#castIfNotSameType
- if (isSourceAndTargetStringLikeType(inputType, targetType) &&
!inputType.equals(targetType)) {
- int sourceLength = ((CharacterType) inputType).getLen();
- int targetLength = ((CharacterType) targetType).getLen();
- if (sourceLength == targetLength) {
- castExpr = TypeCoercionUtils.castIfNotSameType(castExpr,
targetType);
- } else if (truncateString && targetLength >= 0
- && (sourceLength < 0 || sourceLength > targetLength)) {
- // sourceLength < 0 means the source is an unbounded
string like type
- // (e.g. text/string whose getLen() returns -1), which is
always longer
- // than a bounded char/varchar target and therefore needs
truncation.
- castExpr = new Substring(castExpr, Literal.of(1),
Literal.of(targetLength));
- } else if (targetType.isStringType()) {
- castExpr = new Cast(castExpr, StringType.INSTANCE);
- }
- } else {
- castExpr = coerceSinkExpression(castExpr, targetType);
- }
- if (castExpr instanceof NamedExpression) {
- castExprs.add(((NamedExpression) castExpr));
- } else {
- // use expr's original name as alias name
- // so that the LogicalPostFilter node in stream load can bind
its slot successfully
- castExprs.add(new Alias(castExpr, expr.getName()));
+ }
+
+ private Expression coerceColumnExpression(Expression expression, DataType
targetType) {
+ return coerceColumnExpression(expression, targetType,
shouldTruncateString());
+ }
+
+ @VisibleForTesting
+ static Expression coerceColumnExpression(Expression expression, DataType
targetType, boolean truncateString) {
+ DataType inputType = expression.getDataType();
+ if (isSourceAndTargetStringLikeType(inputType, targetType) &&
!inputType.equals(targetType)) {
+ int sourceLength = ((CharacterType) inputType).getLen();
+ int targetLength = ((CharacterType) targetType).getLen();
+ if (sourceLength == targetLength) {
+ return TypeCoercionUtils.castIfNotSameType(expression,
targetType);
+ } else if (truncateString && targetLength >= 0
Review Comment:
[P1] Make generated dependencies observe the string value BE will store
This condition still skips width coercion in cases where BE later accepts
and clips the target. Every Nereids Stream/Routine/Broker Load uses `new
BindSink(false)`, and a normal non-strict SQL INSERT also reaches this branch
when either `enable_insert_value_auto_cast=false` or `enable_strict_cast=true`.
For `'abcd'` into `a VARCHAR(2), d INT AS (length(a))`, FE therefore produces:
```text
OlapSink(a VARCHAR(2), d INT)
Project(a = raw, d = length(raw))
```
BE evaluates that project first, then `VTabletBlockConvertor` applies
non-strict clipping only to `a`, so the committed row is `(a='ab', d=4)`
instead of preserving `d=length(a)`. The generated chain `a VARCHAR(10), c
VARCHAR(2) AS (a), d AS length(c)` similarly becomes `(abcd, ab, 4)`.
The earlier thread covered the default SQL-INSERT settings, which current
head fixes. These load and session-policy paths still rely on later BE
conversion. Please make dependency substitution observe the storage-equivalent
clipped value without bypassing strict-mode rejection/filtering, and add
generated-dependency regressions for non-strict file loads plus VALUES/SELECT
inserts with both false gates.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSink.java:
##########
@@ -552,6 +541,9 @@ private static Map<String, NamedExpression>
getColumnToOutput(
boundExpression = ((Alias) boundExpression).child();
}
boundExpression = ExpressionUtils.replace(boundExpression,
replaceMap);
+ // Dependent generated columns must use the value converted to
this column's declared type.
+ boundExpression = coerceColumnExpression(boundExpression,
Review Comment:
[P1] Keep generated target-cast policy consistent through execution
This coercion now runs inside
`AutoCloseSessionVariable(column.getSessionVariables())`, so
`castIfNotSameType` validates the declared-column cast with the generated
column's persisted creation-time `enable_strict_cast`, not the current INSERT
setting. The emitted `Cast` is not a `NeedSessionVarGuard`, however, and BE
executes an ordinary cast with the request's runtime strictness. The default
fast `INSERT ... VALUES` analyzer also does not run `CheckCast` again after
this point.
For example, a table created with `enable_strict_cast=false` can legally
define `g DOUBLE AS (a)` over a `DATE a`. After switching to
`enable_strict_cast=true`, a default fast VALUES insert is now admitted using
the stored loose policy, even though the deleted outer coercion rejected
DATE-to-DOUBLE under the current strict policy:
```text
OlapSink(a DATE, g DOUBLE)
OneRow(a = DATE '2024-01-02',
g = CAST(DATE '2024-01-02' AS DOUBLE)) // admitted stored-loose,
executes current-strict
```
No later fast/batch VALUES rule rechecks this cast; the ordinary `Cast`
carries no persisted strictness to BE, which executes it with the current
request and can fail at runtime instead of at FE analysis. Please either apply
the target-column coercion after restoring the INSERT session while still
registering its result for dependent columns, or encode the persisted policy
through execution. Add cross-session fast and batch VALUES regressions.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSink.java:
##########
@@ -295,75 +294,63 @@ private Plan
bindOlapTableSink(MatchingContext<UnboundTableSink<Plan>> ctx) {
exprTranslator.createSyncMvWhereClause(), targetTableSlots);
}
- private LogicalProject<?> getOutputProjectByCoercion(List<Column>
tableSchema, LogicalPlan child,
- Map<String,
NamedExpression> columnToOutput) {
- List<NamedExpression> fullOutputExprs =
Utils.fastToImmutableList(columnToOutput.values());
+ private LogicalProject<?> getOutputProject(List<Column> tableSchema,
LogicalPlan child,
+ Map<String, NamedExpression>
columnToOutput) {
if (child instanceof LogicalOneRowRelation) {
- // remove default value slot in one row relation
+ // Remove default value slots after supplying the target columns'
defaults.
child = ((LogicalOneRowRelation)
child).withProjects(((LogicalOneRowRelation) child)
.getProjects().stream()
.filter(p -> !(p instanceof DefaultValueSlot))
.collect(ImmutableList.toImmutableList()));
}
- LogicalProject<?> fullOutputProject = new
LogicalProject<>(fullOutputExprs, child);
+ List<NamedExpression> outputExprs = Lists.newArrayList();
+ for (Column column : tableSchema) {
+ NamedExpression expression = columnToOutput.get(column.getName());
+ // Partial updates omit columns that will be filled by
SegmentWriter.
+ if (expression != null) {
+ outputExprs.add(expression);
+ }
+ }
+ return new LogicalProject<>(outputExprs, child);
+ }
- // add cast project
- List<NamedExpression> castExprs = Lists.newArrayList();
+ private boolean shouldTruncateString() {
ConnectContext connCtx = ConnectContext.get();
- final boolean truncateString = needTruncateStringWhenInsert
+ return needTruncateStringWhenInsert
&& (connCtx == null ||
connCtx.getSessionVariable().enableInsertValueAutoCast)
&& !SessionVariable.enableStrictCast();
- for (int i = 0; i < tableSchema.size(); ++i) {
- Column col = tableSchema.get(i);
- NamedExpression expr = columnToOutput.get(col.getName()); //
relative outputExpr
- if (expr == null) {
- // If `expr` is null, it means that the current load is a
partial update
- // and `col` should not be contained in the output of the sink
node so
- // we skip it.
- continue;
- }
- expr = expr.toSlot();
- DataType inputType = expr.getDataType();
- DataType targetType =
DataType.fromCatalogType(tableSchema.get(i).getType());
- Expression castExpr = expr;
- // TODO move string like type logic into
TypeCoercionUtils#castIfNotSameType
- if (isSourceAndTargetStringLikeType(inputType, targetType) &&
!inputType.equals(targetType)) {
- int sourceLength = ((CharacterType) inputType).getLen();
- int targetLength = ((CharacterType) targetType).getLen();
- if (sourceLength == targetLength) {
- castExpr = TypeCoercionUtils.castIfNotSameType(castExpr,
targetType);
- } else if (truncateString && targetLength >= 0
- && (sourceLength < 0 || sourceLength > targetLength)) {
- // sourceLength < 0 means the source is an unbounded
string like type
- // (e.g. text/string whose getLen() returns -1), which is
always longer
- // than a bounded char/varchar target and therefore needs
truncation.
- castExpr = new Substring(castExpr, Literal.of(1),
Literal.of(targetLength));
- } else if (targetType.isStringType()) {
- castExpr = new Cast(castExpr, StringType.INSTANCE);
- }
- } else {
- castExpr = coerceSinkExpression(castExpr, targetType);
- }
- if (castExpr instanceof NamedExpression) {
- castExprs.add(((NamedExpression) castExpr));
- } else {
- // use expr's original name as alias name
- // so that the LogicalPostFilter node in stream load can bind
its slot successfully
- castExprs.add(new Alias(castExpr, expr.getName()));
+ }
+
+ private Expression coerceColumnExpression(Expression expression, DataType
targetType) {
+ return coerceColumnExpression(expression, targetType,
shouldTruncateString());
+ }
+
+ @VisibleForTesting
+ static Expression coerceColumnExpression(Expression expression, DataType
targetType, boolean truncateString) {
+ DataType inputType = expression.getDataType();
+ if (isSourceAndTargetStringLikeType(inputType, targetType) &&
!inputType.equals(targetType)) {
+ int sourceLength = ((CharacterType) inputType).getLen();
+ int targetLength = ((CharacterType) targetType).getLen();
+ if (sourceLength == targetLength) {
+ return TypeCoercionUtils.castIfNotSameType(expression,
targetType);
+ } else if (truncateString && targetLength >= 0
+ && (sourceLength < 0 || sourceLength > targetLength)) {
+ // An unbounded source can also exceed a bounded CHAR/VARCHAR
column.
+ return new Substring(expression, Literal.of(1),
Literal.of(targetLength));
+ } else if (targetType.isStringType()) {
+ return new Cast(expression, StringType.INSTANCE);
}
+ return expression;
}
- if (!castExprs.equals(fullOutputExprs)) {
- fullOutputProject = new LogicalProject<Plan>(castExprs,
fullOutputProject);
- }
- return fullOutputProject;
+ return coerceSinkExpression(expression, targetType);
Review Comment:
[P1] Normalize bounded string leaves before exposing complex values to
dependencies
This fallback assumes a complex cast produces the value that the sink will
store, but bounded nested strings do not obey that assumption. BE treats
`ARRAY<VARCHAR(10)>` and `ARRAY<VARCHAR(2)>` as equal because
`DataTypeString::equals` ignores the declared length and
`DataTypeArray::equals` delegates to it, so the cast uses the identity wrapper.
With non-strict input into `a ARRAY<VARCHAR(10)>, c ARRAY<VARCHAR(2)> AS
(a), d INT AS (length(element_at(c, 1)))`, current head therefore evaluates:
```text
Project(c = CAST(a AS ARRAY<VARCHAR(2)>),
d = length(element_at(CAST(a AS ARRAY<VARCHAR(2)>), 1)))
```
Both expressions still see `'abcd'`, producing `d=4`. After the project
runs, `VTabletBlockConvertor` recursively clips only the stored `c` element to
`'ab'`, so the committed row violates `d=length(element_at(c,1))`. A direct
`ARRAY<VARCHAR(2)>` input with a dependent generated column has the same
ordering failure.
Please make the dependency expression reflect storage normalization
recursively for bounded string leaves in ARRAY/MAP/STRUCT values, while
retaining the raw sink value where strict filtering needs it. Add direct and
generated-chain non-strict complex-string regressions plus strict rejection
coverage.
--
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]
