github-actions[bot] commented on code in PR #65991:
URL: https://github.com/apache/doris/pull/65991#discussion_r3643543363
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/InsertOverwriteTableCommand.java:
##########
@@ -375,7 +375,8 @@ private void insertIntoPartitions(ConnectContext ctx,
StmtExecutor executor, Lis
false,
TPartialUpdateNewRowPolicy.APPEND,
sink.getDMLCommandType(),
- (LogicalPlan) (sink.child(0)));
+ (LogicalPlan) (sink.child(0)),
+ sink.getStaticPartitionKeyValues());
Review Comment:
[P1] Preserve the static target for a zero-row overwrite
`INSERT OVERWRITE TABLE t PARTITION(dt='2026-07-24') SELECT ... WHERE false`
must replace that named partition with an empty result. Here the map is
preserved only on the copied sink and later becomes per-row constants;
`HiveInsertCommandContext` gets no row-independent target. A planned
`PhysicalEmptyRelation` returns before transaction/finalization, while a
runtime-empty query emits no `THivePartitionUpdate` and `HMSTransaction`
synthesizes empty overwrite work only for unpartitioned tables. Both paths
leave the old partition intact. Please carry the canonical fully-static target
into Hive overwrite finalization and add an empty-result regression that
verifies sibling partitions are unchanged.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSink.java:
##########
@@ -690,11 +690,35 @@ private Plan
bindHiveTableSink(MatchingContext<UnboundHiveTableSink<Plan>> ctx)
+ "(input format: " + inputFormat + "). LZO tables are
read-only in Doris.");
}
+ // Get static partition columns if present:
+ // INSERT [OVERWRITE] TABLE t PARTITION(col='val', ...) SELECT ...
+ Map<String, Expression> staticPartitions =
sink.getStaticPartitionKeyValues();
+ Set<String> staticPartitionColNames = staticPartitions != null
+ ? staticPartitions.keySet()
+ : Sets.newHashSet();
+ Set<String> lowerStaticPartitionColNames = Sets.newHashSet();
+ for (String name : staticPartitionColNames) {
+ lowerStaticPartitionColNames.add(name.toLowerCase());
Review Comment:
[P2] Canonicalize static partition keys once
This lowercased set is not a safe identifier-resolution boundary.
`PARTITION(pt='a', PT='b')` produces two parser-map entries; both validate
here, then the case-insensitive `columnToOutput` map silently lets the latter
value replace the former. The zero-argument `toLowerCase()` calls are also
locale-dependent (for example, Turkish `ID` does not match metadata column
`id`). Resolve each key to the canonical Hive `Column` using locale-independent
matching, reject a second key resolving to the same column, and use that
canonical name for arity checks and aliases.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSink.java:
##########
@@ -715,15 +739,69 @@ private Plan
bindHiveTableSink(MatchingContext<UnboundHiveTableSink<Plan>> ctx)
Optional.empty(),
child);
// we need to insert all the columns of the target table
+ // (except the static partition columns which are supplied by the
PARTITION clause)
if (boundSink.getCols().size() != child.getOutput().size()) {
throw new AnalysisException("insert into cols should be
corresponding to the query output");
}
Map<String, NamedExpression> columnToOutput = getColumnToOutput(ctx,
table, false, false,
boundSink, child);
+
+ // For static partition columns, add constant expressions from the
PARTITION clause.
+ // This ensures every written row carries the static partition value,
so all rows
+ // land in the specified partition (and INSERT OVERWRITE only
overwrites that partition).
+ if (!staticPartitionColNames.isEmpty()) {
+ for (Map.Entry<String, Expression> entry :
staticPartitions.entrySet()) {
+ String colName = entry.getKey();
+ Expression valueExpr = entry.getValue();
+ Column column = table.getColumn(colName);
+ if (column != null) {
+ Expression castExpr = TypeCoercionUtils.castIfNotSameType(
Review Comment:
[P1] Keep overwrite semantics when the target is classified as new
A static overwrite of the default partition can append instead of replace.
For `PARTITION(p='')`, the BE compares the raw value `""` to cached HMS value
`__HIVE_DEFAULT_PARTITION__`, misses the existing partition, and emits `NEW`;
`HMSTransaction.finishInsertTable` then discovers the partition in HMS but
unconditionally converts that update to `INSERT_EXISTING`, even though this is
an overwrite. A stale partition-cache miss has the same result. Please
normalize values consistently, preserve overwrite intent when a `NEW` target
resolves to an existing HMS partition, and propagate the corrected mode to
cache refresh; add repeated empty-string and cache-miss overwrite coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/analyzer/UnboundTableSinkCreator.java:
##########
@@ -98,7 +98,7 @@ public static LogicalSink<? extends Plan>
createUnboundTableSink(List<String> na
Optional.empty(), plan);
} else if (curCatalog instanceof HMSExternalCatalog) {
return new UnboundHiveTableSink<>(nameParts, colNames, hints,
partitions,
Review Comment:
[P1] Include Hive in static-partition VALUES normalization
Adding the static map to the Hive sink is not enough for an inline `VALUES`
child. `InsertUtils.normalizePlanWithoutLock` removes static columns from the
implicit target schema only for Iceberg and MaxCompute, so for Hive table `(v,
dt)`, `PARTITION(dt='x') VALUES (1)` is checked against two columns and fails
with `Column count doesn't match value count` before `BindSink` runs. This
affects both INSERT and OVERWRITE without an explicit column list. Please
include Hive through a shared static-partition-sink path, using canonical key
matching, and add INSERT/OVERWRITE `VALUES` coverage.
##########
regression-test/suites/external_table_p0/hive/write/test_hive_write_static_partition.groovy:
##########
@@ -0,0 +1,270 @@
+// 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("test_hive_write_static_partition",
"p0,external,hive,external_docker,external_docker_hive") {
+
+ // Static partition insert:
+ // INSERT [OVERWRITE] TABLE t PARTITION(col='val', ...) SELECT ...
+ // The partition column value comes from the PARTITION clause instead of
the query output.
+ def testStaticPartitionWrite = { String catalog_name ->
+ String tableName = "hive_static_par_tbl"
+
+ hive_docker """ DROP TABLE IF EXISTS ${tableName}; """
+ hive_docker """
+ CREATE TABLE ${tableName} (
+ tag_value string,
+ user_id string,
+ ts int
+ )
+ PARTITIONED BY (ts_date string)
+ STORED AS parquet;
+ """
+ sql """ refresh catalog ${catalog_name}; """
+
+ // 1. INSERT INTO ... PARTITION(ts_date='2026-07-24') without listing
the partition column in SELECT.
+ // This used to fail with "insert into cols should be corresponding
to the query output".
+ sql """
+ INSERT INTO ${tableName} PARTITION(ts_date='2026-07-24')
+ SELECT 'tagA', 'u1', 100;
+ """
+ // 2. Append another row into the SAME partition.
+ sql """
+ INSERT INTO ${tableName} PARTITION(ts_date='2026-07-24')
+ SELECT 'tagB', 'u2', 200;
+ """
+ // 3. Insert into a DIFFERENT partition.
+ sql """
+ INSERT INTO ${tableName} PARTITION(ts_date='2026-07-25')
+ SELECT 'tagC', 'u3', 300;
+ """
+ sql """ refresh catalog ${catalog_name}; """
+
+ // Verify: 2 rows in partition 2026-07-24 and 1 row in 2026-07-25,
partition column filled correctly.
+ def all = sql """ select tag_value, user_id, ts, ts_date from
${tableName} order by ts_date, ts; """
Review Comment:
[P2] Use the regression golden-output contract
These deterministic result checks use `sql` plus `assertEquals`, but the
repository instructions require `qt`/`order_qt` and an auto-generated `.out`;
no result file is included in this PR. The suite also drops its tables after
execution even though test artifacts should be retained for failed-run
debugging (the pre-test drops are sufficient). Please convert the result checks
to uniquely named golden queries, generate/commit the `.out`, and remove the
post-test cleanup.
##########
regression-test/suites/external_table_p0/hive/write/test_hive_write_static_partition.groovy:
##########
@@ -0,0 +1,270 @@
+// 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("test_hive_write_static_partition",
"p0,external,hive,external_docker,external_docker_hive") {
+
+ // Static partition insert:
+ // INSERT [OVERWRITE] TABLE t PARTITION(col='val', ...) SELECT ...
+ // The partition column value comes from the PARTITION clause instead of
the query output.
+ def testStaticPartitionWrite = { String catalog_name ->
+ String tableName = "hive_static_par_tbl"
+
+ hive_docker """ DROP TABLE IF EXISTS ${tableName}; """
+ hive_docker """
+ CREATE TABLE ${tableName} (
+ tag_value string,
+ user_id string,
+ ts int
+ )
+ PARTITIONED BY (ts_date string)
+ STORED AS parquet;
+ """
+ sql """ refresh catalog ${catalog_name}; """
+
+ // 1. INSERT INTO ... PARTITION(ts_date='2026-07-24') without listing
the partition column in SELECT.
+ // This used to fail with "insert into cols should be corresponding
to the query output".
+ sql """
+ INSERT INTO ${tableName} PARTITION(ts_date='2026-07-24')
+ SELECT 'tagA', 'u1', 100;
+ """
+ // 2. Append another row into the SAME partition.
+ sql """
+ INSERT INTO ${tableName} PARTITION(ts_date='2026-07-24')
+ SELECT 'tagB', 'u2', 200;
+ """
+ // 3. Insert into a DIFFERENT partition.
+ sql """
+ INSERT INTO ${tableName} PARTITION(ts_date='2026-07-25')
+ SELECT 'tagC', 'u3', 300;
+ """
+ sql """ refresh catalog ${catalog_name}; """
+
+ // Verify: 2 rows in partition 2026-07-24 and 1 row in 2026-07-25,
partition column filled correctly.
+ def all = sql """ select tag_value, user_id, ts, ts_date from
${tableName} order by ts_date, ts; """
+ assertEquals(3, all.size())
+ assertEquals("tagA", all[0][0]); assertEquals("2026-07-24", all[0][3])
+ assertEquals("tagB", all[1][0]); assertEquals("2026-07-24", all[1][3])
+ assertEquals("tagC", all[2][0]); assertEquals("2026-07-25", all[2][3])
+
+ // Partition pruning should work on the statically written partition
value.
+ def p24 = sql """ select tag_value from ${tableName} where
ts_date='2026-07-24' order by ts; """
+ assertEquals(2, p24.size())
+
+ // 4. INSERT OVERWRITE a single partition: it should only replace
2026-07-24, leaving 2026-07-25 intact.
+ sql """
+ INSERT OVERWRITE TABLE ${tableName} PARTITION(ts_date='2026-07-24')
+ SELECT 'tagX', 'u9', 999;
+ """
+ sql """ refresh catalog ${catalog_name}; """
+
+ def afterOverwrite = sql """ select tag_value, user_id, ts, ts_date
from ${tableName} order by ts_date, ts; """
+ assertEquals(2, afterOverwrite.size())
+ // partition 2026-07-24 replaced by a single row
+ assertEquals("tagX", afterOverwrite[0][0]); assertEquals(999,
afterOverwrite[0][2])
+ assertEquals("2026-07-24", afterOverwrite[0][3])
+ // partition 2026-07-25 untouched
+ assertEquals("tagC", afterOverwrite[1][0]); assertEquals("2026-07-25",
afterOverwrite[1][3])
+
+ // 5. Static partition combined with an explicit (non-partition)
column list.
+ sql """
+ INSERT INTO ${tableName} (tag_value, user_id, ts)
PARTITION(ts_date='2026-07-26')
Review Comment:
[P1] Put `PARTITION` before the explicit column list
The active Nereids insert grammar is `tableName partitionSpec? ... cols?
query`, so this `INSERT INTO t (cols) PARTITION(...)` form fails at the
`PARTITION` token and never exercises explicit-column Hive binding. The same
reversed order is used by the two negative regression cases and
`HiveDDLAndDMLPlanTest`, so those assertions cannot reach the intended
`BindSink` error either. Please use `INSERT INTO t PARTITION(...) (cols) SELECT
...` in both test files and keep a successful explicit-column regression in
that supported order.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSink.java:
##########
@@ -715,15 +739,69 @@ private Plan
bindHiveTableSink(MatchingContext<UnboundHiveTableSink<Plan>> ctx)
Optional.empty(),
child);
// we need to insert all the columns of the target table
+ // (except the static partition columns which are supplied by the
PARTITION clause)
if (boundSink.getCols().size() != child.getOutput().size()) {
throw new AnalysisException("insert into cols should be
corresponding to the query output");
}
Map<String, NamedExpression> columnToOutput = getColumnToOutput(ctx,
table, false, false,
boundSink, child);
+
+ // For static partition columns, add constant expressions from the
PARTITION clause.
+ // This ensures every written row carries the static partition value,
so all rows
+ // land in the specified partition (and INSERT OVERWRITE only
overwrites that partition).
+ if (!staticPartitionColNames.isEmpty()) {
+ for (Map.Entry<String, Expression> entry :
staticPartitions.entrySet()) {
+ String colName = entry.getKey();
+ Expression valueExpr = entry.getValue();
+ Column column = table.getColumn(colName);
+ if (column != null) {
+ Expression castExpr = TypeCoercionUtils.castIfNotSameType(
+ valueExpr,
DataType.fromCatalogType(column.getType()));
+ columnToOutput.put(colName, new Alias(castExpr, colName));
+ }
+ }
+ }
+
LogicalProject<?> fullOutputProject =
getOutputProjectByCoercion(table.getFullSchema(), child, columnToOutput);
return boundSink.withChildAndUpdateOutput(fullOutputProject);
}
+ /**
+ * Validate static partition specification for Hive table.
+ * The specified columns must be partition columns of the target hive
table,
+ * and each partition value must be a literal.
+ */
+ private void validateHiveStaticPartition(Map<String, Expression>
staticPartitions, HMSExternalTable table) {
+ if (staticPartitions == null || staticPartitions.isEmpty()) {
+ return;
+ }
+ Set<String> partitionColNames = table.getPartitionColumnNames();
+ if (partitionColNames.isEmpty()) {
+ throw new AnalysisException(
+ String.format("Table %s is not a partitioned table, cannot
use static partition syntax",
+ table.getName()));
+ }
+ // build a case-insensitive view of partition column names
+ Set<String> lowerPartitionColNames = Sets.newHashSet();
+ for (String name : partitionColNames) {
+ lowerPartitionColNames.add(name.toLowerCase());
+ }
+ for (Map.Entry<String, Expression> entry :
staticPartitions.entrySet()) {
+ String partitionColName = entry.getKey();
+ Expression partitionValue = entry.getValue();
+ if
(!lowerPartitionColNames.contains(partitionColName.toLowerCase())) {
+ throw new AnalysisException(
+ String.format("Unknown partition column '%s' in table
'%s'. Available partition columns: %s",
+ partitionColName, table.getName(),
partitionColNames));
+ }
+ if (!(partitionValue instanceof Literal)) {
Review Comment:
[P2] Accept signed numeric literals
`PARTITION(p=-1)` is a valid constant specification, but the parser
represents unary minus as `Subtract(0, 1)`, not as a `Literal`. Because this
static map is not expression-analyzed, it reaches this `instanceof Literal`
check unchanged and is rejected, while `+1` passes. Please fold/normalize
signed numeric constants before validation (while still rejecting nonconstant
expressions), and cover negative integral and decimal values for INSERT and
OVERWRITE.
--
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]
