diff --git a/be/src/format_v2/table_reader.h b/be/src/format_v2/table_reader.h index d6b61f1e398cde..8a4885cf75b3b5 100644 --- a/be/src/format_v2/table_reader.h +++ b/be/src/format_v2/table_reader.h @@ -1216,8 +1216,22 @@ class TableReader { if (const auto* array_type = typeid_cast(table_type.get())) { const auto& array_column = assert_cast(**column); ColumnPtr nested_column = array_column.get_data_ptr(); - RETURN_IF_ERROR( - _align_column_nullability(&nested_column, array_type->get_nested_type())); + NullMap descendant_parent_null_map; + // Collection entries use offset coordinates, so inherited row masks must be projected + // only when a required descendant can consume them. This avoids scratch proportional + // to all array entries for the common all-required schema. + const NullMap* descendant_parent_null_map_ptr = nullptr; + if (_requires_collection_parent_null_map( + nullable_parent_null_map, nested_column, array_type->get_nested_type(), + array_column.size(), array_column.get_offsets())) { + descendant_parent_null_map_ptr = + _project_collection_parent_null_map_for_hidden_entries( + nullptr, nullable_parent_null_map, array_column.size(), + array_column.get_offsets(), nested_column->size(), + &descendant_parent_null_map); + } + RETURN_IF_ERROR(_align_column_nullability(&nested_column, array_type->get_nested_type(), + descendant_parent_null_map_ptr)); *column = ColumnArray::create(nested_column, array_column.get_offsets_ptr()); return Status::OK(); } @@ -1225,8 +1239,25 @@ class TableReader { const auto& map_column = assert_cast(**column); ColumnPtr key_column = map_column.get_keys_ptr(); ColumnPtr value_column = map_column.get_values_ptr(); - RETURN_IF_ERROR(_align_column_nullability(&key_column, map_type->get_key_type())); - RETURN_IF_ERROR(_align_column_nullability(&value_column, map_type->get_value_type())); + NullMap descendant_parent_null_map; + const NullMap* descendant_parent_null_map_ptr = nullptr; + if (_requires_collection_parent_null_map(nullable_parent_null_map, key_column, + map_type->get_key_type(), map_column.size(), + map_column.get_offsets()) || + _requires_collection_parent_null_map(nullable_parent_null_map, value_column, + map_type->get_value_type(), map_column.size(), + map_column.get_offsets())) { + // Keys and values share offsets, so one projected mask safely covers both streams. + descendant_parent_null_map_ptr = + _project_collection_parent_null_map_for_hidden_entries( + nullptr, nullable_parent_null_map, map_column.size(), + map_column.get_offsets(), key_column->size(), + &descendant_parent_null_map); + } + RETURN_IF_ERROR(_align_column_nullability(&key_column, map_type->get_key_type(), + descendant_parent_null_map_ptr)); + RETURN_IF_ERROR(_align_column_nullability(&value_column, map_type->get_value_type(), + descendant_parent_null_map_ptr)); *column = ColumnMap::create(key_column, value_column, map_column.get_offsets_ptr()); return Status::OK(); } @@ -1491,27 +1522,107 @@ class TableReader { return column.get(); } + static bool _requires_parent_null_map_for_alignment(const ColumnPtr& column, + const DataTypePtr& table_type) { + DORIS_CHECK(column.get() != nullptr); + DORIS_CHECK(table_type != nullptr); + if (table_type->is_nullable()) { + const auto& nested_type = + assert_cast(*table_type).get_nested_type(); + if (const auto* nullable_column = check_and_get_column(*column)) { + return _requires_parent_null_map_for_alignment( + nullable_column->get_nested_column_ptr(), nested_type); + } + return _requires_parent_null_map_for_alignment(column, nested_type); + } + if (const auto* nullable_column = check_and_get_column(*column)) { + if (nullable_column->has_null()) { + return true; + } + return _requires_parent_null_map_for_alignment(nullable_column->get_nested_column_ptr(), + table_type); + } + if (const auto* array_type = typeid_cast(table_type.get())) { + const auto& array_column = assert_cast(*column); + return _requires_parent_null_map_for_alignment(array_column.get_data_ptr(), + array_type->get_nested_type()); + } + if (const auto* map_type = typeid_cast(table_type.get())) { + const auto& map_column = assert_cast(*column); + return _requires_parent_null_map_for_alignment(map_column.get_keys_ptr(), + map_type->get_key_type()) || + _requires_parent_null_map_for_alignment(map_column.get_values_ptr(), + map_type->get_value_type()); + } + if (const auto* struct_type = typeid_cast(table_type.get())) { + const auto& struct_column = assert_cast(*column); + DORIS_CHECK(struct_column.tuple_size() == struct_type->get_elements().size()); + for (size_t i = 0; i < struct_column.tuple_size(); ++i) { + if (_requires_parent_null_map_for_alignment(struct_column.get_column_ptr(i), + struct_type->get_element(i))) { + return true; + } + } + } + return false; + } + + static bool _requires_collection_parent_null_map(const NullMap* parent_null_map, + const ColumnPtr& column, + const DataTypePtr& table_type) { + // Descendant null maps can be entry-sized. Scan them only when an inherited mask can + // actually hide a row; absent/all-clear masks cannot authorize any physical child NULL. + if (parent_null_map == nullptr || + std::ranges::none_of(*parent_null_map, [](const auto value) { return value != 0; })) { + return false; + } + return _requires_parent_null_map_for_alignment(column, table_type); + } + template - static const NullMap* _project_collection_parent_null_map( - const NullMap* container_null_map, const NullMap* ancestor_null_map, const size_t rows, - const Offsets& offsets, const size_t child_rows, NullMap* const projected_null_map) { + static bool _parent_null_map_hides_collection_entries(const NullMap* container_null_map, + const NullMap* ancestor_null_map, + const size_t rows, + const Offsets& offsets) { if (container_null_map == nullptr && ancestor_null_map == nullptr) { - return nullptr; + return false; } DORIS_CHECK(container_null_map == nullptr || container_null_map->size() == rows); DORIS_CHECK(ancestor_null_map == nullptr || ancestor_null_map->size() == rows); DORIS_CHECK(offsets.size() == rows); - bool has_hidden_row = false; + size_t begin = 0; for (size_t row = 0; row < rows; ++row) { - if ((container_null_map != nullptr && (*container_null_map)[row]) || - (ancestor_null_map != nullptr && (*ancestor_null_map)[row])) { - has_hidden_row = true; - break; + const size_t end = offsets[row]; + const bool hidden = (container_null_map != nullptr && (*container_null_map)[row]) || + (ancestor_null_map != nullptr && (*ancestor_null_map)[row]); + // A hidden collection row protects descendants only when its offset span is nonempty. + if (hidden && end > begin) { + return true; } + begin = end; + } + return false; + } + + template + static bool _requires_collection_parent_null_map(const NullMap* parent_null_map, + const ColumnPtr& column, + const DataTypePtr& table_type, + const size_t rows, const Offsets& offsets) { + if (!_parent_null_map_hides_collection_entries(nullptr, parent_null_map, rows, offsets)) { + return false; } - if (!has_hidden_row) { + return _requires_parent_null_map_for_alignment(column, table_type); + } + + template + static const NullMap* _project_collection_parent_null_map_for_hidden_entries( + const NullMap* container_null_map, const NullMap* ancestor_null_map, const size_t rows, + const Offsets& offsets, const size_t child_rows, NullMap* const projected_null_map) { + if (!_parent_null_map_hides_collection_entries(container_null_map, ancestor_null_map, rows, + offsets)) { // Nullable collection wrappers expose a null-map even when every row is present; avoid - // allocating entry-coordinate scratch proportional to a potentially huge collection. + // allocating entry-coordinate scratch unless a hidden row owns physical entries. return nullptr; } projected_null_map->resize(child_rows); @@ -1656,9 +1767,10 @@ class TableReader { // storage invariant, so add it only at the materialization boundary. element_mapping.table_type = make_nullable(element_mapping.table_type); NullMap descendant_parent_null_map; - const NullMap* descendant_parent_null_map_ptr = _project_collection_parent_null_map( - parent_null_map, nullable_parent_null_map, rows, file_array->get_offsets(), - nested_column->size(), &descendant_parent_null_map); + const NullMap* descendant_parent_null_map_ptr = + _project_collection_parent_null_map_for_hidden_entries( + parent_null_map, nullable_parent_null_map, rows, file_array->get_offsets(), + nested_column->size(), &descendant_parent_null_map); RETURN_IF_ERROR(_materialize_present_child_mapping_column( element_mapping, nested_column, nested_column->size(), &nested_column, descendant_parent_null_map_ptr)); @@ -1710,9 +1822,10 @@ class TableReader { ColumnPtr value_column = file_map->get_values_ptr(); DORIS_CHECK(key_column->size() == value_column->size()); NullMap descendant_parent_null_map; - const NullMap* descendant_parent_null_map_ptr = _project_collection_parent_null_map( - parent_null_map, nullable_parent_null_map, rows, file_map->get_offsets(), - key_column->size(), &descendant_parent_null_map); + const NullMap* descendant_parent_null_map_ptr = + _project_collection_parent_null_map_for_hidden_entries( + parent_null_map, nullable_parent_null_map, rows, file_map->get_offsets(), + key_column->size(), &descendant_parent_null_map); const ColumnMapping* key_mapping = nullptr; const ColumnMapping* value_mapping = nullptr; diff --git a/be/test/format_v2/table_reader_test.cpp b/be/test/format_v2/table_reader_test.cpp index d879570852f37c..768472b1e2a3b7 100644 --- a/be/test/format_v2/table_reader_test.cpp +++ b/be/test/format_v2/table_reader_test.cpp @@ -976,7 +976,9 @@ class TableReaderCastTestHelper final : public TableReader { using TableReader::_materialize_map_mapping_column; using TableReader::_materialize_present_child_mapping_column; using TableReader::_materialize_struct_mapping_column; - using TableReader::_project_collection_parent_null_map; + using TableReader::_project_collection_parent_null_map_for_hidden_entries; + using TableReader::_requires_collection_parent_null_map; + using TableReader::_requires_parent_null_map_for_alignment; }; TEST(TableReaderTest, TruncateCharOrVarcharPredicateOnlyAppliesToParquetStringWidthMismatch) { @@ -3219,9 +3221,52 @@ TEST(TableReaderTest, CollectionParentMaskFastPathSkipsEntryScratchForClearMasks ColumnArray::Offsets64 offsets {500000, 1000000}; NullMap projected_null_map; - const auto* result = TableReaderCastTestHelper::_project_collection_parent_null_map( - &container_null_map, &ancestor_null_map, 2, offsets, offsets.back(), - &projected_null_map); + const auto* result = + TableReaderCastTestHelper::_project_collection_parent_null_map_for_hidden_entries( + &container_null_map, &ancestor_null_map, 2, offsets, offsets.back(), + &projected_null_map); + + EXPECT_EQ(nullptr, result); + EXPECT_TRUE(projected_null_map.empty()); +} + +TEST(TableReaderTest, CollectionParentMaskSkipsLargeArrayWhenOnlyEmptyRowIsHidden) { + constexpr size_t entries = 500000; + auto values = ColumnInt32::create(entries, 0); + auto value_null_map = ColumnUInt8::create(entries, 0); + value_null_map->get_data().back() = 1; + ColumnPtr nullable_values = + ColumnNullable::create(std::move(values), std::move(value_null_map)); + NullMap parent_null_map {1, 0}; + ColumnArray::Offsets64 offsets {0, entries}; + NullMap projected_null_map; + + EXPECT_FALSE(TableReaderCastTestHelper::_requires_collection_parent_null_map( + &parent_null_map, nullable_values, std::make_shared(), 2, offsets)); + const auto* result = + TableReaderCastTestHelper::_project_collection_parent_null_map_for_hidden_entries( + nullptr, &parent_null_map, 2, offsets, entries, &projected_null_map); + + EXPECT_EQ(nullptr, result); + EXPECT_TRUE(projected_null_map.empty()); +} + +TEST(TableReaderTest, CollectionParentMaskSkipsLargeMapWhenOnlyEmptyRowIsHidden) { + constexpr size_t entries = 500000; + auto values = ColumnInt32::create(entries, 0); + auto value_null_map = ColumnUInt8::create(entries, 0); + value_null_map->get_data().back() = 1; + ColumnPtr nullable_values = + ColumnNullable::create(std::move(values), std::move(value_null_map)); + NullMap parent_null_map {1, 0}; + ColumnArray::Offsets64 offsets {0, entries}; + NullMap projected_null_map; + + EXPECT_FALSE(TableReaderCastTestHelper::_requires_collection_parent_null_map( + &parent_null_map, nullable_values, std::make_shared(), 2, offsets)); + const auto* result = + TableReaderCastTestHelper::_project_collection_parent_null_map_for_hidden_entries( + nullptr, &parent_null_map, 2, offsets, entries, &projected_null_map); EXPECT_EQ(nullptr, result); EXPECT_TRUE(projected_null_map.empty()); @@ -5020,6 +5065,112 @@ TEST(TableReaderTest, ArrayElementMaterializationPreservesNullMap) { EXPECT_EQ(result_strings.get_data_at(2).to_string(), "doris-nereids-5"); } +TEST(TableReaderTest, TrivialArrayChildProjectsNullableStructParentMask) { + const auto int_type = std::make_shared(); + const auto element_struct_type = + std::make_shared(DataTypes {int_type}, Strings {"value"}); + const auto nullable_element_struct_type = make_nullable(element_struct_type); + const auto array_type = std::make_shared(nullable_element_struct_type); + const auto table_struct_type = make_nullable(std::make_shared( + DataTypes {array_type, int_type}, Strings {"items", "added"})); + const auto file_struct_type = make_nullable( + std::make_shared(DataTypes {array_type}, Strings {"items"})); + + auto table_items = make_table_column(0, "items", array_type); + table_items.type = array_type; + auto table_element = make_table_column(0, "element", element_struct_type); + table_element.type = element_struct_type; + auto table_value = make_table_column(0, "value", int_type); + table_value.type = int_type; + table_element.children = {table_value}; + table_items.children = {table_element}; + auto table_added = make_table_column(1, "added", int_type); + table_added.type = int_type; + auto table_struct = make_table_column(0, "payload", table_struct_type); + table_struct.type = table_struct_type; + table_struct.children = {table_items, table_added}; + + auto file_items = make_file_column(0, "items", array_type); + file_items.type = array_type; + auto file_element = make_file_column(0, "element", element_struct_type); + file_element.type = element_struct_type; + auto file_value = make_file_column(0, "value", int_type); + file_value.type = int_type; + file_element.children = {file_value}; + file_items.children = {file_element}; + auto file_struct = make_file_column(0, "payload", file_struct_type); + file_struct.type = file_struct_type; + file_struct.children = {file_items}; + + TableColumnMapper mapper({.mode = TableColumnMappingMode::BY_NAME}); + ASSERT_TRUE(mapper.create_mapping({table_struct}, {}, {file_struct}).ok()); + ASSERT_EQ(mapper.mappings().size(), 1); + ASSERT_FALSE(mapper.mappings()[0].is_trivial); + ASSERT_TRUE(mapper.mappings()[0].child_mappings[0].is_trivial); + + auto values = ColumnInt32::create(); + values->get_data().assign({0, 7}); + auto value_null_map = ColumnUInt8::create(); + value_null_map->get_data().assign({1, 0}); + MutableColumns element_children; + element_children.push_back( + ColumnNullable::create(std::move(values), std::move(value_null_map))); + auto element_null_map = ColumnUInt8::create(2, 0); + auto array_values = ColumnNullable::create(ColumnStruct::create(std::move(element_children)), + std::move(element_null_map)); + auto offsets = ColumnArray::ColumnOffsets::create(); + offsets->get_data().assign({1, 2}); + MutableColumns struct_children; + struct_children.push_back(ColumnArray::create(std::move(array_values), std::move(offsets))); + auto parent_null_map = ColumnUInt8::create(); + parent_null_map->get_data().assign({1, 0}); + ColumnPtr file_data = ColumnNullable::create(ColumnStruct::create(std::move(struct_children)), + std::move(parent_null_map)); + + TableReaderCastTestHelper reader; + ColumnPtr result; + const auto status = + reader._materialize_struct_mapping_column(mapper.mappings()[0], file_data, 2, &result); + ASSERT_TRUE(status.ok()) << status.to_string(); + const auto& result_struct = assert_cast( + assert_cast(*result).get_nested_column()); + const auto& result_array = assert_cast(result_struct.get_column(0)); + const auto& result_elements = assert_cast(result_array.get_data()); + const auto& result_element_struct = + assert_cast(result_elements.get_nested_column()); + ASSERT_FALSE(result_element_struct.get_column(0).is_nullable()); + EXPECT_EQ(assert_cast(result_element_struct.get_column(0)).get_element(1), + 7); +} + +TEST(TableReaderTest, ParentMaskProjectionOnlyWhenRequiredDescendantCanConsumeIt) { + const auto int_type = std::make_shared(); + auto mutable_values = ColumnInt32::create(); + mutable_values->get_data().assign({1, 2}); + ColumnPtr values = std::move(mutable_values); + EXPECT_FALSE( + TableReaderCastTestHelper::_requires_parent_null_map_for_alignment(values, int_type)); + + ColumnPtr nullable_values = ColumnNullable::create(values->clone(), ColumnUInt8::create(2, 0)); + EXPECT_FALSE(TableReaderCastTestHelper::_requires_parent_null_map_for_alignment(nullable_values, + int_type)); + auto null_map = ColumnUInt8::create(2, 0); + null_map->get_data()[0] = 1; + ColumnPtr nullable_values_with_null = + ColumnNullable::create(values->clone(), std::move(null_map)); + EXPECT_TRUE(TableReaderCastTestHelper::_requires_parent_null_map_for_alignment( + nullable_values_with_null, int_type)); + + NullMap all_clear_parent_mask(2, 0); + EXPECT_FALSE(TableReaderCastTestHelper::_requires_collection_parent_null_map( + &all_clear_parent_mask, nullable_values_with_null, int_type)); + NullMap hidden_parent_mask {1, 0}; + EXPECT_TRUE(TableReaderCastTestHelper::_requires_collection_parent_null_map( + &hidden_parent_mask, nullable_values_with_null, int_type)); + EXPECT_FALSE(TableReaderCastTestHelper::_requires_collection_parent_null_map( + nullptr, nullable_values_with_null, int_type)); +} + TEST(TableReaderTest, CreateScanRequestPromotesProjectedColumnToPredicateColumn) { const auto int_type = std::make_shared(); const std::vector projected_columns = { diff --git a/fe/fe-common/src/main/java/org/apache/doris/common/util/SqlUtils.java b/fe/fe-common/src/main/java/org/apache/doris/common/util/SqlUtils.java index ff867e529b784d..ffe92ffe830fc6 100644 --- a/fe/fe-common/src/main/java/org/apache/doris/common/util/SqlUtils.java +++ b/fe/fe-common/src/main/java/org/apache/doris/common/util/SqlUtils.java @@ -54,6 +54,12 @@ public static String escapeQuota(String str) { return str.replaceAll("\"", "\\\\\""); } + /** Quote a value as a SQL string literal under the requested backslash-escape mode. */ + public static String quoteStringLiteral(String value, boolean noBackslashEscapes) { + String escaped = noBackslashEscapes ? value : value.replace("\\", "\\\\"); + return "\"" + escaped.replace("\"", "\"\"") + "\""; + } + public static List splitMultiStmts(String sql) { DorisSqlSeparatorLexer lexer = new DorisSqlSeparatorLexer(CharStreams.fromString(sql)); CommonTokenStream tokenStream = new CommonTokenStream(lexer); diff --git a/fe/fe-connector/fe-connector-api/src/main/java/org/apache/doris/connector/api/ConnectorTableSchema.java b/fe/fe-connector/fe-connector-api/src/main/java/org/apache/doris/connector/api/ConnectorTableSchema.java index 54a002701129e5..4a6c8b60e46dc6 100644 --- a/fe/fe-connector/fe-connector-api/src/main/java/org/apache/doris/connector/api/ConnectorTableSchema.java +++ b/fe/fe-connector/fe-connector-api/src/main/java/org/apache/doris/connector/api/ConnectorTableSchema.java @@ -106,13 +106,16 @@ public final class ConnectorTableSchema { private final String tableFormatType; private final Map properties; private final Set tableCapabilities; + // Opaque connector generation captured by the same remote load as columns. Write binding carries this + // value forward so a later planning load can validate rather than silently replace the bind-time baseline. + private final String writeMetadataIdentity; /** For a connector whose tables all have the same capabilities — the per-table set is empty. */ public ConnectorTableSchema(String tableName, List columns, String tableFormatType, Map properties) { - this(tableName, columns, tableFormatType, properties, Collections.emptySet()); + this(tableName, columns, tableFormatType, properties, Collections.emptySet(), null); } /** For a connector that refines its capabilities per table — see {@link #getTableCapabilities()}. */ @@ -121,6 +124,18 @@ public ConnectorTableSchema(String tableName, String tableFormatType, Map properties, Set tableCapabilities) { + this(tableName, columns, tableFormatType, properties, tableCapabilities, null); + } + + /** + * Builds a schema with the connector's opaque write generation captured from the same table load. + */ + public ConnectorTableSchema(String tableName, + List columns, + String tableFormatType, + Map properties, + Set tableCapabilities, + String writeMetadataIdentity) { this.tableName = Objects.requireNonNull(tableName, "tableName"); this.columns = columns == null ? Collections.emptyList() @@ -132,6 +147,7 @@ public ConnectorTableSchema(String tableName, this.tableCapabilities = tableCapabilities == null || tableCapabilities.isEmpty() ? Collections.emptySet() : Collections.unmodifiableSet(EnumSet.copyOf(tableCapabilities)); + this.writeMetadataIdentity = writeMetadataIdentity; } public String getTableName() { @@ -170,6 +186,11 @@ public Set getTableCapabilities() { return tableCapabilities; } + /** Opaque write generation captured together with {@link #getColumns()}, or {@code null}. */ + public String getWriteMetadataIdentity() { + return writeMetadataIdentity; + } + @Override public boolean equals(Object o) { if (this == o) { @@ -183,12 +204,14 @@ public boolean equals(Object o) { && columns.equals(that.columns) && Objects.equals(tableFormatType, that.tableFormatType) && properties.equals(that.properties) - && tableCapabilities.equals(that.tableCapabilities); + && tableCapabilities.equals(that.tableCapabilities) + && Objects.equals(writeMetadataIdentity, that.writeMetadataIdentity); } @Override public int hashCode() { - return Objects.hash(tableName, columns, tableFormatType, properties, tableCapabilities); + return Objects.hash(tableName, columns, tableFormatType, properties, tableCapabilities, + writeMetadataIdentity); } @Override diff --git a/fe/fe-connector/fe-connector-api/src/main/java/org/apache/doris/connector/api/handle/ConnectorWriteHandle.java b/fe/fe-connector/fe-connector-api/src/main/java/org/apache/doris/connector/api/handle/ConnectorWriteHandle.java index f04a9e28206101..b1d8efc443bfe0 100644 --- a/fe/fe-connector/fe-connector-api/src/main/java/org/apache/doris/connector/api/handle/ConnectorWriteHandle.java +++ b/fe/fe-connector/fe-connector-api/src/main/java/org/apache/doris/connector/api/handle/ConnectorWriteHandle.java @@ -41,6 +41,18 @@ public interface ConnectorWriteHandle { /** The columns being written, ordered to match the INSERT column list. */ List getColumns(); + /** + * The complete target schema captured when this write was bound, in target-schema order. + * + *

This is deliberately separate from {@link #getColumns()}: an INSERT column list and static + * partitions can make the write list a subset even though schema-drift validation must compare the + * complete bound schema. The default preserves compatibility for handles that already carry a full + * write list.

+ */ + default List getBoundTargetColumns() { + return getColumns(); + } + /** Whether this is an INSERT OVERWRITE. */ boolean isOverwrite(); @@ -84,6 +96,11 @@ default TSortInfo getSortInfo() { return null; } + /** Metadata identity captured when the engine bound the physical write plan, or {@code null}. */ + default String getBoundWriteMetadataIdentity() { + return null; + } + /** * Whether the statement behind this write is a SQL {@code MERGE INTO} whose cardinality rule the sink * must enforce: a target row matched by more than one source row is an error, and the connector's BE diff --git a/fe/fe-connector/fe-connector-api/src/main/java/org/apache/doris/connector/api/write/ConnectorWritePlanProvider.java b/fe/fe-connector/fe-connector-api/src/main/java/org/apache/doris/connector/api/write/ConnectorWritePlanProvider.java index d4bc3c86bffe4f..a8d3710dc12fb0 100644 --- a/fe/fe-connector/fe-connector-api/src/main/java/org/apache/doris/connector/api/write/ConnectorWritePlanProvider.java +++ b/fe/fe-connector/fe-connector-api/src/main/java/org/apache/doris/connector/api/write/ConnectorWritePlanProvider.java @@ -98,6 +98,25 @@ default List getWriteSortColumns(ConnectorSession sess return null; } + /** + * Resolves write-sort positions against the bind-time target schema. Connectors with stable field + * identities should override this form; the default preserves existing name/ordinal behavior. + */ + default List getWriteSortColumns(ConnectorSession session, + ConnectorTableHandle tableHandle, List boundTargetColumns) { + return getWriteSortColumns(session, tableHandle); + } + + /** + * Returns an opaque identity for metadata that shapes the physical write plan. The engine captures it + * while binding the sink and returns it through {@link ConnectorWriteHandle}; connectors can reject the + * write if a later metadata refresh would make that physical plan stale. Default: {@code null} when the + * connector has no such metadata fence. + */ + default String getWriteMetadataIdentity(ConnectorSession session, ConnectorTableHandle tableHandle) { + return null; + } + /** * Declares the target's write-time partitioning, in an engine-neutral form, so the engine can reproduce * the connector's write distribution (the iceberg merge-write {@code DistributionSpecMerge}) without diff --git a/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorMetadata.java b/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorMetadata.java index 62e98dc2a0a048..ff0e3740ae715d 100644 --- a/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorMetadata.java +++ b/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorMetadata.java @@ -549,7 +549,10 @@ private ConnectorTableSchema buildTableSchema(String tableName, Table table, Sch } } - return new ConnectorTableSchema(tableName, columns, "ICEBERG", tableProps); + // Capture the write identity from this exact Table object so the schema-cache generation and the + // later beginWrite fence cannot straddle a drop/recreate or metadata commit. + return new ConnectorTableSchema(tableName, columns, "ICEBERG", tableProps, + Collections.emptySet(), IcebergWritePlanProvider.writeMetadataIdentity(table)); } /** @@ -1693,7 +1696,7 @@ public Optional getSysTableHandle(ConnectorSession session IcebergTableHandle base = (IcebergTableHandle) baseTableHandle; return Optional.of(IcebergTableHandle.forSystemTable( base.getDbName(), base.getTableName(), sys, - base.getSnapshotId(), base.getRef(), base.getSchemaId())); + base.getSnapshotId(), base.getRef(), base.getSchemaId(), base.isSnapshotResolved())); } /** @@ -1847,7 +1850,8 @@ public boolean supportsCastPredicatePushdown(ConnectorSession session) { * remote PARTITIONS scan runs inside the FE-injected auth context. * *

The partition set + freshness are enumerated at the handle's pinned snapshot when present - * ({@code iceHandle.getSnapshotId() >= 0}), else the table's latest snapshot. The generic model (3/3) must + * ({@code iceHandle.getSnapshotId() >= 0}), remain empty for an explicitly resolved-empty handle, or else + * use the table's latest snapshot. The generic model (3/3) must * thread the query's pin onto the handle (via {@code applySnapshot} with {@code beginQuerySnapshot}'s * snapshot) before calling this, so the MTMV partition/freshness view stays consistent with the data-scan * pin — mirroring master, which routes enumeration, freshness and the scan through ONE snapshot cache value.

@@ -1866,10 +1870,11 @@ public Optional getMvccPartitionView( // function of the pinned MVCC coordinate (a new snapshot/schema yields a new key, never a stale hit). // The lookup sits INSIDE executeAuthenticated so a miss runs the loader (resolveTableForRead + the // remote PARTITIONS build) under the FE-injected auth scope; a hit returns without any remote call. A - // null cache (session=user / no-cache catalog) computes directly every call. -1 (empty table / unpinned) - // enumerates the current snapshot and caches a trivially-empty view (harmless; REFRESH re-pins). + // null cache (session=user / no-cache catalog) computes directly every call. A resolved-empty -1 + // bypasses cache A because its numeric key is otherwise indistinguishable from an unresolved latest + // read, even though only the former is a query-begin MVCC boundary. return context.executeAuthenticated(() -> { - if (mvccPartitionViewCache == null) { + if (mvccPartitionViewCache == null || iceHandle.isResolvedEmptySnapshot()) { return Optional.of(buildMvccPartitionViewUncached(session, iceHandle)); } ConnectorTableKey key = new ConnectorTableKey(iceHandle.getDbName(), @@ -1892,6 +1897,11 @@ public Optional getMvccPartitionView( private ConnectorMvccPartitionView buildMvccPartitionViewUncached( ConnectorSession session, IcebergTableHandle iceHandle) { Table table = resolveTableForRead(session, iceHandle); + if (iceHandle.isResolvedEmptySnapshot()) { + // Data rows and partition freshness must describe the same query-begin generation; a concurrent + // first append may already be visible through this live Table object but not through the empty pin. + return IcebergPartitionUtils.buildResolvedEmptyMvccPartitionView(table); + } return IcebergPartitionUtils.buildMvccPartitionView(table, iceHandle.getSnapshotId(), TableIdentifier.of(iceHandle.getDbName(), iceHandle.getTableName()), partitionCache); } @@ -2128,8 +2138,8 @@ private long parseTimestampMillis(ConnectorSession session, ConnectorTimeTravelS * Threads a resolved MVCC / time-travel pin onto the handle BEFORE the scan reads it (the generic * {@code PluginDrivenScanNode} calls this via {@code applyMvccSnapshotPin}). Reads the typed * {@code snapshotId}/{@code schemaId} and the {@code iceberg.scan.ref} property; an empty-table / query-begin - * latest pin ({@code snapshotId<0} and no ref) returns the handle UNCHANGED (read latest — a - * {@code useSnapshot(-1)} would be a non-existent snapshot; mirrors paimon's {@code -1} guard). + * latest pin ({@code snapshotId<0} and no ref) remains unpinned for scanning because + * {@code useSnapshot(-1)} would be invalid, but is recorded as an explicitly resolved empty snapshot. */ @Override public ConnectorTableHandle applySnapshot(ConnectorSession session, @@ -2140,9 +2150,6 @@ public ConnectorTableHandle applySnapshot(ConnectorSession session, } String ref = snapshot.getProperties().get(REF_PROPERTY); long snapshotId = snapshot.getSnapshotId(); - if (snapshotId < 0 && ref == null) { - return iceHandle; - } return iceHandle.withSnapshot(snapshotId, ref, snapshot.getSchemaId()); } diff --git a/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorTransaction.java b/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorTransaction.java index 666e59a8741f8e..a853896d0f1404 100644 --- a/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorTransaction.java +++ b/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorTransaction.java @@ -211,8 +211,13 @@ public void beginWrite(ConnectorSession session, String db, String tableName, Ic Table loaded = IcebergStatementScope.sharedWritableTable(session, db, tableName, () -> catalogOps.loadTable(db, tableName)); this.table = loaded; + validateBoundWriteGeneration(ctx, loaded); applyBeginGuards(ctx, tableName); - this.transaction = openTransaction(loaded); + Transaction opened = openTransaction(loaded); + // BaseTable.newTransaction refreshes metadata. Fence that second load too, closing the + // drop/recreate race between the initial generation check and transaction construction. + validateBoundWriteGeneration(ctx, loaded); + this.transaction = opened; return null; }); } catch (Exception e) { @@ -225,6 +230,17 @@ public void beginWrite(ConnectorSession session, String db, String tableName, Ic } } + private static void validateBoundWriteGeneration(IcebergWriteContext ctx, Table loaded) { + String boundIdentity = ctx.getBoundWriteMetadataIdentity(); + // Reject a same-name replacement before opening its SDK transaction; the replacement must never + // become the conflict baseline for a plan whose rows and defaults were bound against the old UUID. + if (boundIdentity != null + && !boundIdentity.equals(IcebergWritePlanProvider.writeMetadataIdentity(loaded))) { + throw new IllegalArgumentException( + "Iceberg write metadata changed after the write was bound; retry the statement"); + } + } + /** * Opens the SDK transaction for {@code loaded}. On a Kerberos catalog the table's {@link FileIO} is wrapped * in a plugin-side Kerberos {@code doAs} ({@link IcebergAuthenticatedFileIO}); otherwise this is byte-for-byte @@ -278,13 +294,14 @@ private void applyBeginGuards(IcebergWriteContext ctx, String tableName) { // scan used, S_read), threaded onto the write handle and carried on the ctx. The commit-time // removeDeletes (option D) re-derives from baseSnapshotId, and BE unions the scan-time (S_read) // old deletes into the new DV — anchoring both at S_read keeps supply and remove on one snapshot - // (no resurrection under a concurrent commit in the read->begin-write window). A -1 readSnapshotId - // (no pin: a caller without the threaded handle) falls back to the begin-time current snapshot. + // (no resurrection under a concurrent commit in the read->begin-write window). long pinnedReadSnapshot = ctx.getReadSnapshotId(); - // Keep both ternary arms boxed (Long): getSnapshotIdIfPresent returns null for an empty table - // (no snapshot), and a primitive arm would force-unbox that null into an NPE. - this.baseSnapshotId = pinnedReadSnapshot >= 0 - ? Long.valueOf(pinnedReadSnapshot) : getSnapshotIdIfPresent(table); + if (ctx.isReadSnapshotResolved()) { + // An explicitly empty read stays null so validation covers a concurrent first append. + this.baseSnapshotId = pinnedReadSnapshot >= 0 ? Long.valueOf(pinnedReadSnapshot) : null; + } else { + this.baseSnapshotId = getSnapshotIdIfPresent(table); + } if (table instanceof HasTableOperations) { int formatVersion = ((HasTableOperations) table).operations().current().formatVersion(); if (formatVersion < 2) { @@ -623,9 +640,11 @@ private Expression buildPartitionFilter(Map staticPartitions, Pa } List predicates = new ArrayList<>(); + Set unmatchedFields = new HashSet<>(staticPartitions.keySet()); for (PartitionField field : spec.fields()) { String partitionColName = field.name(); if (staticPartitions.containsKey(partitionColName)) { + unmatchedFields.remove(partitionColName); String partitionValueStr = staticPartitions.get(partitionColName); Types.NestedField sourceField = schema.findField(field.sourceId()); if (sourceField == null) { @@ -642,8 +661,10 @@ private Expression buildPartitionFilter(Map staticPartitions, Pa } } - if (predicates.isEmpty()) { - return Expressions.alwaysTrue(); + // A stale nonempty static spec must never widen into an always-true full-table overwrite. + if (!unmatchedFields.isEmpty()) { + throw new DorisConnectorException("Static partition field does not match the current Iceberg spec: " + + unmatchedFields); } Expression result = predicates.get(0); for (int i = 1; i < predicates.size(); i++) { diff --git a/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergPartitionUtils.java b/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergPartitionUtils.java index ba077b4f63294b..b19acb5447973e 100644 --- a/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergPartitionUtils.java +++ b/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergPartitionUtils.java @@ -534,6 +534,15 @@ static ConnectorMvccPartitionView buildMvccPartitionView(Table table, long pinne return buildMvccPartitionView(table, pinnedSnapshotId, null, null); } + /** Builds the spec-derived style without consulting a snapshot committed after query begin. */ + static ConnectorMvccPartitionView buildResolvedEmptyMvccPartitionView(Table table) { + if (!isValidRelatedTable(table)) { + return ConnectorMvccPartitionView.unpartitioned(); + } + return new ConnectorMvccPartitionView(ConnectorMvccPartitionView.Style.RANGE, + ConnectorMvccPartitionView.Freshness.SNAPSHOT_ID, Collections.emptyList(), 0L); + } + /** * Cache-aware overload (PERF-02): {@code id} + {@code cache} route the PARTITIONS scan through the * per-catalog {@link IcebergPartitionCache} keyed by {@code (id, resolvedSnapshotId)}. {@code cache == null} diff --git a/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergScanPlanProvider.java b/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergScanPlanProvider.java index 42db23c93ac1c4..1e1fe650f0b52f 100644 --- a/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergScanPlanProvider.java +++ b/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergScanPlanProvider.java @@ -431,7 +431,8 @@ public List planScan(ConnectorSession session, ConnectorScan public long streamingSplitEstimate(ConnectorSession session, ConnectorTableHandle handle, Optional filter, boolean countPushdown) { IcebergTableHandle iceHandle = (IcebergTableHandle) handle; - if (iceHandle.isSystemTable() || !sessionBool(session, ENABLE_EXTERNAL_TABLE_BATCH_MODE, true)) { + if (iceHandle.isResolvedEmptySnapshot() || iceHandle.isSystemTable() + || !sessionBool(session, ENABLE_EXTERNAL_TABLE_BATCH_MODE, true)) { return -1; } Table table = resolveTable(session, iceHandle); @@ -469,13 +470,19 @@ public long streamingSplitEstimate(ConnectorSession session, ConnectorTableHandl * a FIXED size ({@code file_split_size} if set, else {@code max_split_size} — NOT the per-table * {@link #determineTargetFileSplitSize} heuristic, which would force materializing every task), so * {@code planFiles()} streams without holding the full task list — the OOM protection. Bypasses the manifest - * cache (its planning materializes; legacy's lazy batch path only ran with the manifest cache off). Only - * called after {@link #streamingSplitEstimate} returned ≥ 0, so the snapshot/non-sys/v<3 gates already hold. + * cache (its planning materializes; legacy's lazy batch path only ran with the manifest cache off). Usually + * called after {@link #streamingSplitEstimate} returned ≥ 0; because MVCC pinning happens afterward, this + * method must independently preserve an explicitly empty pinned snapshot. */ @Override public ConnectorSplitSource streamSplits(ConnectorSession session, ConnectorTableHandle handle, List columns, Optional filter, long limit) { IcebergTableHandle iceHandle = (IcebergTableHandle) handle; + if (iceHandle.isResolvedEmptySnapshot()) { + // The batch decision is made before the engine pins MVCC; once pinned empty, streaming must + // preserve that boundary instead of interpreting Iceberg's sentinel as the latest snapshot. + return emptySplitSource(); + } Table table = resolveTable(session, iceHandle); TableScan scan = buildScan(table, iceHandle, filter, session); int formatVersion = getFormatVersion(table); @@ -493,6 +500,24 @@ public ConnectorSplitSource streamSplits(ConnectorSession session, ConnectorTabl orderedPartitionKeys, zone, uriNormalizer, sliceSize, iceHandle.getRewriteFileScope()); } + private static ConnectorSplitSource emptySplitSource() { + return new ConnectorSplitSource() { + @Override + public boolean hasNext() { + return false; + } + + @Override + public ConnectorScanRange next() { + throw new NoSuchElementException(); + } + + @Override + public void close() { + } + }; + } + /** * The streaming source's whole-file enumeration, byte-offset-split at {@code sliceSize}. PERF-04 (C17): when * the manifest cache is enabled, read manifests THROUGH THE CACHE via the lazy {@link #cacheBackedFileScanTasks} @@ -607,6 +632,13 @@ private List planScanInternal( Optional filter, boolean countPushdown) { IcebergTableHandle iceHandle = (IcebergTableHandle) handle; + if (iceHandle.isResolvedEmptySnapshot() && !isSnapshotIndependentSystemTable(iceHandle)) { + // Iceberg has no snapshot id that can represent "before the first commit". Returning no ranges is + // the read-side MVCC fence; otherwise a refreshed Table would turn -1 into "latest" and expose a + // concurrent first append to MERGE after its anti-join had already decided the row was absent. Static + // metadata-history tables are exempt because their creation rows exist without a data snapshot. + return Collections.emptyList(); + } if (iceHandle.isSystemTable()) { // System tables take a metadata-table path, never the data-file path below (no count pushdown, no // data-file ranges) — mirrors legacy IcebergScanNode branching on isSystemTable. $position_deletes @@ -1137,6 +1169,18 @@ private static boolean supportsSnapshotSelection(IcebergTableHandle handle) { && type != MetadataTableType.ALL_ENTRIES; } + /** Metadata tables whose rows describe table metadata rather than files reachable from a data snapshot. */ + private static boolean isSnapshotIndependentSystemTable(IcebergTableHandle handle) { + if (!handle.isSystemTable()) { + return false; + } + MetadataTableType type = MetadataTableType.from(handle.getSysTableName()); + return type == MetadataTableType.HISTORY + || type == MetadataTableType.SNAPSHOTS + || type == MetadataTableType.REFS + || type == MetadataTableType.METADATA_LOG_ENTRIES; + } + /** * The schema AS OF the handle's pinned schema id (for time-travel reads under schema evolution); the latest * schema when there is no pinned id or it is absent from {@code table.schemas()} (defensive — legacy diff --git a/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergTableHandle.java b/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergTableHandle.java index 295402a0f66de9..db2b853541df92 100644 --- a/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergTableHandle.java +++ b/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergTableHandle.java @@ -34,11 +34,14 @@ * iceberg SDK applies time-travel through {@code TableScan.useSnapshot(id)} / {@code useRef(name)} rather * than a {@code Table.copy(properties)} option map: *
    - *
  • {@code snapshotId} ({@code -1} = none) — {@code FOR VERSION AS OF } / {@code FOR TIME AS OF}.
    • + *
  • {@code snapshotId} ({@code -1} = no concrete snapshot) — {@code FOR VERSION AS OF } / + * {@code FOR TIME AS OF}.
    • *
  • {@code ref} ({@code null} = none) — a tag/branch name; the scan pins by REF ({@code useRef}) so a * later commit to the tag/branch is honored (legacy parity).
    • *
  • {@code schemaId} ({@code -1} = latest) — the schema version AS OF the pin, so the field-id dictionary * and {@code getTableSchema(@snapshot)} read the historical schema.
    • + *
  • {@code snapshotResolved} distinguishes an unresolved latest read from a query-begin read that + * resolved to an empty table; both have {@code snapshotId=-1}, but only the latter is an MVCC boundary.
    • *
* The handle is immutable: {@link #withSnapshot} returns a NEW handle (the pin is part of the handle * identity, so {@link #equals}/{@link #hashCode}/{@link #toString} include it). @@ -61,6 +64,7 @@ public class IcebergTableHandle implements ConnectorTableHandle { private final long snapshotId; private final String ref; private final long schemaId; + private final boolean snapshotResolved; /** * Bare system-table name (no {@code "$"}), lower-cased by the caller @@ -97,16 +101,18 @@ public class IcebergTableHandle implements ConnectorTableHandle { private final boolean topnLazyMaterialize; public IcebergTableHandle(String dbName, String tableName) { - this(dbName, tableName, NO_PIN, null, NO_PIN, null, null, false); + this(dbName, tableName, NO_PIN, null, NO_PIN, false, null, null, false); } private IcebergTableHandle(String dbName, String tableName, long snapshotId, String ref, long schemaId, - String sysTableName, Set rewriteFileScope, boolean topnLazyMaterialize) { + boolean snapshotResolved, String sysTableName, Set rewriteFileScope, + boolean topnLazyMaterialize) { this.dbName = dbName; this.tableName = tableName; this.snapshotId = snapshotId; this.ref = ref; this.schemaId = schemaId; + this.snapshotResolved = snapshotResolved; this.sysTableName = sysTableName; this.rewriteFileScope = rewriteFileScope; this.topnLazyMaterialize = topnLazyMaterialize; @@ -121,7 +127,14 @@ private IcebergTableHandle(String dbName, String tableName, long snapshotId, Str */ public static IcebergTableHandle forSystemTable(String dbName, String tableName, String sysName, long snapshotId, String ref, long schemaId) { - return new IcebergTableHandle(dbName, tableName, snapshotId, ref, schemaId, sysName, null, false); + return forSystemTable(dbName, tableName, sysName, snapshotId, ref, schemaId, + snapshotId >= 0 || ref != null); + } + + static IcebergTableHandle forSystemTable(String dbName, String tableName, String sysName, + long snapshotId, String ref, long schemaId, boolean snapshotResolved) { + return new IcebergTableHandle(dbName, tableName, snapshotId, ref, schemaId, + snapshotResolved, sysName, null, false); } public String getDbName() { @@ -147,6 +160,11 @@ public long getSchemaId() { return schemaId; } + /** Whether query-begin snapshot resolution ran, including an explicitly empty table ({@code -1}). */ + public boolean isSnapshotResolved() { + return snapshotResolved; + } + /** Bare system-table name (no {@code "$"}), or {@code null} for a normal data-table handle. */ public String getSysTableName() { return sysTableName; @@ -162,6 +180,11 @@ public boolean hasSnapshotPin() { return snapshotId >= 0 || ref != null; } + /** Whether snapshot resolution observed a table before its first snapshot was committed. */ + public boolean isResolvedEmptySnapshot() { + return snapshotResolved && snapshotId < 0 && ref == null; + } + /** * The rewrite file scope (raw iceberg data-file paths the scan is restricted to), or {@code null} for a * normal full scan. See {@link #rewriteFileScope} and {@link #withRewriteFileScope}. @@ -183,8 +206,9 @@ public IcebergTableHandle withSnapshot(long snapshotId, String ref, long schemaI // sysTableName, rewriteFileScope and topnLazyMaterialize are preserved: threading a resolved // time-travel pin in must not degrade a sys handle (t$snapshots) into a normal data-table handle, // drop a rewrite scope, or drop the lazy-materialization signal. - return new IcebergTableHandle(dbName, tableName, snapshotId, ref, schemaId, sysTableName, - rewriteFileScope, topnLazyMaterialize); + // A resolved empty table still needs a marker even though useSnapshot(-1) is invalid. + return new IcebergTableHandle(dbName, tableName, snapshotId, ref, schemaId, true, + sysTableName, rewriteFileScope, topnLazyMaterialize); } /** @@ -196,8 +220,8 @@ public IcebergTableHandle withSnapshot(long snapshotId, String ref, long schemaI * The other carriers (snapshot/ref/schema/sys) are preserved. */ public IcebergTableHandle withRewriteFileScope(Set rawDataFilePaths) { - return new IcebergTableHandle(dbName, tableName, snapshotId, ref, schemaId, sysTableName, - ImmutableSet.copyOf(rawDataFilePaths), topnLazyMaterialize); + return new IcebergTableHandle(dbName, tableName, snapshotId, ref, schemaId, snapshotResolved, + sysTableName, ImmutableSet.copyOf(rawDataFilePaths), topnLazyMaterialize); } /** @@ -205,8 +229,8 @@ public IcebergTableHandle withRewriteFileScope(Set rawDataFilePaths) { * {@link #topnLazyMaterialize}). The other carriers (snapshot/ref/schema/sys/rewriteScope) are preserved. */ public IcebergTableHandle withTopnLazyMaterialize(boolean topnLazyMaterialize) { - return new IcebergTableHandle(dbName, tableName, snapshotId, ref, schemaId, sysTableName, - rewriteFileScope, topnLazyMaterialize); + return new IcebergTableHandle(dbName, tableName, snapshotId, ref, schemaId, snapshotResolved, + sysTableName, rewriteFileScope, topnLazyMaterialize); } @Override @@ -220,6 +244,7 @@ public boolean equals(Object o) { IcebergTableHandle that = (IcebergTableHandle) o; return snapshotId == that.snapshotId && schemaId == that.schemaId + && snapshotResolved == that.snapshotResolved && topnLazyMaterialize == that.topnLazyMaterialize && Objects.equals(dbName, that.dbName) && Objects.equals(tableName, that.tableName) @@ -230,8 +255,8 @@ public boolean equals(Object o) { @Override public int hashCode() { - return Objects.hash(dbName, tableName, snapshotId, ref, schemaId, sysTableName, rewriteFileScope, - topnLazyMaterialize); + return Objects.hash(dbName, tableName, snapshotId, ref, schemaId, snapshotResolved, + sysTableName, rewriteFileScope, topnLazyMaterialize); } @Override @@ -243,6 +268,8 @@ public String toString() { if (hasSnapshotPin()) { sb.append(", snapshotId=").append(snapshotId).append(", ref=").append(ref) .append(", schemaId=").append(schemaId); + } else if (snapshotResolved) { + sb.append(", snapshot=empty"); } if (rewriteFileScope != null) { sb.append(", rewriteFileScope=").append(rewriteFileScope.size()).append(" files"); diff --git a/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergTypeMapping.java b/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergTypeMapping.java index 005ac7f7289830..14779059cdff65 100644 --- a/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergTypeMapping.java +++ b/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergTypeMapping.java @@ -63,7 +63,7 @@ public static ConnectorType fromIcebergType(Type icebergType, Types.ListType list = (Types.ListType) icebergType; ConnectorType elemType = fromIcebergType( list.elementType(), enableMappingVarbinary, enableMappingTimestampTz); - return ConnectorType.arrayOf(elemType) + return ConnectorType.arrayOf(elemType, list.isElementOptional()) .withChildrenFieldIds(Collections.singletonList(list.elementId())); case MAP: // Carry key + value field-ids (legacy recurses into both via MapType.fields()). @@ -72,7 +72,7 @@ public static ConnectorType fromIcebergType(Type icebergType, map.keyType(), enableMappingVarbinary, enableMappingTimestampTz); ConnectorType valType = fromIcebergType( map.valueType(), enableMappingVarbinary, enableMappingTimestampTz); - return ConnectorType.mapOf(keyType, valType) + return ConnectorType.mapOf(keyType, valType, map.isValueOptional()) .withChildrenFieldIds(Arrays.asList(map.keyId(), map.valueId())); case STRUCT: // Carry each field's field-id, parallel to the field types (legacy recurses field-by-field). diff --git a/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergWriteContext.java b/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergWriteContext.java index 18d1445817713e..5b6132a3efc8c1 100644 --- a/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergWriteContext.java +++ b/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergWriteContext.java @@ -42,20 +42,38 @@ final class IcebergWriteContext { private final Map staticPartitionValues; private final Optional branchName; private final long readSnapshotId; + private final boolean readSnapshotResolved; + private final String boundWriteMetadataIdentity; IcebergWriteContext(WriteOperation writeOperation, boolean overwrite, Map staticPartitionValues, Optional branchName) { - this(writeOperation, overwrite, staticPartitionValues, branchName, -1L); + this(writeOperation, overwrite, staticPartitionValues, branchName, -1L, false); } IcebergWriteContext(WriteOperation writeOperation, boolean overwrite, Map staticPartitionValues, Optional branchName, long readSnapshotId) { + this(writeOperation, overwrite, staticPartitionValues, branchName, + readSnapshotId, readSnapshotId >= 0); + } + + IcebergWriteContext(WriteOperation writeOperation, boolean overwrite, + Map staticPartitionValues, Optional branchName, + long readSnapshotId, boolean readSnapshotResolved) { + this(writeOperation, overwrite, staticPartitionValues, branchName, + readSnapshotId, readSnapshotResolved, null); + } + + IcebergWriteContext(WriteOperation writeOperation, boolean overwrite, + Map staticPartitionValues, Optional branchName, + long readSnapshotId, boolean readSnapshotResolved, String boundWriteMetadataIdentity) { this.writeOperation = writeOperation; this.overwrite = overwrite; this.staticPartitionValues = staticPartitionValues == null ? Collections.emptyMap() : new HashMap<>(staticPartitionValues); this.branchName = branchName == null ? Optional.empty() : branchName; this.readSnapshotId = readSnapshotId; + this.readSnapshotResolved = readSnapshotResolved; + this.boundWriteMetadataIdentity = boundWriteMetadataIdentity; } WriteOperation getWriteOperation() { @@ -81,7 +99,8 @@ Optional getBranchName() { /** * The statement's READ snapshot id (the MVCC pin the scan used, S_read), threaded from the write - * handle in {@code planWrite}; {@code -1} = no pin (the legacy fresh-current behavior). The + * handle in {@code planWrite}; {@code -1} means either no pin or an explicitly empty read, as + * distinguished by {@link #isReadSnapshotResolved()}. The * RowDelta path anchors {@code baseSnapshotId} at this snapshot so the commit-time removeDeletes * (option D) and the scan-time deletes BE unions into the new DV share one snapshot — see * {@link IcebergConnectorTransaction} [SHOULD-2] / Fix B. @@ -89,4 +108,13 @@ Optional getBranchName() { long getReadSnapshotId() { return readSnapshotId; } + + /** Whether the read snapshot was resolved, including a table with no snapshot yet. */ + boolean isReadSnapshotResolved() { + return readSnapshotResolved; + } + + String getBoundWriteMetadataIdentity() { + return boundWriteMetadataIdentity; + } } diff --git a/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergWritePlanProvider.java b/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergWritePlanProvider.java index d9410c3dc93621..a855fde3273e90 100644 --- a/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergWritePlanProvider.java +++ b/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergWritePlanProvider.java @@ -73,10 +73,13 @@ import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Locale; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.function.Function; +import java.util.stream.Collectors; /** * Write plan provider for iceberg INSERT / INSERT OVERWRITE. @@ -173,6 +176,8 @@ public ConnectorSinkPlan planWrite(ConnectorSession session, ConnectorWriteHandl IcebergWriteContext writeContext = buildWriteContext(handle); transaction.beginWrite(session, tableHandle.getDbName(), tableHandle.getTableName(), writeContext); Table table = transaction.getTable(); + validateBoundWriteMetadata(table, handle); + validateBoundWriteColumns(table, handle, writeContext.getWriteOperation()); // commit-bridge supply (S4 part 2): read the non-equality delete supply the scan seam accumulated into the // per-statement scope. DELETE/MERGE attach it to the sink so the BE OR-merges old deletes into the new @@ -230,6 +235,125 @@ public ConnectorSinkPlan planWrite(ConnectorSession session, ConnectorWriteHandl } } + private void validateBoundWriteColumns(Table table, ConnectorWriteHandle handle, + WriteOperation writeOperation) { + List boundTargetColumns = handle.getBoundTargetColumns(); + if (!boundTargetColumns.isEmpty() + && (writeOperation == WriteOperation.REWRITE + || writeOperation == WriteOperation.UPDATE + || writeOperation == WriteOperation.MERGE)) { + long boundLineageColumns = boundTargetColumns.stream() + .filter(ConnectorColumn::isReservedPassthrough) + .count(); + long currentLineageColumns = IcebergWriterHelper.getFormatVersion(table) >= 3 ? 2 : 0; + // Format version changes the physical sink arity without changing table.schema(); the reserved + // columns are the bind-time witness that the output and v3 schema-json belong to one generation. + if (boundLineageColumns != currentLineageColumns) { + throw new DorisConnectorException( + "Iceberg write metadata changed after the write was bound; retry the statement"); + } + } + // V3 row-lineage columns are engine-generated metadata, not fields in table.schema(). Excluding + // their neutral marker keeps the comparison on the complete user schema for INSERT and MERGE. + List boundColumns = boundTargetColumns.stream() + .filter(column -> !column.isReservedPassthrough()) + .collect(Collectors.toList()); + if (writeOperation == WriteOperation.DELETE || boundColumns.isEmpty()) { + return; + } + List currentColumns = table.schema().columns(); + boolean rowLevelWrite = writeOperation == WriteOperation.UPDATE || writeOperation == WriteOperation.MERGE; + boolean hasSyntheticRowId = rowLevelWrite && boundColumns.size() == currentColumns.size() + 1 + && DORIS_ICEBERG_ROWID_COL.equals(boundColumns.get(boundColumns.size() - 1).getName()); + if (boundColumns.size() != currentColumns.size() && !hasSyntheticRowId) { + throw new DorisConnectorException("Iceberg table schema changed after the write was bound; retry the " + + "statement with the latest schema"); + } + boolean enableVarbinary = Boolean.parseBoolean(properties.getOrDefault( + IcebergConnectorProperties.ENABLE_MAPPING_VARBINARY, "false")); + boolean enableTimestampTz = Boolean.parseBoolean(properties.getOrDefault( + IcebergConnectorProperties.ENABLE_MAPPING_TIMESTAMP_TZ, "false")); + for (int i = 0; i < currentColumns.size(); ++i) { + NestedField current = currentColumns.get(i); + ConnectorColumn bound = boundColumns.get(i); + ConnectorType currentType = IcebergTypeMapping.fromIcebergType( + current.type(), enableVarbinary, enableTimestampTz); + // Do not compare top-level nullability: Doris widens Iceberg required columns in its read schema + // so evolution default-fill may yield NULL. Nested requiredness remains authoritative in + // sameBoundType, while current schema JSON enforces writes at the root. + if (!current.name().equalsIgnoreCase(bound.getName()) + || !sameBoundType(currentType, bound.getType()) + || (bound.getUniqueId() >= 0 && current.fieldId() != bound.getUniqueId()) + // Omitted columns and DEFAULT expressions were already materialized from this value at bind. + || !Objects.equals(bound.getDefaultValue(), IcebergSchemaUtils.writeDefaultToDorisString( + current.type(), current.writeDefault(), enableTimestampTz))) { + // BE maps write expressions to schema-json by ordinal, so accepting a reordered live + // schema here could silently place values under the wrong Iceberg field names. + throw new DorisConnectorException("Iceberg table schema changed after the write was bound; retry " + + "the statement with the latest schema"); + } + } + } + + private void validateBoundWriteMetadata(Table table, ConnectorWriteHandle handle) { + String boundIdentity = handle.getBoundWriteMetadataIdentity(); + if (boundIdentity == null) { + return; + } + // The FE sort/distribution and Iceberg file metadata must come from one generation; otherwise + // beginWrite's refresh can silently stamp files with a sort order or partition spec they did not use. + if (!boundIdentity.equals(writeMetadataIdentity(table))) { + throw new DorisConnectorException( + "Iceberg write metadata changed after the write was bound; retry the statement"); + } + } + + private static boolean sameBoundType(ConnectorType current, ConnectorType bound) { + String currentName = canonicalTypeName(current.getTypeName()); + String boundName = canonicalTypeName(bound.getTypeName()); + if (!currentName.equals(boundName) + || current.getChildren().size() != bound.getChildren().size() + || current.getFieldNames().size() != bound.getFieldNames().size()) { + return false; + } + if (hasMeaningfulTypeParameters(currentName) + && (current.getPrecision() != bound.getPrecision() || current.getScale() != bound.getScale())) { + return false; + } + for (int i = 0; i < current.getChildren().size(); i++) { + if (!current.getFieldNames().isEmpty() + && !current.getFieldNames().get(i).equalsIgnoreCase(bound.getFieldNames().get(i))) { + return false; + } + int boundFieldId = bound.getChildFieldId(i); + if ((boundFieldId >= 0 && current.getChildFieldId(i) != boundFieldId) + || current.isChildNullable(i) != bound.isChildNullable(i) + || !sameBoundType(current.getChildren().get(i), bound.getChildren().get(i))) { + // Nested field identity and requiredness are part of the write contract even though the SPI + // type's general equals() deliberately excludes them for non-write consumers. + return false; + } + } + return true; + } + + private static String canonicalTypeName(String typeName) { + String normalized = typeName.toUpperCase(Locale.ROOT); + // Doris chooses a physical DECIMAL width after conversion, while Iceberg exposes one logical decimal. + // Width aliases with the same precision/scale are one schema, not concurrent evolution. + return normalized.startsWith("DECIMAL") && !"DECIMALV2".equals(normalized) + ? "DECIMALV3" : normalized; + } + + private static boolean hasMeaningfulTypeParameters(String typeName) { + return typeName.startsWith("DECIMAL") + || "CHAR".equals(typeName) + || "VARCHAR".equals(typeName) + || "VARBINARY".equals(typeName) + || "DATETIMEV2".equals(typeName) + || "TIMESTAMPTZ".equals(typeName); + } + @Override public void appendExplainInfo(StringBuilder output, String prefix, ConnectorSession session, ConnectorWriteHandle handle) { @@ -257,6 +381,12 @@ public void appendExplainInfo(StringBuilder output, String prefix, @Override public List getWriteSortColumns(ConnectorSession session, ConnectorTableHandle tableHandle) { + return getWriteSortColumns(session, tableHandle, Collections.emptyList()); + } + + @Override + public List getWriteSortColumns(ConnectorSession session, + ConnectorTableHandle tableHandle, List boundTargetColumns) { Table table = resolveTable(session, (IcebergTableHandle) tableHandle); SortOrder sortOrder = table.sortOrder(); if (!sortOrder.isSorted()) { @@ -265,24 +395,89 @@ public List getWriteSortColumns(ConnectorSession sessi // unconditional setSortInfo inside the isSorted() branch even when no identity column resolves. return null; } - List columns = table.schema().columns(); + Map positionsByFieldId = new HashMap<>(); + if (boundTargetColumns.isEmpty()) { + List currentColumns = table.schema().columns(); + for (int i = 0; i < currentColumns.size(); i++) { + positionsByFieldId.put(currentColumns.get(i).fieldId(), i); + } + } else { + for (int i = 0; i < boundTargetColumns.size(); i++) { + positionsByFieldId.put(boundTargetColumns.get(i).getUniqueId(), i); + } + } List result = new ArrayList<>(); for (SortField sortField : sortOrder.fields()) { if (!sortField.transform().isIdentity()) { continue; } - for (int i = 0; i < columns.size(); i++) { - if (columns.get(i).fieldId() == sortField.sourceId()) { - result.add(new ConnectorWriteSortColumn(i, - sortField.direction() == SortDirection.ASC, - sortField.nullOrder() == NullOrder.NULLS_FIRST)); - break; - } + Integer position = positionsByFieldId.get(sortField.sourceId()); + if (position != null) { + // Resolve against the bound field id, never a newly refreshed live ordinal; otherwise + // schema reorder can sort one output expression using another column's ordering contract. + result.add(new ConnectorWriteSortColumn(position, + sortField.direction() == SortDirection.ASC, + sortField.nullOrder() == NullOrder.NULLS_FIRST)); } } return result; } + @Override + public String getWriteMetadataIdentity(ConnectorSession session, ConnectorTableHandle tableHandle) { + return writeMetadataIdentity(resolveTable(session, (IcebergTableHandle) tableHandle)); + } + + static String writeMetadataIdentity(Table table) { + StringBuilder identity = new StringBuilder(); + // UUID and schema id make this a table-generation fence, not just a physical-layout signature. They + // reject same-name recreation and every schema commit before a stale bound output can reach the sink. + appendMetadataToken(identity, "uuid"); + appendMetadataToken(identity, tableUuid(table)); + appendMetadataToken(identity, "schema"); + appendMetadataToken(identity, table.schema().schemaId()); + appendMetadataToken(identity, "format"); + appendMetadataToken(identity, IcebergWriterHelper.getFormatVersion(table)); + SortOrder sortOrder = table.sortOrder(); + appendMetadataToken(identity, "sort"); + // Preserve a deterministic fence for partial Table implementations instead of failing schema loads. + appendMetadataToken(identity, sortOrder == null ? null : sortOrder.orderId()); + if (sortOrder != null) { + for (SortField field : sortOrder.fields()) { + appendMetadataToken(identity, field.sourceId()); + appendMetadataToken(identity, field.transform()); + appendMetadataToken(identity, field.direction()); + appendMetadataToken(identity, field.nullOrder()); + } + } + PartitionSpec spec = table.spec(); + appendMetadataToken(identity, "spec"); + appendMetadataToken(identity, spec == null ? null : spec.specId()); + if (spec != null) { + for (PartitionField field : spec.fields()) { + appendMetadataToken(identity, field.sourceId()); + appendMetadataToken(identity, field.fieldId()); + appendMetadataToken(identity, field.name()); + appendMetadataToken(identity, field.transform()); + } + } + return identity.toString(); + } + + private static Object tableUuid(Table table) { + try { + return table.uuid(); + } catch (UnsupportedOperationException e) { + // Non-BaseTable test doubles may not expose UUID; their remaining metadata still forms a fence. + return null; + } + } + + private static void appendMetadataToken(StringBuilder identity, Object value) { + String token = String.valueOf(value); + identity.append(token.length()).append(':').append(token); + } + @Override public ConnectorWritePartitionSpec getWritePartitioning(ConnectorSession session, ConnectorTableHandle tableHandle) { @@ -369,13 +564,16 @@ private IcebergWriteContext buildWriteContext(ConnectorWriteHandle handle) { // Carry it on the op-context so beginWrite anchors the RowDelta baseSnapshotId at S_read, keeping // the commit-time removeDeletes (option D) and BE's scan-time DV union on one snapshot. -1 (no pin) // preserves the legacy begin-time current snapshot. - long readSnapshotId = handle.getTableHandle() instanceof IcebergTableHandle - ? ((IcebergTableHandle) handle.getTableHandle()).getSnapshotId() : -1L; + IcebergTableHandle readHandle = handle.getTableHandle() instanceof IcebergTableHandle + ? (IcebergTableHandle) handle.getTableHandle() : null; + long readSnapshotId = readHandle != null ? readHandle.getSnapshotId() : -1L; + boolean readSnapshotResolved = readHandle != null && readHandle.isSnapshotResolved(); // Branch-targeted INSERT (INSERT INTO tbl@branch): the branch is threaded from the generic insert // command context onto the write handle; beginWrite validates it against the table refs and points // the commit at the branch. Empty for a default-ref write. return new IcebergWriteContext(op, handle.isOverwrite(), handle.getStaticPartitionSpec(), - handle.getBranchName(), readSnapshotId); + handle.getBranchName(), readSnapshotId, readSnapshotResolved, + handle.getBoundWriteMetadataIdentity()); } private TIcebergTableSink buildSink(Table table, IcebergTableHandle tableHandle, diff --git a/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergConnectorMetadataMvccTest.java b/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergConnectorMetadataMvccTest.java index 35748356013e52..4175d5099558a0 100644 --- a/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergConnectorMetadataMvccTest.java +++ b/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergConnectorMetadataMvccTest.java @@ -335,16 +335,17 @@ public void applySnapshotThreadsRef() { } @Test - public void applySnapshotLatestPinLeavesHandleUnchanged() { + public void applySnapshotRecordsExplicitlyEmptyPinWithoutScanPin() { Fixture f = fixture(); IcebergConnectorMetadata md = metadataFor(f.table, new RecordingIcebergCatalogOps()); ConnectorTableHandle bare = handle(); - // null snapshot and an empty-table (-1, no ref) pin must both read latest (handle unchanged) — a - // useSnapshot(-1) would be a non-existent snapshot. + // A null snapshot leaves the handle untouched. An empty-table pin must remain unpinned for scanning + // because useSnapshot(-1) is invalid, while retaining that resolution for write conflict validation. Assertions.assertSame(bare, md.applySnapshot(null, bare, null)); IcebergTableHandle afterMinusOne = (IcebergTableHandle) md.applySnapshot(null, bare, ConnectorMvccSnapshot.builder().snapshotId(-1L).build()); Assertions.assertFalse(afterMinusOne.hasSnapshotPin()); + Assertions.assertTrue(afterMinusOne.isSnapshotResolved()); } // --------------------------------------------------------------------- diff --git a/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergConnectorMetadataPartitionViewCacheTest.java b/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergConnectorMetadataPartitionViewCacheTest.java index e5cb30b0ed97d8..f9bce35b09f0b5 100644 --- a/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergConnectorMetadataPartitionViewCacheTest.java +++ b/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergConnectorMetadataPartitionViewCacheTest.java @@ -197,6 +197,29 @@ public void getMvccPartitionViewNullCacheEnumeratesEveryCall() { Assertions.assertEquals(2, loadCount(ops), "a null (disabled) cache must re-enumerate every call"); } + @Test + public void resolvedEmptyPartitionViewIgnoresConcurrentFirstAppend() { + InMemoryCatalog catalog = new InMemoryCatalog(); + catalog.initialize("test", Collections.emptyMap()); + catalog.createNamespace(Namespace.of("db1")); + PartitionSpec spec = PartitionSpec.builderFor(PARTITIONED_SCHEMA).day("ts").build(); + Table table = catalog.createTable(TableIdentifier.of("db1", "t1"), PARTITIONED_SCHEMA, spec); + IcebergTableHandle resolvedEmpty = handle().withSnapshot(-1L, null, table.schema().schemaId()); + + table.newAppend().appendFile( + dayFile(spec, "s3://b/db1/t1/concurrent.parquet", "ts_day=1970-04-11")).commit(); + RecordingIcebergCatalogOps ops = new RecordingIcebergCatalogOps(); + ops.table = catalog.loadTable(TableIdentifier.of("db1", "t1")); + IcebergConnectorMetadata md = metadataWithMvccCache(ops, null); + + ConnectorMvccPartitionView view = md.getMvccPartitionView(null, resolvedEmpty).orElseThrow(); + + // Data rows and freshness must describe the same query-begin generation after the first append. + Assertions.assertEquals(ConnectorMvccPartitionView.Style.RANGE, view.getStyle()); + Assertions.assertTrue(view.getPartitions().isEmpty()); + Assertions.assertEquals(0L, view.getNewestUpdateMonotonicMarker()); + } + // --------------------------------------------------------------------- // listPartitions // --------------------------------------------------------------------- diff --git a/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergConnectorMetadataTest.java b/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergConnectorMetadataTest.java index f134618c386a28..e9923102ac15f2 100644 --- a/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergConnectorMetadataTest.java +++ b/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergConnectorMetadataTest.java @@ -601,6 +601,9 @@ public void getTableSchemaParsesColumnsFromLoadedTable() { Assertions.assertEquals("INT", cols.get(0).getType().getTypeName()); Assertions.assertEquals("name", cols.get(1).getName()); Assertions.assertEquals("STRING", cols.get(1).getType().getTypeName()); + Assertions.assertEquals(IcebergWritePlanProvider.writeMetadataIdentity(ops.table), + schema.getWriteMetadataIdentity(), + "the bind-time schema and write fence must be derived from the exact same table load"); // WHY: legacy IcebergUtils.parseSchema builds EVERY column with isAllowNull=true regardless of // the Iceberg field's required/optional flag (rows can still read NULL under schema-evolution diff --git a/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergConnectorTransactionTest.java b/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergConnectorTransactionTest.java index 0ed5a711ae065a..c9b10e55212b08 100644 --- a/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergConnectorTransactionTest.java +++ b/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergConnectorTransactionTest.java @@ -21,6 +21,7 @@ import org.apache.doris.connector.api.ConnectorType; import org.apache.doris.connector.api.DorisConnectorException; import org.apache.doris.connector.api.handle.WriteOperation; +import org.apache.doris.connector.api.mvcc.ConnectorMvccSnapshot; import org.apache.doris.connector.api.pushdown.ConnectorAnd; import org.apache.doris.connector.api.pushdown.ConnectorBetween; import org.apache.doris.connector.api.pushdown.ConnectorColumnRef; @@ -28,6 +29,7 @@ import org.apache.doris.connector.api.pushdown.ConnectorIsNull; import org.apache.doris.connector.api.pushdown.ConnectorLiteral; import org.apache.doris.connector.api.pushdown.ConnectorPredicate; +import org.apache.doris.connector.api.scan.ConnectorScanRequest; import org.apache.doris.thrift.TFileContent; import org.apache.doris.thrift.TIcebergColumnStats; import org.apache.doris.thrift.TIcebergCommitData; @@ -57,6 +59,8 @@ import org.junit.jupiter.api.Test; import java.io.IOException; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Proxy; import java.nio.ByteBuffer; import java.util.ArrayList; import java.util.Arrays; @@ -66,6 +70,9 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; /** * Pins {@link IcebergConnectorTransaction}: the T03 skeleton (single SDK transaction held through the @@ -616,6 +623,26 @@ public void overwriteStaticPartitionUsesRowFilter() { Assertions.assertEquals("1", snap.summary().get("added-data-files")); } + @Test + public void overwriteStaticPartitionRejectsUnmatchedPartitionField() { + InMemoryCatalog catalog = freshCatalog(); + TableIdentifier id = TableIdentifier.of("db1", "t1"); + PartitionSpec spec = PartitionSpec.builderFor(PART_SCHEMA).identity("region").build(); + Table table = catalog.createTable(id, PART_SCHEMA, spec, + props("write.format.default", "parquet")); + table.updateSpec().renameField("region", "renamed_region").commit(); + IcebergConnectorTransaction txn = txnFor( + opsReturning(catalog.loadTable(id)), new RecordingConnectorContext()); + + txn.beginWrite(SESSION, "db1", "t1", + overwriteStaticCtx(Collections.singletonMap("region", "us"))); + + DorisConnectorException ex = Assertions.assertThrows( + DorisConnectorException.class, txn::commit); + Assertions.assertTrue(ex.getMessage().contains("does not match"), + "a nonempty stale spec must never degrade to an always-true overwrite filter"); + } + @Test public void deleteWritesRowDeltaDeleteFiles() { InMemoryCatalog catalog = freshCatalog(); @@ -693,6 +720,38 @@ public void rollbackAndCloseAreNoOps() { }); } + @Test + public void beginWriteRejectsReplacementExposedByTransactionRefresh() { + InMemoryCatalog catalog = freshCatalog(); + TableIdentifier id = TableIdentifier.of("db1", "t1"); + Table original = catalog.createTable(id, SCHEMA, PartitionSpec.unpartitioned(), + props("format-version", "2")); + String originalIdentity = IcebergWritePlanProvider.writeMetadataIdentity(original); + catalog.dropTable(id, false); + Table replacement = catalog.createTable(id, SCHEMA, PartitionSpec.unpartitioned(), + props("format-version", "2")); + AtomicReference delegate = new AtomicReference<>(original); + Table refreshingTable = (Table) Proxy.newProxyInstance(Table.class.getClassLoader(), + new Class[] {Table.class}, (proxy, method, args) -> { + if ("newTransaction".equals(method.getName())) { + delegate.set(replacement); + } + try { + return method.invoke(delegate.get(), args); + } catch (InvocationTargetException e) { + throw e.getCause(); + } + }); + IcebergConnectorTransaction txn = txnFor(opsReturning(refreshingTable), new RecordingConnectorContext()); + IcebergWriteContext ctx = new IcebergWriteContext(WriteOperation.INSERT, false, + Collections.emptyMap(), Optional.empty(), -1L, false, originalIdentity); + + DorisConnectorException ex = Assertions.assertThrows(DorisConnectorException.class, + () -> txn.beginWrite(SESSION, "db1", "t1", ctx)); + Assertions.assertTrue(ex.getMessage().contains("write metadata changed"), + "a replacement exposed by newTransaction refresh must not become the write baseline"); + } + // ─────────────────── commit-time conflict-detection validation suite (T05) ─────────────────── @Test @@ -719,6 +778,65 @@ public void deleteDetectsConcurrentDataFileConflict() { "a concurrent data-file append since the base snapshot must be detected as a conflict"); } + @Test + public void mergeFromResolvedEmptySnapshotRejectsConcurrentFirstAppend() throws Exception { + InMemoryCatalog catalog = freshCatalog(); + TableIdentifier id = TableIdentifier.of("db1", "t1"); + Table empty = catalog.createTable(id, SCHEMA, PartitionSpec.unpartitioned(), + props("format-version", "2", "write.format.default", "parquet")); + RecordingIcebergCatalogOps ops = opsReturning(empty); + RecordingConnectorContext context = new RecordingConnectorContext(); + IcebergConnectorMetadata metadata = new IcebergConnectorMetadata(ops, Collections.emptyMap(), context); + + CountDownLatch mergeReadResolved = new CountDownLatch(1); + CountDownLatch concurrentInsertCommitted = new CountDownLatch(1); + AtomicReference insertFailure = new AtomicReference<>(); + Thread concurrentInsert = new Thread(() -> { + try { + if (!mergeReadResolved.await(10, TimeUnit.SECONDS)) { + throw new AssertionError("MERGE read barrier was not reached"); + } + catalog.loadTable(id).newAppend() + .appendFile(dataFile(PartitionSpec.unpartitioned(), + "s3://b/db1/t1/concurrent.parquet", 1L)) + .commit(); + } catch (Throwable t) { + insertFailure.set(t); + } finally { + concurrentInsertCommitted.countDown(); + } + }, "iceberg-concurrent-first-append"); + concurrentInsert.start(); + + ConnectorMvccSnapshot snapshot = metadata.beginQuerySnapshot(null, + new IcebergTableHandle("db1", "t1")).orElseThrow(AssertionError::new); + IcebergTableHandle emptyRead = (IcebergTableHandle) metadata.applySnapshot( + null, new IcebergTableHandle("db1", "t1"), snapshot); + mergeReadResolved.countDown(); + Assertions.assertTrue(concurrentInsertCommitted.await(10, TimeUnit.SECONDS)); + concurrentInsert.join(); + Assertions.assertNull(insertFailure.get(), "the concurrent INSERT must commit at the barrier"); + + ops.table = catalog.loadTable(id); + IcebergScanPlanProvider scanProvider = new IcebergScanPlanProvider(Collections.emptyMap(), ops); + Assertions.assertTrue(scanProvider.planScan(null, + ConnectorScanRequest.builder(emptyRead, Collections.emptyList()).build()).isEmpty(), + "MERGE must keep reading the empty snapshot after the concurrent INSERT"); + + IcebergConnectorTransaction merge = txnFor(ops, context); + merge.beginWrite(SESSION, "db1", "t1", new IcebergWriteContext( + WriteOperation.MERGE, false, Collections.emptyMap(), Optional.empty(), + emptyRead.getSnapshotId(), emptyRead.isSnapshotResolved())); + merge.addCommitData(commitBytes(dataFileItem("s3://b/db1/t1/merge.parquet", 1L, 1024L))); + + Assertions.assertThrows(DorisConnectorException.class, merge::commit, + "RowDelta must validate from table creation and reject the first concurrent append"); + List committedFiles = currentDataFiles(catalog.loadTable(id)); + Assertions.assertEquals(1, committedFiles.size(), "the failed MERGE must not add a duplicate row file"); + Assertions.assertEquals("s3://b/db1/t1/concurrent.parquet", + committedFiles.get(0).path().toString()); + } + @Test public void deletePassesValidationSuiteWhenNoConcurrentChange() { InMemoryCatalog catalog = freshCatalog(); diff --git a/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergScanPlanProviderTest.java b/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergScanPlanProviderTest.java index 148f47c47aa652..87fe0f25475bd1 100644 --- a/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergScanPlanProviderTest.java +++ b/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergScanPlanProviderTest.java @@ -1192,6 +1192,80 @@ public void planScanPinnedToOlderSnapshotReadsOnlyThatSnapshotsFiles() { Assertions.assertTrue(pinned.get(0).getPath().get().endsWith("f1.parquet")); } + @Test + public void planScanResolvedEmptySnapshotIgnoresConcurrentFirstAppend() { + // MERGE may resolve its read snapshot while the target has no snapshots, then race with the first append. + // The -1 marker is a real MVCC boundary: treating it as "latest" lets MERGE miss the new row and insert a + // duplicate, so both the ordinary scan and COUNT pushdown must remain empty after the concurrent commit. + Table table = createTable("t1", SCHEMA, PartitionSpec.unpartitioned()); + IcebergTableHandle emptyRead = new IcebergTableHandle("db1", "t1") + .withSnapshot(-1L, null, table.schema().schemaId()); + + table.newAppend().appendFile( + dataFile(table.spec(), "s3://b/db/t1/concurrent.parquet", 1024, null, null)).commit(); + IcebergScanPlanProvider provider = + new IcebergScanPlanProvider(Collections.emptyMap(), opsReturning(table)); + + List rows = provider.planScan(null, + ConnectorScanRequest.builder(emptyRead, Collections.emptyList()).build()); + List count = provider.planScan(null, + ConnectorScanRequest.builder(emptyRead, Collections.emptyList()) + .requiredPartitions(Collections.emptyList()).countPushdown(true).build()); + + Assertions.assertTrue(rows.isEmpty(), "an explicitly empty read must not see the first concurrent append"); + Assertions.assertTrue(count.isEmpty(), "COUNT over an explicitly empty read must remain zero"); + } + + @Test + public void streamSplitsResolvedEmptySnapshotIgnoresConcurrentFirstAppend() throws IOException { + Table table = createTable("t1", SCHEMA, PartitionSpec.unpartitioned()); + IcebergTableHandle emptyRead = new IcebergTableHandle("db1", "t1") + .withSnapshot(-1L, null, table.schema().schemaId()); + IcebergScanPlanProvider provider = + new IcebergScanPlanProvider(Collections.emptyMap(), opsReturning(table)); + + table.newAppend().appendFile( + dataFile(table.spec(), "s3://b/db/t1/concurrent.parquet", 1024, null, null)).commit(); + + Assertions.assertEquals(1, provider.streamingSplitEstimate(batchSession(1, true), + new IcebergTableHandle("db1", "t1"), Optional.empty(), false), + "the pre-pin batch decision must observe the concurrent first append"); + List ranges = drain(provider.streamSplits(emptySession(), + emptyRead, Collections.emptyList(), Optional.empty(), -1L)); + Assertions.assertTrue(ranges.isEmpty(), + "the post-pin streaming path must preserve the explicitly empty read boundary"); + } + + @Test + public void resolvedEmptyMetadataLogEntriesStillReturnsCreationEntry() throws Exception { + Table table = createTable("t1", SCHEMA, PartitionSpec.unpartitioned()); + IcebergScanPlanProvider provider = + new IcebergScanPlanProvider(Collections.emptyMap(), opsReturning(table)); + IcebergTableHandle handle = IcebergTableHandle.forSystemTable( + "db1", "t1", "metadata_log_entries", -1L, null, table.schema().schemaId(), true); + + List ranges = provider.planScan(null, + ConnectorScanRequest.builder(handle, Collections.emptyList()).build()); + + Assertions.assertEquals(1L, countSerializedSplitRows(ranges), + "metadata history exists before the first data snapshot and must not be hidden by the data fence"); + } + + @Test + public void resolvedEmptyAllFilesDoesNotExposeConcurrentFirstAppend() { + Table table = createTable("t1", SCHEMA, PartitionSpec.unpartitioned()); + IcebergTableHandle emptyAllFiles = IcebergTableHandle.forSystemTable( + "db1", "t1", "all_files", -1L, null, table.schema().schemaId(), true); + table.newAppend().appendFile( + dataFile(table.spec(), "s3://b/db/t1/concurrent.parquet", 1024, null, null)).commit(); + IcebergScanPlanProvider provider = + new IcebergScanPlanProvider(Collections.emptyMap(), opsReturning(table)); + + Assertions.assertTrue(provider.planScan(null, + ConnectorScanRequest.builder(emptyAllFiles, Collections.emptyList()).build()).isEmpty(), + "snapshot-derived history must preserve the empty boundary across a concurrent first append"); + } + @Test public void planScanPinnedToTagReadsViaUseRefNotSnapshotId() { // The handle carries BOTH a ref (tag1 -> S1) AND the LATEST snapshot id (s2). The scan must pin by REF diff --git a/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergWritePlanProviderTest.java b/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergWritePlanProviderTest.java index 5a1475df05217c..09671529c71ac4 100644 --- a/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergWritePlanProviderTest.java +++ b/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergWritePlanProviderTest.java @@ -26,6 +26,7 @@ import org.apache.doris.connector.api.handle.ConnectorTransaction; import org.apache.doris.connector.api.handle.ConnectorWriteHandle; import org.apache.doris.connector.api.handle.WriteOperation; +import org.apache.doris.connector.api.mvcc.ConnectorMvccSnapshot; import org.apache.doris.connector.api.write.ConnectorSinkPlan; import org.apache.doris.connector.api.write.ConnectorWritePartitionField; import org.apache.doris.connector.api.write.ConnectorWritePartitionSpec; @@ -53,6 +54,8 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; +import org.apache.iceberg.DataFiles; +import org.apache.iceberg.FileFormat; import org.apache.iceberg.NullOrder; import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.PartitionSpecParser; @@ -63,6 +66,7 @@ import org.apache.iceberg.catalog.Namespace; import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.expressions.Expressions; +import org.apache.iceberg.expressions.Literal; import org.apache.iceberg.inmemory.InMemoryCatalog; import org.apache.iceberg.io.FileIO; import org.apache.iceberg.io.InputFile; @@ -71,6 +75,7 @@ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.EnumSet; @@ -220,7 +225,15 @@ private static WriteSession sessionFor(Table table, RecordingConnectorContext ct private static TIcebergTableSink planSink(Table table, RecordingConnectorContext ctx, ConnectorWriteHandle handle) { - ConnectorSinkPlan plan = providerFor(table, ctx).planWrite(sessionFor(table, ctx), handle); + return planSink(table, ctx, handle, NON_REST_PROPS); + } + + private static TIcebergTableSink planSink(Table table, RecordingConnectorContext ctx, + ConnectorWriteHandle handle, Map properties) { + RecordingIcebergCatalogOps ops = new RecordingIcebergCatalogOps(); + ops.table = table; + ConnectorSinkPlan plan = new IcebergWritePlanProvider(properties, ops, ctx) + .planWrite(sessionFor(table, ctx), handle); Assertions.assertEquals(TDataSinkType.ICEBERG_TABLE_SINK, plan.getDataSink().getType()); return plan.getDataSink().getIcebergTableSink(); } @@ -249,6 +262,162 @@ public void planWriteBuildsInsertSinkWithTableDerivedFields() { Assertions.assertFalse(sink.isSetStaticPartitionValues()); } + @Test + public void planWriteValidatesFullBoundSchemaForPartialInsert() { + Table table = unpartitionedUnsortedTable(freshCatalog()); + List fullBoundSchema = Arrays.asList( + new ConnectorColumn("id", ConnectorType.of("INT"), "", false, null) + .withUniqueId(table.schema().findField("id").fieldId()), + new ConnectorColumn("name", ConnectorType.of("STRING"), "", true, null) + .withUniqueId(table.schema().findField("name").fieldId())); + WriteHandle handle = new WriteHandle(new IcebergTableHandle("db1", "t2")) + .columns(Collections.singletonList(fullBoundSchema.get(0))) + .boundTargetColumns(fullBoundSchema); + + TIcebergTableSink sink = planSink(table, contextWithStorage(), handle); + + Assertions.assertEquals(SchemaParser.toJson(table.schema()), sink.getSchemaJson()); + } + + @Test + public void planWriteAcceptsCanonicalEquivalentScalarDefaults() { + Table table = unpartitionedUnsortedTable(freshCatalog()); + List boundSchema = Arrays.asList( + new ConnectorColumn("id", ConnectorType.of("INT", 0, 0), "", false, null) + .withUniqueId(table.schema().findField("id").fieldId()), + new ConnectorColumn("name", ConnectorType.of("STRING", 0, 0), "", true, null) + .withUniqueId(table.schema().findField("name").fieldId())); + + // Doris scalar defaults are an encoding detail, not an Iceberg schema change. Rejecting this exact + // production conversion shape broke every ordinary Iceberg INSERT in external regression. + Assertions.assertDoesNotThrow(() -> planSink(table, contextWithStorage(), + new WriteHandle(new IcebergTableHandle("db1", "t2")).boundTargetColumns(boundSchema))); + } + + @Test + public void planWriteAcceptsReadFacingTopLevelNullability() { + Table table = unpartitionedUnsortedTable(freshCatalog()); + List boundSchema = Arrays.asList( + new ConnectorColumn("id", ConnectorType.of("INT"), "", true, null) + .withUniqueId(table.schema().findField("id").fieldId()), + new ConnectorColumn("name", ConnectorType.of("STRING"), "", true, null) + .withUniqueId(table.schema().findField("name").fieldId())); + + // Iceberg exposes required top-level fields as nullable Doris scan columns so schema-evolution + // default fill can still produce NULL. The write validator must not treat that read contract as drift. + Assertions.assertDoesNotThrow(() -> planSink(table, contextWithStorage(), + new WriteHandle(new IcebergTableHandle("db1", "t2")).boundTargetColumns(boundSchema))); + } + + @Test + public void planWriteAcceptsDorisPhysicalScalarAliases() { + Schema schema = new Schema( + Types.NestedField.required(1, "amount", Types.DecimalType.of(12, 2)), + Types.NestedField.optional(2, "event_time", Types.TimestampType.withoutZone()), + Types.NestedField.optional(3, "payload", Types.BinaryType.get())); + InMemoryCatalog catalog = freshCatalog(); + Table table = catalog.createTable(TableIdentifier.of("db1", "scalars"), schema, + PartitionSpec.unpartitioned()); + List boundSchema = Arrays.asList( + new ConnectorColumn("amount", ConnectorType.of("DECIMAL64", 12, 2), "", false, null) + .withUniqueId(1), + new ConnectorColumn("event_time", ConnectorType.of("DATETIMEV2", 6, 0), "", true, null) + .withUniqueId(2), + new ConnectorColumn("payload", ConnectorType.of("VARBINARY"), "", true, null) + .withUniqueId(3)); + + // Doris physical decimal widths and unbounded binary encoding must not look like schema evolution. + Map properties = new HashMap<>(NON_REST_PROPS); + properties.put(IcebergConnectorProperties.ENABLE_MAPPING_VARBINARY, "true"); + Assertions.assertDoesNotThrow(() -> planSink(table, contextWithStorage(), + new WriteHandle(new IcebergTableHandle("db1", "scalars")).boundTargetColumns(boundSchema), + properties)); + } + + @Test + public void planWriteRejectsRecreatedNestedFieldWithSameShape() { + Schema nestedSchema = new Schema(Types.NestedField.optional(1, "payload", + Types.StructType.of(Types.NestedField.optional(2, "value", Types.FixedType.ofLength(4))))); + InMemoryCatalog catalog = freshCatalog(); + Table table = catalog.createTable(TableIdentifier.of("db1", "nested"), nestedSchema, + PartitionSpec.unpartitioned()); + ConnectorType boundType = ConnectorType.structOf( + Collections.singletonList("value"), + Collections.singletonList(ConnectorType.of("CHAR", 4, 0)), + Collections.singletonList(true), Collections.singletonList(null)) + .withChildrenFieldIds(Collections.singletonList(2)); + ConnectorColumn bound = new ConnectorColumn("payload", boundType, "", true, null) + .withUniqueId(1); + + table.updateSchema().deleteColumn("payload.value").commit(); + table.updateSchema().addColumn("payload", "value", Types.FixedType.ofLength(4)).commit(); + + // The replacement has the same name/type/ordinal but a new nested field id. Accepting it would write + // the old bound payload under a different Iceberg identity. + Assertions.assertThrows(DorisConnectorException.class, () -> planSink(table, contextWithStorage(), + new WriteHandle(new IcebergTableHandle("db1", "nested")) + .boundTargetColumns(Collections.singletonList(bound)))); + } + + @Test + public void planWriteIgnoresReservedRowLineageColumnsDuringSchemaValidation() { + Table table = unpartitionedUnsortedTable(freshCatalog()); + List fullBoundSchema = Arrays.asList( + new ConnectorColumn("id", ConnectorType.of("INT"), "", false, null) + .withUniqueId(table.schema().findField("id").fieldId()), + new ConnectorColumn("name", ConnectorType.of("STRING"), "", true, null) + .withUniqueId(table.schema().findField("name").fieldId()), + new ConnectorColumn("_row_id", ConnectorType.of("BIGINT"), "", true, null) + .invisible().reservedPassthrough(), + new ConnectorColumn("_last_updated_sequence_number", ConnectorType.of("BIGINT"), "", true, null) + .invisible().reservedPassthrough()); + WriteHandle handle = new WriteHandle(new IcebergTableHandle("db1", "t2")) + .columns(Collections.singletonList(fullBoundSchema.get(0))) + .boundTargetColumns(fullBoundSchema); + + Assertions.assertDoesNotThrow(() -> planSink(table, contextWithStorage(), handle)); + } + + @Test + public void planWriteRejectsRecreatedColumnWithSameNameAndType() { + Table table = unpartitionedUnsortedTable(freshCatalog()); + WriteHandle handle = new WriteHandle(new IcebergTableHandle("db1", "t2")).columns(Arrays.asList( + new ConnectorColumn("id", ConnectorType.of("INT"), "", false, null) + .withUniqueId(table.schema().findField("id").fieldId()), + new ConnectorColumn("name", ConnectorType.of("STRING"), "", true, null) + .withUniqueId(table.schema().findField("name").fieldId()))); + table.updateSchema().deleteColumn("name").commit(); + table.updateSchema().addColumn("name", Types.StringType.get()).commit(); + + Assertions.assertThrows(DorisConnectorException.class, + () -> planSink(table, contextWithStorage(), handle)); + } + + @Test + public void planWriteRejectsSchemaReorderedAfterBinding() { + Table table = unpartitionedUnsortedTable(freshCatalog()); + WriteHandle handle = new WriteHandle(new IcebergTableHandle("db1", "t2")).columns(Arrays.asList( + new ConnectorColumn("id", ConnectorType.of("INT"), "", false, null), + new ConnectorColumn("name", ConnectorType.of("STRING"), "", true, null))); + table.updateSchema().moveFirst("name").commit(); + + DorisConnectorException ex = Assertions.assertThrows(DorisConnectorException.class, + () -> planSink(table, contextWithStorage(), handle)); + Assertions.assertTrue(ex.getMessage().contains("schema changed")); + } + + @Test + public void planWriteRejectsColumnTypeChangedAfterBinding() { + Table table = unpartitionedUnsortedTable(freshCatalog()); + WriteHandle handle = new WriteHandle(new IcebergTableHandle("db1", "t2")).columns(Arrays.asList( + new ConnectorColumn("id", ConnectorType.of("INT"), "", false, null), + new ConnectorColumn("name", ConnectorType.of("STRING"), "", true, null))); + table.updateSchema().updateColumn("id", Types.LongType.get()).commit(); + + Assertions.assertThrows(DorisConnectorException.class, + () -> planSink(table, contextWithStorage(), handle)); + } + // ───────────────────────────── REWRITE: compaction sink (TIcebergTableSink) ───────────────────────────── // // WHY: post-cutover rewrite_data_files reuses the INSERT TIcebergTableSink dialect with two deltas vs @@ -287,6 +456,35 @@ public void planWriteRewriteFv3AppendsRowLineageSchema() { "fv3 rewrite schema-json must include the row-lineage _last_updated_sequence_number field"); } + @Test + public void planWriteRewriteRejectsRequestScopedRowLocatorForV2AndV3() { + assertRewriteRejectsRequestScopedRowLocator(unpartitionedUnsortedTable(freshCatalog())); + assertRewriteRejectsRequestScopedRowLocator(formatVersionThreeTable(freshCatalog())); + } + + private static void assertRewriteRejectsRequestScopedRowLocator(Table table) { + RecordingConnectorContext context = contextWithStorage(); + IcebergWritePlanProvider provider = providerFor(table, context); + WriteSession session = sessionFor(table, context); + IcebergTableHandle tableHandle = new IcebergTableHandle("db1", + IcebergWriterHelper.getFormatVersion(table) >= 3 ? "tv3" : "t2"); + List boundColumns = new ArrayList<>(boundDataColumns(table)); + if (IcebergWriterHelper.getFormatVersion(table) >= 3) { + boundColumns.add(new ConnectorColumn("_row_id", ConnectorType.of("BIGINT"), "", true, null) + .invisible().reservedPassthrough()); + boundColumns.add(new ConnectorColumn( + "_last_updated_sequence_number", ConnectorType.of("BIGINT"), "", true, null) + .invisible().reservedPassthrough()); + } + boundColumns.add(provider.getSyntheticWriteColumns(session, tableHandle).get(0)); + + DorisConnectorException exception = Assertions.assertThrows(DorisConnectorException.class, + () -> provider.planWrite(session, new WriteHandle(tableHandle) + .boundTargetColumns(boundColumns) + .writeOperation(WriteOperation.REWRITE))); + Assertions.assertTrue(exception.getMessage().contains("schema changed")); + } + @Test public void planWriteRewriteRejectsOverwrite() { // REWRITE is a compaction, never a user INSERT OVERWRITE; the BE writer rejects the pairing, so the @@ -589,6 +787,23 @@ public void getWriteSortColumnsForSortedTableMapsIdentityFields() { Assertions.assertTrue(cols.get(0).isNullsFirst()); } + @Test + public void getWriteSortColumnsUsesBoundFieldIdentityInsteadOfLiveOrdinal() { + Table table = partitionedSortedTable(freshCatalog()); + List reversedBoundSchema = Arrays.asList( + new ConnectorColumn("name", ConnectorType.of("STRING"), "", true, null) + .withUniqueId(table.schema().findField("name").fieldId()), + new ConnectorColumn("id", ConnectorType.of("INT"), "", false, null) + .withUniqueId(table.schema().findField("id").fieldId())); + + List cols = providerFor(table, contextWithStorage()) + .getWriteSortColumns(sessionFor(table, contextWithStorage()), + new IcebergTableHandle("db1", "t1"), reversedBoundSchema); + + // Sort positions index the already-bound output. A live ordinal would incorrectly select name here. + Assertions.assertEquals(1, cols.get(0).getColumnIndex()); + } + @Test public void getWriteSortColumnsNullForUnsortedTable() { // null == "no write sort order" (legacy gates setSortInfo on isSorted()) -> the engine emits no @@ -623,6 +838,169 @@ public void getWriteSortColumnsNonNullEmptyForSortOrderWithoutIdentityColumns() Assertions.assertTrue(cols.isEmpty(), "no identity column resolves -> empty list -> empty TSortInfo"); } + @Test + public void planWriteRejectsSortOrderEvolutionAfterPhysicalShaping() { + InMemoryCatalog catalog = freshCatalog(); + TableIdentifier id = TableIdentifier.of("db1", "t2"); + Table plannedTable = unpartitionedUnsortedTable(catalog); + RecordingConnectorContext ctx = contextWithStorage(); + RecordingIcebergCatalogOps ops = new RecordingIcebergCatalogOps(); + ops.table = plannedTable; + IcebergWritePlanProvider provider = new IcebergWritePlanProvider(NON_REST_PROPS, ops, ctx); + WriteSession session = new WriteSession(new IcebergConnectorTransaction(42L, ops, ctx)); + IcebergTableHandle tableHandle = new IcebergTableHandle("db1", "t2"); + String boundMetadataIdentity = provider.getWriteMetadataIdentity(session, tableHandle); + Assertions.assertNull(provider.getWriteSortColumns(session, tableHandle), + "the physical plan must be shaped as unsorted at S0"); + + catalog.loadTable(id).replaceSortOrder().asc("name").commit(); + + DorisConnectorException ex = Assertions.assertThrows(DorisConnectorException.class, + () -> provider.planWrite(session, + new WriteHandle(tableHandle).boundWriteMetadataIdentity(boundMetadataIdentity))); + Assertions.assertTrue(ex.getMessage().contains("write metadata changed"), + "a post-bind sort-order change must fail before files can be labeled with the refreshed order"); + } + + @Test + public void planWriteRejectsPartitionFieldRenameAfterStaticOverwriteBinding() { + InMemoryCatalog catalog = freshCatalog(); + TableIdentifier id = TableIdentifier.of("db1", "t1"); + Table plannedTable = catalog.createTable(id, SCHEMA, + PartitionSpec.builderFor(SCHEMA).identity("id").build()); + RecordingConnectorContext ctx = contextWithStorage(); + RecordingIcebergCatalogOps ops = new RecordingIcebergCatalogOps(); + ops.table = plannedTable; + IcebergWritePlanProvider provider = new IcebergWritePlanProvider(NON_REST_PROPS, ops, ctx); + WriteSession session = new WriteSession(new IcebergConnectorTransaction(42L, ops, ctx)); + IcebergTableHandle tableHandle = new IcebergTableHandle("db1", "t1"); + String boundMetadataIdentity = provider.getWriteMetadataIdentity(session, tableHandle); + + catalog.loadTable(id).updateSpec().renameField("id", "renamed_id").commit(); + + DorisConnectorException ex = Assertions.assertThrows(DorisConnectorException.class, + () -> provider.planWrite(session, new WriteHandle(tableHandle) + .overwrite(true) + .writeContext(Collections.singletonMap("id", "7")) + .boundWriteMetadataIdentity(boundMetadataIdentity))); + Assertions.assertTrue(ex.getMessage().contains("write metadata changed"), + "a post-bind partition-field rename must fail before the stale static spec reaches commit"); + } + + @Test + public void writeMetadataIdentityChangesWithFormatVersion() { + InMemoryCatalog catalog = freshCatalog(); + Table table = unpartitionedUnsortedTable(catalog); + RecordingConnectorContext ctx = contextWithStorage(); + IcebergWritePlanProvider provider = providerFor(table, ctx); + WriteSession session = sessionFor(table, ctx); + IcebergTableHandle handle = new IcebergTableHandle("db1", "t2"); + String v2Identity = provider.getWriteMetadataIdentity(session, handle); + + table.updateProperties().set(TableProperties.FORMAT_VERSION, "3").commit(); + + Assertions.assertNotEquals(v2Identity, provider.getWriteMetadataIdentity(session, handle), + "format version changes the physical write schema and must change the generation fence"); + } + + @Test + public void planWriteRejectsDropRecreateBeforeFirstWritableTableLoad() { + InMemoryCatalog catalog = freshCatalog(); + TableIdentifier identifier = TableIdentifier.of("db1", "t2"); + Table original = unpartitionedUnsortedTable(catalog); + IcebergTableHandle handle = new IcebergTableHandle("db1", "t2"); + // BindSink got U0's schema and identity from one load. Construct the write provider now, but do not + // resolve its sharedWritableTable until after the same-name replacement below. + String originalIdentity = IcebergWritePlanProvider.writeMetadataIdentity(original); + RecordingConnectorContext ctx = contextWithStorage(); + RecordingIcebergCatalogOps ops = new RecordingIcebergCatalogOps(); + ops.table = original; + IcebergWritePlanProvider provider = new IcebergWritePlanProvider(NON_REST_PROPS, ops, ctx); + WriteSession session = new WriteSession(new IcebergConnectorTransaction(42L, ops, ctx)); + + catalog.dropTable(identifier, false); + Table recreated = unpartitionedUnsortedTable(catalog); + ops.table = recreated; + + DorisConnectorException ex = Assertions.assertThrows(DorisConnectorException.class, + () -> provider.planWrite(session, + new WriteHandle(handle).boundWriteMetadataIdentity(originalIdentity))); + Assertions.assertTrue(ex.getMessage().contains("write metadata changed"), + "the first writable-table load must reject U1 instead of moving U0's conflict baseline"); + } + + @Test + public void planWriteRejectsWriteDefaultEvolution() { + InMemoryCatalog catalog = freshCatalog(); + Table table = unpartitionedUnsortedTable(catalog); + table.updateSchema().updateColumnDefault("id", Literal.of(42)).commit(); + List boundColumns = Arrays.asList( + new ConnectorColumn("id", ConnectorType.of("INT"), "", false, "42") + .withUniqueId(table.schema().findField("id").fieldId()), + new ConnectorColumn("name", ConnectorType.of("STRING"), "", true, null) + .withUniqueId(table.schema().findField("name").fieldId())); + + table.updateSchema().updateColumnDefault("id", Literal.of(7)).commit(); + + DorisConnectorException ex = Assertions.assertThrows(DorisConnectorException.class, + () -> planSink(table, contextWithStorage(), + new WriteHandle(new IcebergTableHandle("db1", "t2")) + .boundTargetColumns(boundColumns))); + Assertions.assertTrue(ex.getMessage().contains("schema changed"), + "a statement must retry instead of writing a value materialized from the stale default"); + } + + @Test + public void planRewriteRejectsV2BoundSchemaAtV3Planning() { + InMemoryCatalog catalog = freshCatalog(); + Table table = unpartitionedUnsortedTable(catalog); + List v2BoundSchema = boundDataColumns(table); + table.updateProperties().set(TableProperties.FORMAT_VERSION, "3").commit(); + RecordingConnectorContext ctx = contextWithStorage(); + IcebergWritePlanProvider provider = providerFor(table, ctx); + WriteSession session = sessionFor(table, ctx); + IcebergTableHandle tableHandle = new IcebergTableHandle("db1", "t2"); + + DorisConnectorException ex = Assertions.assertThrows(DorisConnectorException.class, + () -> provider.planWrite(session, new WriteHandle(tableHandle) + .boundTargetColumns(v2BoundSchema) + .boundWriteMetadataIdentity(provider.getWriteMetadataIdentity(session, tableHandle)) + .writeOperation(WriteOperation.REWRITE))); + Assertions.assertTrue(ex.getMessage().contains("write metadata changed"), + "a v3 rewrite sink must not consume output bound without row-lineage columns"); + } + + @Test + public void planMergeRejectsV2BoundSchemaAtV3Planning() { + InMemoryCatalog catalog = freshCatalog(); + Table table = unpartitionedUnsortedTable(catalog); + RecordingConnectorContext ctx = contextWithStorage(); + IcebergWritePlanProvider provider = providerFor(table, ctx); + WriteSession session = sessionFor(table, ctx); + IcebergTableHandle tableHandle = new IcebergTableHandle("db1", "t2"); + List v2DataColumns = boundDataColumns(table); + List v2BoundSchemaWithRowId = Arrays.asList( + v2DataColumns.get(0), v2DataColumns.get(1), + provider.getSyntheticWriteColumns(session, tableHandle).get(0)); + table.updateProperties().set(TableProperties.FORMAT_VERSION, "3").commit(); + + DorisConnectorException ex = Assertions.assertThrows(DorisConnectorException.class, + () -> provider.planWrite(session, new WriteHandle(tableHandle) + .boundTargetColumns(v2BoundSchemaWithRowId) + .boundWriteMetadataIdentity(provider.getWriteMetadataIdentity(session, tableHandle)) + .writeOperation(WriteOperation.MERGE))); + Assertions.assertTrue(ex.getMessage().contains("write metadata changed"), + "a v3 merge sink must not advertise lineage columns absent from the bound output"); + } + + private static List boundDataColumns(Table table) { + return Arrays.asList( + new ConnectorColumn("id", ConnectorType.of("INT"), "", false, null) + .withUniqueId(table.schema().findField("id").fieldId()), + new ConnectorColumn("name", ConnectorType.of("STRING"), "", true, null) + .withUniqueId(table.schema().findField("name").fieldId())); + } + // ───────────────────────────── getWritePartitioning (connector declares, ② C3b-core) ───────────────────────────── // // WHY: post-flip the iceberg merge-write distribution (DistributionSpecMerge) is built fe-core-side, but @@ -970,6 +1348,38 @@ public void planWriteThreadsPinnedReadSnapshotFromHandleToTransaction() { "planWrite must thread the handle's pinned read snapshot into beginWrite as baseSnapshotId"); } + @Test + public void planMergePreservesExplicitlyEmptyReadAcrossConcurrentFirstAppend() { + InMemoryCatalog catalog = freshCatalog(); + TableIdentifier id = TableIdentifier.of("db1", "tv2"); + Table empty = catalog.createTable(id, SCHEMA, PartitionSpec.unpartitioned(), + Collections.singletonMap("format-version", "2")); + RecordingConnectorContext ctx = contextWithStorage(); + RecordingIcebergCatalogOps ops = new RecordingIcebergCatalogOps(); + ops.table = empty; + IcebergConnectorMetadata metadata = new IcebergConnectorMetadata(ops, Collections.emptyMap(), ctx); + ConnectorMvccSnapshot emptySnapshot = metadata.beginQuerySnapshot(null, + new IcebergTableHandle("db1", "tv2")).orElseThrow(AssertionError::new); + IcebergTableHandle emptyPinnedHandle = (IcebergTableHandle) metadata.applySnapshot( + null, new IcebergTableHandle("db1", "tv2"), emptySnapshot); + + empty.newAppend().appendFile(DataFiles.builder(PartitionSpec.unpartitioned()) + .withPath("s3://bucket/db1/tv2/concurrent.parquet") + .withFileSizeInBytes(100) + .withRecordCount(1) + .withFormat(FileFormat.PARQUET) + .build()).commit(); + ops.table = catalog.loadTable(id); + Assertions.assertNotNull(ops.table.currentSnapshot(), "the concurrent append must create S1"); + + IcebergConnectorTransaction txn = new IcebergConnectorTransaction(42L, ops, ctx); + providerFor(ops.table, ctx).planWrite(new WriteSession(txn), + new WriteHandle(emptyPinnedHandle).writeOperation(WriteOperation.MERGE)); + + Assertions.assertNull(txn.getBaseSnapshotId(), + "an explicitly empty read must leave RowDelta validation unbounded across the first append"); + } + // ───────────────────────────── MERGE sink (TIcebergMergeSink) ───────────────────────────── // // WHY: UPDATE and MERGE both write the TIcebergMergeSink dialect. Two parity traps vs the table/delete @@ -1129,6 +1539,9 @@ private static final class WriteHandle implements ConnectorWriteHandle { private WriteOperation writeOperation = WriteOperation.INSERT; private Optional branchName = Optional.empty(); private boolean requireMergeCardinalityCheck; + private List columns = Collections.emptyList(); + private List boundTargetColumns; + private String boundWriteMetadataIdentity; WriteHandle(ConnectorTableHandle tableHandle) { this.tableHandle = tableHandle; @@ -1139,6 +1552,31 @@ WriteHandle branch(String v) { return this; } + WriteHandle columns(List v) { + this.columns = v; + return this; + } + + WriteHandle boundTargetColumns(List v) { + this.boundTargetColumns = v; + return this; + } + + WriteHandle boundWriteMetadataIdentity(String v) { + this.boundWriteMetadataIdentity = v; + return this; + } + + @Override + public String getBoundWriteMetadataIdentity() { + return boundWriteMetadataIdentity; + } + + @Override + public List getBoundTargetColumns() { + return boundTargetColumns == null ? columns : boundTargetColumns; + } + @Override public Optional getBranchName() { return branchName; @@ -1186,7 +1624,7 @@ public ConnectorTableHandle getTableHandle() { @Override public List getColumns() { - return Collections.emptyList(); + return columns; } @Override diff --git a/fe/fe-core/src/main/java/org/apache/doris/common/proc/IndexSchemaProcNode.java b/fe/fe-core/src/main/java/org/apache/doris/common/proc/IndexSchemaProcNode.java index 7578685a771d64..0b0f7d67277f54 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/common/proc/IndexSchemaProcNode.java +++ b/fe/fe-core/src/main/java/org/apache/doris/common/proc/IndexSchemaProcNode.java @@ -21,6 +21,7 @@ import org.apache.doris.common.AnalysisException; import org.apache.doris.common.FeConstants; import org.apache.doris.qe.ConnectContext; +import org.apache.doris.qe.SqlModeHelper; import com.google.common.base.Preconditions; import com.google.common.base.Strings; @@ -89,7 +90,8 @@ public static ProcResult createResult(List schema, Set bfColumns String extraStr = StringUtils.join(extras, ","); List rowList = Lists.newArrayList(column.getDisplayName(), - column.getOriginType().hideVersionForVersionColumn(true, showNestedComment), + column.getOriginType().hideVersionForVersionColumn( + true, showNestedComment, SqlModeHelper.hasNoBackSlashEscapes()), column.isAllowNull() ? "Yes" : "No", ((Boolean) column.isKey()).toString(), column.getDefaultValue() == null diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/connector/converter/ConnectorColumnConverter.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/connector/converter/ConnectorColumnConverter.java index 8d209646121990..1f4f895f38acad 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/connector/converter/ConnectorColumnConverter.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/connector/converter/ConnectorColumnConverter.java @@ -97,11 +97,10 @@ public static Column convertColumn(ConnectorColumn cc) { if (cc.isReservedPassthrough()) { column.setReservedPassthrough(true); } - // Stamp the nested (STRUCT/ARRAY/MAP) child column tree with the per-field ids the connector carried - // on the ConnectorType (iceberg), mirroring legacy IcebergUtils.updateIcebergColumnUniqueId's - // recursive set. The BE field-id scan path matches a pruned nested leaf by id; a -1 leaf is skipped - // and returns NULL. Inert for connectors that don't carry field ids (getChildFieldId returns -1). - applyNestedFieldIds(column, cc.getType()); + // Stamp the nested (STRUCT/ARRAY/MAP) child column tree with connector field metadata. ArrayType + // cannot retain required element semantics itself, so the child Column is the canonical round-trip + // carrier; field ids are also needed by the BE nested-field scan path. + applyNestedFieldMetadata(column, cc.getType()); return column; } @@ -110,9 +109,10 @@ public static Column convertColumn(ConnectorColumn cc) { * per-child field ids carried on {@code type} ({@link ConnectorType#getChildFieldId(int)}). The Doris * child column order built by {@code Column.createChildrenColumn} matches the {@link ConnectorType} * children order (array element / map key,value / struct fields-in-order), so a parallel walk aligns them. - * Only sets a child whose carried id is {@code >= 0}, leaving others at the default -1. + * Nullability is always copied because ARRAY/MAP type objects cannot consistently retain it. A field id is + * set only when the connector carries one ({@code >= 0}), leaving other ids at the default -1. */ - private static void applyNestedFieldIds(Column column, ConnectorType type) { + private static void applyNestedFieldMetadata(Column column, ConnectorType type) { List childColumns = column.getChildren(); if (childColumns == null || childColumns.isEmpty()) { return; @@ -121,11 +121,12 @@ private static void applyNestedFieldIds(Column column, ConnectorType type) { int n = Math.min(childColumns.size(), childTypes.size()); for (int i = 0; i < n; i++) { Column childColumn = childColumns.get(i); + childColumn.setIsAllowNull(type.isChildNullable(i)); int childFieldId = type.getChildFieldId(i); if (childFieldId >= 0) { childColumn.setUniqueId(childFieldId); } - applyNestedFieldIds(childColumn, childTypes.get(i)); + applyNestedFieldMetadata(childColumn, childTypes.get(i)); } } @@ -148,8 +149,10 @@ public static List toConnectorColumns(List columns) { * and the connector could not tell an aggregated/auto-inc column apart from a plain one.

*/ public static ConnectorColumn toConnectorColumn(Column col) { - ConnectorType connectorType = toConnectorType(col.getType()); - return new ConnectorColumn( + // Preserve the complete field-id tree when a bound write schema crosses the connector boundary. + // Iceberg field ids, rather than nested names, are the stable identity across schema evolution. + ConnectorType connectorType = toConnectorType(col); + ConnectorColumn result = new ConnectorColumn( col.getName(), connectorType, col.getComment(), @@ -162,6 +165,39 @@ public static ConnectorColumn toConnectorColumn(Column col) { // omit-preserves-metadata (an omitted NULL/NOT NULL never widens the field; an omitted COMMENT // keeps the current doc). Inert for connectors / paths that don't read them. .withSpecified(col.isNullableSpecified(), col.isCommentSpecified()); + if (!col.isVisible()) { + result = result.invisible(); + } + if (col.getUniqueId() >= 0) { + result = result.withUniqueId(col.getUniqueId()); + } + if (col.isReservedPassthrough()) { + result = result.reservedPassthrough(); + } + return result; + } + + private static ConnectorType toConnectorType(Column column) { + ConnectorType type = toConnectorType(column.getType()); + List childColumns = column.getChildren(); + if (childColumns == null || childColumns.size() != type.getChildren().size()) { + return type; + } + List childTypes = new ArrayList<>(childColumns.size()); + List childIds = new ArrayList<>(childColumns.size()); + List childNullable = new ArrayList<>(childColumns.size()); + List childComments = new ArrayList<>(childColumns.size()); + List childCommentSpecified = new ArrayList<>(childColumns.size()); + for (int i = 0; i < childColumns.size(); i++) { + Column child = childColumns.get(i); + childTypes.add(toConnectorType(child)); + childIds.add(child.getUniqueId()); + childNullable.add(child.isAllowNull()); + childComments.add(type.getChildComment(i)); + childCommentSpecified.add(type.isChildCommentSpecified(i)); + } + return new ConnectorType(type.getTypeName(), type.getPrecision(), type.getScale(), childTypes, + type.getFieldNames(), childNullable, childComments, childIds, childCommentSpecified); } /** @@ -207,15 +243,28 @@ public static ConnectorType toConnectorType(Type dorisType) { // CHAR/VARCHAR store their length in `len`, not `precision`; encode it // into the ConnectorType precision field (matching convertScalarType and // the connector type convention) so CREATE TABLE requests keep the length. + if (primitiveType == PrimitiveType.VARBINARY + && scalar.getLength() == ScalarType.MAX_VARBINARY_LENGTH) { + // Doris materializes an unbounded connector VARBINARY with the maximum internal length. + // Collapse it on the reverse path so an unchanged Iceberg BINARY schema compares equal. + return ConnectorType.of(primitiveType.toString()); + } if (primitiveType == PrimitiveType.CHAR - || primitiveType == PrimitiveType.VARCHAR) { + || primitiveType == PrimitiveType.VARCHAR + || primitiveType == PrimitiveType.VARBINARY) { return ConnectorType.of(primitiveType.toString(), scalar.getLength(), 0); } - return ConnectorType.of( - primitiveType.toString(), - scalar.getScalarPrecision(), - scalar.getScalarScale()); + if (primitiveType.isDecimalV3Type() || primitiveType == PrimitiveType.DECIMALV2) { + return ConnectorType.of(primitiveType.toString(), + scalar.getScalarPrecision(), scalar.getScalarScale()); + } + if (primitiveType == PrimitiveType.DATETIMEV2 || primitiveType == PrimitiveType.TIMESTAMPTZ) { + return ConnectorType.of(primitiveType.toString(), scalar.getScalarScale(), 0); + } + // Parameterless Doris scalars expose 0/0 through ScalarType, while connector schemas use -1/-1. + // Canonicalizing them prevents an unchanged schema from being mistaken for concurrent drift. + return ConnectorType.of(primitiveType.toString()); } else { return ConnectorType.of(dorisType.toString(), -1, -1); } @@ -252,7 +301,8 @@ private static Type convertMapType(ConnectorType ct) { if (children.size() < 2) { return new MapType(Type.NULL, Type.NULL); } - return new MapType(convertType(children.get(0)), convertType(children.get(1))); + return new MapType(convertType(children.get(0)), convertType(children.get(1)), + ct.isChildNullable(0), ct.isChildNullable(1)); } private static Type convertStructType(ConnectorType ct) { diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalTable.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalTable.java index e4c75c3ad03705..c6f4a48867e998 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalTable.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalTable.java @@ -569,7 +569,8 @@ protected PluginDrivenSchemaCacheValue toSchemaCacheValue(ConnectorMetadata meta } } return new PluginDrivenSchemaCacheValue(columns, partitionColumns, partitionColumnRemoteNames, - tableSchema.getProperties(), tableSchema.getTableCapabilities()); + tableSchema.getProperties(), tableSchema.getTableCapabilities(), + tableSchema.getWriteMetadataIdentity()); } @Override @@ -735,6 +736,54 @@ private List appendSyntheticWriteColumns(List schema) { return result; } + /** Immutable write-facing views captured from one schema-cache generation. */ + public static final class WriteSchemaSnapshot { + private final List baseSchema; + private final List fullSchema; + private final List partitionColumns; + private final String writeMetadataIdentity; + + private WriteSchemaSnapshot(List baseSchema, List fullSchema, + List partitionColumns, String writeMetadataIdentity) { + this.baseSchema = Collections.unmodifiableList(new ArrayList<>(baseSchema)); + this.fullSchema = Collections.unmodifiableList(new ArrayList<>(fullSchema)); + this.partitionColumns = Collections.unmodifiableList(new ArrayList<>(partitionColumns)); + this.writeMetadataIdentity = writeMetadataIdentity; + } + + public List getBaseSchema() { + return baseSchema; + } + + public List getFullSchema() { + return fullSchema; + } + + public List getPartitionColumns() { + return partitionColumns; + } + + public String getWriteMetadataIdentity() { + return writeMetadataIdentity; + } + } + + /** + * Captures schema and partition identities from one cache value for write binding. Reading them through + * separate table APIs can straddle a concurrent refresh and make the planner hash an older output by a + * newer column ordinal. + */ + public WriteSchemaSnapshot getWriteSchemaSnapshot() { + makeSureInitialized(); + PluginDrivenSchemaCacheValue value = getSchemaCacheValue(Optional.empty()) + .map(PluginDrivenSchemaCacheValue.class::cast) + .orElseGet(() -> new PluginDrivenSchemaCacheValue( + Collections.emptyList(), Collections.emptyList(), Collections.emptyList())); + List baseSchema = value.getSchema(); + return new WriteSchemaSnapshot(baseSchema, appendSyntheticWriteColumns(baseSchema), + value.getPartitionColumns(), value.getWriteMetadataIdentity()); + } + /** * Fetches the connector's declared synthetic write columns for this table, in engine-neutral form. * Degrades to an empty list on any miss (non-plugin catalog, a read-only connector with no write-plan diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenSchemaCacheValue.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenSchemaCacheValue.java index d7a12be9b01d8f..1cd655fd50954a 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenSchemaCacheValue.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenSchemaCacheValue.java @@ -60,25 +60,34 @@ public class PluginDrivenSchemaCacheValue extends SchemaCacheValue { // rationale as tableProperties: the ConnectorTableSchema is transient, so the schema cache is the // carrier. Empty for every connector that does not refine per table. private final Set tableCapabilities; + private final String writeMetadataIdentity; public PluginDrivenSchemaCacheValue(List schema, List partitionColumns, List partitionColumnRemoteNames) { - this(schema, partitionColumns, partitionColumnRemoteNames, Collections.emptyMap()); + this(schema, partitionColumns, partitionColumnRemoteNames, Collections.emptyMap(), + Collections.emptySet(), null); } public PluginDrivenSchemaCacheValue(List schema, List partitionColumns, List partitionColumnRemoteNames, Map tableProperties) { - this(schema, partitionColumns, partitionColumnRemoteNames, tableProperties, Collections.emptySet()); + this(schema, partitionColumns, partitionColumnRemoteNames, tableProperties, Collections.emptySet(), null); } public PluginDrivenSchemaCacheValue(List schema, List partitionColumns, List partitionColumnRemoteNames, Map tableProperties, Set tableCapabilities) { + this(schema, partitionColumns, partitionColumnRemoteNames, tableProperties, tableCapabilities, null); + } + + public PluginDrivenSchemaCacheValue(List schema, List partitionColumns, + List partitionColumnRemoteNames, Map tableProperties, + Set tableCapabilities, String writeMetadataIdentity) { super(schema); this.partitionColumns = partitionColumns; this.partitionColumnRemoteNames = partitionColumnRemoteNames; this.tableProperties = tableProperties == null ? Collections.emptyMap() : tableProperties; this.tableCapabilities = tableCapabilities == null ? Collections.emptySet() : tableCapabilities; + this.writeMetadataIdentity = writeMetadataIdentity; } public List getPartitionColumns() { @@ -96,4 +105,8 @@ public Map getTableProperties() { public Set getTableCapabilities() { return tableCapabilities; } + + public String getWriteMetadataIdentity() { + return writeMetadataIdentity; + } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java index c09072174ce2d8..8fcfc860da78e6 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java @@ -555,7 +555,8 @@ public PlanFragment visitPhysicalExternalRowLevelDeleteSink( // TIcebergDeleteSink dialect. No output-expr / materialized-name loop is needed: the row id reaches // BE as the __DORIS_ICEBERG_ROWID_COL__ block column (a real hidden column), and viceberg_delete_sink // resolves it by block-name, not by output-expr name. - rootFragment.setSink(buildPluginRowLevelDmlSink(deleteSink, WriteOperation.DELETE, false)); + rootFragment.setSink(buildPluginRowLevelDmlSink(deleteSink, WriteOperation.DELETE, false, + deleteSink.getBoundWriteMetadataIdentity())); return rootFragment; } @@ -592,7 +593,7 @@ public PlanFragment visitPhysicalExternalRowLevelMergeSink( // SQL MERGE INTO carries the cardinality requirement onto the write handle; UPDATE shares this // sink dialect but has no such rule, so it threads false (see PhysicalExternalRowLevelMergeSink). rootFragment.setSink(buildPluginRowLevelDmlSink(mergeSink, WriteOperation.MERGE, - mergeSink.isRequireMergeCardinalityCheck())); + mergeSink.isRequireMergeCardinalityCheck(), mergeSink.getBoundWriteMetadataIdentity())); return rootFragment; } @@ -609,7 +610,7 @@ public PlanFragment visitPhysicalExternalRowLevelMergeSink( */ private PluginDrivenTableSink buildPluginRowLevelDmlSink( PhysicalBaseExternalTableSink sink, WriteOperation writeOperation, - boolean requireMergeCardinalityCheck) { + boolean requireMergeCardinalityCheck, String boundWriteMetadataIdentity) { PluginDrivenExternalTable targetTable = (PluginDrivenExternalTable) sink.getTargetTable(); PluginDrivenExternalCatalog catalog = (PluginDrivenExternalCatalog) targetTable.getCatalog(); @@ -621,9 +622,7 @@ private PluginDrivenTableSink buildPluginRowLevelDmlSink( // __DORIS_ICEBERG_ROWID_COL__ STRUCT, and the target may hold ARRAY/MAP/STRUCT data columns. // Naming only the tag would drop the children and yield a childless (invalid) complex type. List connectorColumns = sink.getCols().stream() - .map(col -> new ConnectorColumn(col.getName(), - ConnectorColumnConverter.toConnectorType(col.getType()), - null, col.isAllowNull(), null)) + .map(PhysicalPlanTranslator::toWriteConnectorColumn) .collect(java.util.stream.Collectors.toList()); // Resolve the table handle first so BOTH the write-admission gate and the write provider are chosen @@ -648,11 +647,11 @@ private PluginDrivenTableSink buildPluginRowLevelDmlSink( } providerTableHandle = PluginDrivenScanNode.applyMvccSnapshotPin( metadata, connSession, providerTableHandle, MvccUtil.getSnapshotFromContext(targetTable)); - // writeSortInfo == null: a row-level DML has no engine-resolved write sort (MERGE's sort lives in the // connector's TIcebergMergeSink.sort_fields, DELETE is unsorted). return new PluginDrivenTableSink(targetTable, writePlanProvider, connSession, - providerTableHandle, connectorColumns, null, writeOperation, requireMergeCardinalityCheck); + providerTableHandle, connectorColumns, connectorColumns, null, writeOperation, + requireMergeCardinalityCheck, boundWriteMetadataIdentity); } @Override @@ -676,10 +675,19 @@ public PlanFragment visitPhysicalConnectorTableSink( // converted (see the row-level DML arm): a bare primitive tag drops an ARRAY/MAP/STRUCT // column's children and yields a childless, invalid complex type. List connectorColumns = connectorTableSink.getCols().stream() - .map(col -> new ConnectorColumn(col.getName(), - ConnectorColumnConverter.toConnectorType(col.getType()), - null, col.isAllowNull(), null)) + .map(PhysicalPlanTranslator::toWriteConnectorColumn) + .collect(java.util.stream.Collectors.toList()); + List boundTargetColumns = connectorTableSink.getBoundTargetSchema().stream() + .map(PhysicalPlanTranslator::toWriteConnectorColumn) .collect(java.util.stream.Collectors.toList()); + // Sort ordinals are consumed against the sink output. BindSink puts positional writes in physical + // bound-schema order, while name-mapped writes keep user order. Preserve that coordinate space so + // partial/static INSERTs cannot sort another slot. + List boundOutputColumns = targetTable.requiresFullSchemaWriteOrder() + ? connectorTableSink.getBoundTargetSchema().stream() + .map(PhysicalPlanTranslator::toWriteConnectorColumn) + .collect(java.util.stream.Collectors.toList()) + : connectorColumns; // Every write-capable connector builds its own opaque TDataSink via its write-plan // provider (jdbc / maxcompute / iceberg). A connector whose declared write operations do @@ -712,12 +720,16 @@ public PlanFragment visitPhysicalConnectorTableSink( providerTableHandle = PluginDrivenScanNode.applyMvccSnapshotPin( metadata, connSession, providerTableHandle, MvccUtil.getSnapshotFromContext(targetTable)); + // Preserve the generation captured from the exact remote table load that supplied the bound schema. + // A live lookup here would silently move the fence after a concurrent drop/recreate. + String boundWriteMetadataIdentity = connectorTableSink.getBoundWriteMetadataIdentity(); + // The connector declares its write-sort columns (e.g. an iceberg WRITE ORDERED BY) as positions // into the sink's full-schema output; the engine resolves them to bound slots and builds the // TSortInfo here (the connector's planWrite has no bound exprs). Empty for connectors with no // write sort (jdbc/maxcompute) -> null, byte-identical unsorted sink. TSortInfo writeSortInfo = buildConnectorWriteSortInfo( - writePlanProvider.getWriteSortColumns(connSession, providerTableHandle), + writePlanProvider.getWriteSortColumns(connSession, providerTableHandle, boundOutputColumns), connectorTableSink, context); // A distributed rewrite_data_files INSERT-SELECT threads WriteOperation.REWRITE so the connector's @@ -726,13 +738,22 @@ public PlanFragment visitPhysicalConnectorTableSink( // an instanceof Iceberg. Ordinary connector INSERTs keep WriteOperation.INSERT (byte-identical). WriteOperation writeOperation = connectorTableSink.isRewrite() ? WriteOperation.REWRITE : WriteOperation.INSERT; + // The write list can omit explicit/static-partition columns, but schema-drift validation must + // retain the complete generation captured by BindSink instead of comparing that subset. PluginDrivenTableSink providerSink = new PluginDrivenTableSink(targetTable, - writePlanProvider, connSession, providerTableHandle, connectorColumns, writeSortInfo, - writeOperation); + writePlanProvider, connSession, providerTableHandle, connectorColumns, + boundTargetColumns, writeSortInfo, writeOperation, false, + boundWriteMetadataIdentity); rootFragment.setSink(providerSink); return rootFragment; } + private static ConnectorColumn toWriteConnectorColumn(Column column) { + // Use the shared recursive conversion so write validation receives nested field identities as well + // as the root id; rebuilding only the root silently accepted drop-and-recreate nested fields. + return ConnectorColumnConverter.toConnectorColumn(column); + } + private TSortInfo buildConnectorWriteSortInfo(List sortColumns, PhysicalConnectorTableSink connectorTableSink, PlanTranslatorContext context) { // null == no write sort order -> no TSortInfo (jdbc/maxcompute, unsorted iceberg). A non-null diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSink.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSink.java index 1db75dfd0ef634..98454b08fbed2d 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSink.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSink.java @@ -358,6 +358,14 @@ private static Map getColumnToOutput( MatchingContext> ctx, TableIf table, boolean isPartialUpdate, boolean isDeletePartialUpdate, LogicalTableSink boundSink, LogicalPlan child) { + return getColumnToOutput(ctx, table, isPartialUpdate, isDeletePartialUpdate, + boundSink, child, sinkTargetFullSchema(boundSink.getTargetTable())); + } + + private static Map getColumnToOutput( + MatchingContext> ctx, + TableIf table, boolean isPartialUpdate, boolean isDeletePartialUpdate, + LogicalTableSink boundSink, LogicalPlan child, List targetFullSchema) { // we need to insert all the columns of the target table // although some columns are not mentions. // so we add a projects to supply the default value. @@ -373,7 +381,7 @@ private static Map getColumnToOutput( List materializedViewColumn = Lists.newArrayList(); List shadowColumns = Lists.newArrayList(); // generate slots not mentioned in sql, mv slots and shaded slots. - for (Column column : sinkTargetFullSchema(boundSink.getTargetTable())) { + for (Column column : targetFullSchema) { if (column.isGeneratedColumn()) { generatedColumns.add(column); continue; @@ -665,11 +673,27 @@ private static List sinkTargetFullSchema(TableIf table) { return table.getFullSchema(); } - private static Column connectorSinkTargetColumn(PluginDrivenExternalTable table, String name) { - return sinkTargetFullSchema(table).stream() - .filter(column -> name.equalsIgnoreCase(column.getName())) - .findFirst() - .orElse(null); + private static final class ConnectorSinkTargetSchema { + private final List fullSchema; + private final List partitionColumns; + private final String writeMetadataIdentity; + private final Map columnsByName; + + private ConnectorSinkTargetSchema(PluginDrivenExternalTable table) { + // A bind must observe one coherent remote schema generation across target and partition lookups. + PluginDrivenExternalTable.WriteSchemaSnapshot snapshot = table.getWriteSchemaSnapshot(); + this.fullSchema = ImmutableList.copyOf(snapshot.getFullSchema()); + this.partitionColumns = ImmutableList.copyOf(snapshot.getPartitionColumns()); + this.writeMetadataIdentity = snapshot.getWriteMetadataIdentity(); + this.columnsByName = Maps.newTreeMap(String.CASE_INSENSITIVE_ORDER); + for (Column column : fullSchema) { + columnsByName.put(column.getName(), column); + } + } + + private Column getColumn(String name) { + return columnsByName.get(name); + } } /** @@ -748,13 +772,13 @@ private void checkConnectorWritePartitionNames(PluginDrivenExternalTable table, * Resolves the user-typed static-partition column names ({@code PARTITION(TS_DATE='x')}) to their canonical * schema names ({@code ts_date}), and rejects a column named twice. * - *

Resolution goes through {@link org.apache.doris.datasource.ExternalTable#getColumn}, which already - * matches with {@code equalsIgnoreCase} — the same lookup the two sibling statements in - * {@link #bindConnectorTableSink} use (the materialize block and the explicit-column-list bind). Only the - * exclusion filter in {@link #selectConnectorSinkBindColumns} compared raw names, so this removes an - * inconsistency rather than introducing a case rule into the engine. It is safe because no plugin-driven - * schema may hold two columns differing only by case ({@code SchemaCacheValue.validateSchema} rejects that - * on every schema load), so the fold cannot merge two distinct columns.

+ *

Resolution uses the same case-insensitive rule as + * {@link org.apache.doris.datasource.ExternalTable#getColumn}, over the single latest target-schema snapshot + * captured by {@link #bindConnectorTableSink}. Only the exclusion filter in + * {@link #selectConnectorSinkBindColumns} compared raw names, so this removes an inconsistency rather than + * introducing a case rule into the engine. It is safe because no plugin-driven schema may hold two columns + * differing only by case ({@code SchemaCacheValue.validateSchema} rejects that on every schema load), so the + * fold cannot merge two distinct columns.

* *

A name that resolves to no column is kept VERBATIM: on iceberg the PARTITION clause names a partition * FIELD (e.g. {@code category_bucket} for {@code bucket(4, category)}), which is not a table column. The @@ -768,12 +792,17 @@ private void checkConnectorWritePartitionNames(PluginDrivenExternalTable table, @VisibleForTesting static Set canonicalStaticPartitionColNames(PluginDrivenExternalTable table, Map staticPartitions) { + return canonicalStaticPartitionColNames(new ConnectorSinkTargetSchema(table), staticPartitions); + } + + private static Set canonicalStaticPartitionColNames(ConnectorSinkTargetSchema targetSchema, + Map staticPartitions) { if (staticPartitions == null || staticPartitions.isEmpty()) { return Sets.newHashSet(); } Set canonical = Sets.newLinkedHashSet(); for (String name : staticPartitions.keySet()) { - Column column = connectorSinkTargetColumn(table, name); + Column column = targetSchema.getColumn(name); if (!canonical.add(column != null ? column.getName() : name)) { throw new AnalysisException("Duplicate partition column: " + name); } @@ -786,6 +815,7 @@ private Plan bindConnectorTableSink(MatchingContext pair = bind(ctx.cascadesContext, sink); ExternalDatabase database = pair.first; PluginDrivenExternalTable table = pair.second; + ConnectorSinkTargetSchema targetSchema = new ConnectorSinkTargetSchema(table); LogicalPlan child = ((LogicalPlan) sink.child()); // Static-partition columns (e.g. MaxCompute `PARTITION(pt='x')`) carry their value via the @@ -806,7 +836,7 @@ private Plan bindConnectorTableSink(MatchingContext targetWriteSchema = targetSchema.fullSchema.stream() + .filter(column -> isConnectorSinkWriteColumn(column, sink.isRewrite())) + .collect(ImmutableList.toImmutableList()); + if (sink.isRewrite()) { + List rewriteOutputs = selectConnectorRewriteOutputs( + targetWriteSchema, child.getOutput()); + if (!rewriteOutputs.equals(child.getOutput())) { + child = new LogicalProject<>(rewriteOutputs, child); + } + } List bindColumns = selectConnectorSinkBindColumns( - table, sink.getColNames(), staticPartitionColNames, sink.isRewrite()); + table, targetSchema, sink.getColNames(), staticPartitionColNames, sink.isRewrite()); LogicalConnectorTableSink boundSink = new LogicalConnectorTableSink<>( database, table, + targetWriteSchema, + targetSchema.partitionColumns, + targetSchema.writeMetadataIdentity, bindColumns, child.getOutput().stream() .map(NamedExpression.class::cast) @@ -844,7 +887,8 @@ private Plan bindConnectorTableSink(MatchingContext columnToOutput = getColumnToOutput(ctx, table, false, false, boundSink, child); + Map columnToOutput = getColumnToOutput( + ctx, table, false, false, boundSink, child, targetWriteSchema); if (table.materializeStaticPartitionValues() && !staticPartitionColNames.isEmpty()) { // Connectors that consume the partition value FROM THE ROW must write the static partition value // INTO the data column: getColumnToOutput excluded it from the bound columns and NULL-filled it, @@ -855,7 +899,7 @@ private Plan bindConnectorTableSink(MatchingContext entry : staticPartitions.entrySet()) { - Column column = connectorSinkTargetColumn(table, entry.getKey()); + Column column = targetSchema.getColumn(entry.getKey()); if (column != null) { Expression castExpr = TypeCoercionUtils.castIfNotSameType( entry.getValue(), DataType.fromCatalogType(column.getType())); @@ -865,22 +909,8 @@ private Plan bindConnectorTableSink(MatchingContext targetFullSchema = sinkTargetFullSchema(table); - List writeSchema = sink.isRewrite() - ? targetFullSchema - : targetFullSchema.stream() - .filter(Column::isVisible) - .collect(ImmutableList.toImmutableList()); LogicalProject fullOutputProject = - getOutputProjectByCoercion(writeSchema, child, columnToOutput); + getOutputProjectByCoercion(targetWriteSchema, child, columnToOutput); return boundSink.withChildAndUpdateOutput(fullOutputProject); } // Name-mapped connector tables (JDBC / ES): keep columns in user-specified order because the @@ -901,9 +931,9 @@ private Plan bindConnectorTableSink(MatchingContext{@code staticPartitionColNames} must already be canonicalized by * {@link #canonicalStaticPartitionColNames}, so both the exclusion filter and the explicit-column-list @@ -912,14 +942,21 @@ private Plan bindConnectorTableSink(MatchingContext selectConnectorSinkBindColumns(PluginDrivenExternalTable table, List colNames, Set staticPartitionColNames, boolean isRewrite) { + return selectConnectorSinkBindColumns(table, new ConnectorSinkTargetSchema(table), + colNames, staticPartitionColNames, isRewrite); + } + + private static List selectConnectorSinkBindColumns(PluginDrivenExternalTable table, + ConnectorSinkTargetSchema targetSchema, List colNames, + Set staticPartitionColNames, boolean isRewrite) { if (colNames.isEmpty()) { - return sinkTargetFullSchema(table).stream() + return targetSchema.fullSchema.stream() .filter(col -> !staticPartitionColNames.contains(col.getName())) - .filter(col -> isRewrite || col.isVisible()) + .filter(col -> isConnectorSinkWriteColumn(col, isRewrite)) .collect(ImmutableList.toImmutableList()); } return colNames.stream().map(cn -> { - Column column = connectorSinkTargetColumn(table, cn); + Column column = targetSchema.getColumn(cn); if (column == null) { throw new AnalysisException(String.format("column %s is not found in table %s", cn, table.getName())); @@ -931,13 +968,7 @@ static List selectConnectorSinkBindColumns(PluginDrivenExternalTable tab throw new AnalysisException(String.format( "column %s is a static partition column, should not be in the insert column list", cn)); } - // Reject explicitly naming an engine-managed invisible column (e.g. iceberg v3 row-lineage - // _row_id / _last_updated_sequence_number) in an ordinary INSERT: the user never supplies - // its value. RETAINED for a rewrite (rewrite_data_files reads/rewrites full rows, preserving - // the engine-managed values), mirroring the isVisible/isRewrite split of the empty-colNames - // branch above. Uses only Column.isVisible(), so no source-specific code enters the generic - // SPI path (replaces the retired legacy source-specific iceberg row-lineage guard). - if (!isRewrite && !column.isVisible()) { + if (!isConnectorSinkWriteColumn(column, isRewrite)) { throw new AnalysisException(String.format( "Cannot specify invisible column '%s' in INSERT statement", cn)); } @@ -945,6 +976,33 @@ static List selectConnectorSinkBindColumns(PluginDrivenExternalTable tab }).collect(ImmutableList.toImmutableList()); } + private static boolean isConnectorSinkWriteColumn(Column column, boolean isRewrite) { + // Hidden request-scoped scan columns are not sink fields; only connector-declared persistent + // passthrough columns may survive a rewrite and change its physical arity. + return column.isVisible() || (isRewrite && column.isReservedPassthrough()); + } + + @VisibleForTesting + static List selectConnectorRewriteOutputs( + List writeSchema, List sourceOutputs) { + Map outputByName = Maps.newTreeMap(String.CASE_INSENSITIVE_ORDER); + for (NamedExpression output : sourceOutputs) { + if (outputByName.put(output.getName(), output) != null) { + throw new AnalysisException("Duplicate column in connector rewrite source: " + output.getName()); + } + } + // Source scans can expose request-scoped hidden columns under show-hidden. Selecting by the physical + // write schema keeps source output, bound schema, and BE sink arity on one shared invariant. + return writeSchema.stream().map(column -> { + NamedExpression output = outputByName.get(column.getName()); + if (output == null) { + throw new AnalysisException("Column " + column.getName() + + " is missing from connector rewrite source"); + } + return output; + }).collect(ImmutableList.toImmutableList()); + } + /** * Build column-to-output mapping for connector table sinks. * Maps each user-specified column to the corresponding child output expression diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalConnectorTableSinkToPhysicalConnectorTableSink.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalConnectorTableSinkToPhysicalConnectorTableSink.java index d58ae8f40963fb..ff32bddfa139a6 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalConnectorTableSinkToPhysicalConnectorTableSink.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalConnectorTableSinkToPhysicalConnectorTableSink.java @@ -36,6 +36,9 @@ public Rule build() { return new PhysicalConnectorTableSink<>( sink.getDatabase(), sink.getTargetTable(), + sink.getBoundTargetSchema(), + sink.getBoundPartitionColumns(), + sink.getBoundWriteMetadataIdentity(), sink.getCols(), sink.getOutputExprs(), Optional.empty(), diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalExternalRowLevelDeleteSinkToPhysicalExternalRowLevelDeleteSink.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalExternalRowLevelDeleteSinkToPhysicalExternalRowLevelDeleteSink.java index eb5e662adc153e..69215b683d0a67 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalExternalRowLevelDeleteSinkToPhysicalExternalRowLevelDeleteSink.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalExternalRowLevelDeleteSinkToPhysicalExternalRowLevelDeleteSink.java @@ -37,6 +37,7 @@ public Rule build() { return new PhysicalExternalRowLevelDeleteSink<>( sink.getDatabase(), sink.getTargetTable(), + sink.getBoundWriteMetadataIdentity(), sink.getCols(), sink.getOutputExprs(), Optional.empty(), diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalExternalRowLevelMergeSinkToPhysicalExternalRowLevelMergeSink.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalExternalRowLevelMergeSinkToPhysicalExternalRowLevelMergeSink.java index c00d9e7fd619a6..4d84d6ebda60e1 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalExternalRowLevelMergeSinkToPhysicalExternalRowLevelMergeSink.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalExternalRowLevelMergeSinkToPhysicalExternalRowLevelMergeSink.java @@ -37,6 +37,7 @@ public Rule build() { return new PhysicalExternalRowLevelMergeSink<>( sink.getDatabase(), sink.getTargetTable(), + sink.getBoundWriteMetadataIdentity(), sink.getCols(), sink.getOutputExprs(), sink.isRequireMergeCardinalityCheck(), diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/CreateNamedStruct.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/CreateNamedStruct.java index b0ab4725908f5d..c27b5cc94c9a54 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/CreateNamedStruct.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/CreateNamedStruct.java @@ -25,6 +25,7 @@ import org.apache.doris.nereids.trees.expressions.functions.CustomSignature; import org.apache.doris.nereids.trees.expressions.functions.ExpressionTrait; import org.apache.doris.nereids.trees.expressions.literal.StringLikeLiteral; +import org.apache.doris.nereids.trees.expressions.literal.StructLiteral; import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; import org.apache.doris.nereids.types.DataType; import org.apache.doris.nereids.types.StructField; @@ -108,7 +109,8 @@ public FunctionSignature customSignature() { // A named struct has the same value-nullability contract as struct(...); keeping // the field nullable here would reject safe casts into required target fields. structFields.add(new StructField(nameLiteral.getStringValue(), - children.get(i + 1).getDataType(), children.get(i + 1).nullable(), "")); + children.get(i + 1).getDataType(), + StructLiteral.computeFieldNullable(children.get(i + 1)), "")); } return FunctionSignature.ret(new StructType(structFields.build())) .args(children.stream().map(ExpressionTrait::getDataType).toArray(DataType[]::new)); diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/StructLiteral.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/StructLiteral.java index 3104e490fb3414..53d1044cb7efc7 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/StructLiteral.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/StructLiteral.java @@ -158,9 +158,16 @@ public static StructType computeDataType(List fields) { ImmutableList.Builder structFields = ImmutableList.builder(); for (int i = 0; i < fields.size(); i++) { Expression field = fields.get(i); - // Preserve literal nullability so a known non-null value can satisfy a required nested field. - structFields.add(new StructField(COL_PREFIX + (i + 1), field.getDataType(), field.nullable(), "")); + structFields.add(new StructField(COL_PREFIX + (i + 1), field.getDataType(), + computeFieldNullable(field), "")); } return new StructType(structFields.build()); } + + /** Infer field nullability for struct constructors. */ + public static boolean computeFieldNullable(Expression field) { + // Strict cast changes failure behavior, not the physical result column. Preserve the cast's + // nullable type so FunctionStruct never inserts ColumnNullable into a required child column. + return field.nullable(); + } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ExternalRowLevelDeletePlanBuilder.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ExternalRowLevelDeletePlanBuilder.java index b2e01752dd8159..77320cd5ca7e64 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ExternalRowLevelDeletePlanBuilder.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ExternalRowLevelDeletePlanBuilder.java @@ -20,6 +20,7 @@ import org.apache.doris.catalog.Column; import org.apache.doris.datasource.ExternalDatabase; import org.apache.doris.datasource.ExternalTable; +import org.apache.doris.datasource.plugin.PluginDrivenExternalTable; import org.apache.doris.nereids.analyzer.UnboundAlias; import org.apache.doris.nereids.analyzer.UnboundSlot; import org.apache.doris.nereids.trees.expressions.NamedExpression; @@ -85,6 +86,10 @@ public ExternalRowLevelDeletePlanBuilder( // package-visible: the generic RowLevelDmlCommand shell delegates synthesis here. LogicalPlan completeQueryPlan(ConnectContext ctx, LogicalPlan logicalQuery, ExternalTable icebergTable) { + // The row shape and conflict fence must come from one cache generation; otherwise a replacement + // between planning and beginWrite could become the new baseline instead of aborting the stale DELETE. + PluginDrivenExternalTable.WriteSchemaSnapshot writeSchema = + ((PluginDrivenExternalTable) icebergTable).getWriteSchemaSnapshot(); LogicalPlan queryPlan = buildPositionDeletePlan(ctx, logicalQuery, icebergTable); // Convert output to NamedExpression list @@ -103,7 +108,8 @@ LogicalPlan completeQueryPlan(ConnectContext ctx, LogicalPlan logicalQuery, LogicalExternalRowLevelDeleteSink deleteSink = new LogicalExternalRowLevelDeleteSink<>( (ExternalDatabase) icebergTable.getDatabase(), icebergTable, - icebergTable.getBaseSchema(true), // cols + writeSchema.getWriteMetadataIdentity(), + writeSchema.getBaseSchema(), // cols outputExprs, // outputExprs Optional.empty(), // groupExpression Optional.empty(), // logicalProperties diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ExternalRowLevelMergePlanBuilder.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ExternalRowLevelMergePlanBuilder.java index 129e4f41c7066b..5a0ed430c2e209 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ExternalRowLevelMergePlanBuilder.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ExternalRowLevelMergePlanBuilder.java @@ -22,6 +22,7 @@ import org.apache.doris.common.util.Util; import org.apache.doris.datasource.ExternalDatabase; import org.apache.doris.datasource.ExternalTable; +import org.apache.doris.datasource.plugin.PluginDrivenExternalTable; import org.apache.doris.nereids.analyzer.UnboundAlias; import org.apache.doris.nereids.analyzer.UnboundRelation; import org.apache.doris.nereids.analyzer.UnboundSlot; @@ -316,9 +317,8 @@ private List generateFinalProjections(List colNames, return output; } - private LogicalPlan buildMergeProjectPlan(ConnectContext ctx, ExternalTable icebergTable) { - List columns = icebergTable.getBaseSchema(true); - + private LogicalPlan buildMergeProjectPlan(ConnectContext ctx, ExternalTable icebergTable, + List columns) { LogicalPlan plan = generateBasePlan(); plan = injectRowIdColumn(plan, icebergTable); @@ -380,7 +380,12 @@ private LogicalPlan buildMergeProjectPlan(ConnectContext ctx, ExternalTable iceb // package-visible: the generic RowLevelDmlCommand shell delegates synthesis here. LogicalPlan buildMergePlan(ConnectContext ctx, ExternalTable icebergTable) { - LogicalPlan projectPlan = buildMergeProjectPlan(ctx, icebergTable); + // Branch projections and sink metadata must share one schema read; a concurrent reorder between + // two reads would pair expressions from S0 with columns from S1 and silently swap written values. + PluginDrivenExternalTable.WriteSchemaSnapshot writeSchema = + ((PluginDrivenExternalTable) icebergTable).getWriteSchemaSnapshot(); + List columns = writeSchema.getBaseSchema(); + LogicalPlan projectPlan = buildMergeProjectPlan(ctx, icebergTable, columns); List outputExprs; if (!RowLevelDmlRowIdUtils.hasUnboundPlan(projectPlan)) { @@ -396,7 +401,8 @@ LogicalPlan buildMergePlan(ConnectContext ctx, ExternalTable icebergTable) { return new LogicalExternalRowLevelMergeSink<>( (ExternalDatabase) icebergTable.getDatabase(), icebergTable, - icebergTable.getBaseSchema(true), + writeSchema.getWriteMetadataIdentity(), + columns, outputExprs, true, Optional.empty(), diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ExternalRowLevelUpdatePlanBuilder.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ExternalRowLevelUpdatePlanBuilder.java index c483cde24fafb8..e751a4df8da68b 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ExternalRowLevelUpdatePlanBuilder.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ExternalRowLevelUpdatePlanBuilder.java @@ -21,6 +21,7 @@ import org.apache.doris.common.util.Util; import org.apache.doris.datasource.ExternalDatabase; import org.apache.doris.datasource.ExternalTable; +import org.apache.doris.datasource.plugin.PluginDrivenExternalTable; import org.apache.doris.nereids.analyzer.UnboundAlias; import org.apache.doris.nereids.analyzer.UnboundSlot; import org.apache.doris.nereids.exceptions.AnalysisException; @@ -111,8 +112,13 @@ LogicalPlan buildMergePlan(ConnectContext ctx, LogicalPlan logicalQuery, String tableName = tableAlias != null ? tableAlias : Util.getTempTableDisplayName(icebergTable.getName()); + // Projection and sink metadata must share one schema read; otherwise concurrent evolution can + // associate expressions from one generation with sink columns from the next. + PluginDrivenExternalTable.WriteSchemaSnapshot writeSchema = + ((PluginDrivenExternalTable) icebergTable).getWriteSchemaSnapshot(); + List columns = writeSchema.getBaseSchema(); LogicalPlan queryPlan = buildMergeProjectPlan(ctx, logicalQuery, assignments, - icebergTable.getBaseSchema(true), tableName); + columns, tableName); List outputExprs; if (!RowLevelDmlRowIdUtils.hasUnboundPlan(queryPlan)) { @@ -128,7 +134,8 @@ LogicalPlan buildMergePlan(ConnectContext ctx, LogicalPlan logicalQuery, return new LogicalExternalRowLevelMergeSink<>( (ExternalDatabase) icebergTable.getDatabase(), icebergTable, - icebergTable.getBaseSchema(true), + writeSchema.getWriteMetadataIdentity(), + columns, outputExprs, false, Optional.empty(), diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalConnectorTableSink.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalConnectorTableSink.java index d1ec0d63041bb6..396d6466beba4b 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalConnectorTableSink.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalConnectorTableSink.java @@ -47,6 +47,11 @@ public class LogicalConnectorTableSink extends LogicalT // bound data sink private final ExternalDatabase database; private final ExternalTable targetTable; + // Preserve the single schema generation captured by BindSink; later phases must not re-read a + // newer schema and accidentally move the baseline used for ordinal write validation. + private final List boundTargetSchema; + private final List boundPartitionColumns; + private final String boundWriteMetadataIdentity; private final DMLCommandType dmlCommandType; // Rewrite (compaction) marker, carried from UnboundConnectorTableSink.isRewrite so the physical sink // can force single-node GATHER output for a rewrite_data_files INSERT-SELECT. Part of plan identity @@ -58,6 +63,42 @@ public class LogicalConnectorTableSink extends LogicalT */ public LogicalConnectorTableSink(ExternalDatabase database, ExternalTable targetTable, + List boundTargetSchema, + List cols, + List outputExprs, + DMLCommandType dmlCommandType, + boolean rewrite, + Optional groupExpression, + Optional logicalProperties, + CHILD_TYPE child) { + this(database, targetTable, boundTargetSchema, ImmutableList.of(), cols, outputExprs, + dmlCommandType, rewrite, groupExpression, logicalProperties, child); + } + + /** + * Builds a connector sink with target and partition columns captured from one schema generation. + */ + public LogicalConnectorTableSink(ExternalDatabase database, + ExternalTable targetTable, + List boundTargetSchema, + List boundPartitionColumns, + List cols, + List outputExprs, + DMLCommandType dmlCommandType, + boolean rewrite, + Optional groupExpression, + Optional logicalProperties, + CHILD_TYPE child) { + this(database, targetTable, boundTargetSchema, boundPartitionColumns, null, cols, outputExprs, + dmlCommandType, rewrite, groupExpression, logicalProperties, child); + } + + /** Builds a connector sink with its opaque write generation captured by the same schema load. */ + public LogicalConnectorTableSink(ExternalDatabase database, + ExternalTable targetTable, + List boundTargetSchema, + List boundPartitionColumns, + String boundWriteMetadataIdentity, List cols, List outputExprs, DMLCommandType dmlCommandType, @@ -68,6 +109,9 @@ public LogicalConnectorTableSink(ExternalDatabase database, super(PlanType.LOGICAL_CONNECTOR_TABLE_SINK, outputExprs, groupExpression, logicalProperties, cols, child); this.database = Objects.requireNonNull(database, "database != null in LogicalConnectorTableSink"); this.targetTable = Objects.requireNonNull(targetTable, "targetTable != null in LogicalConnectorTableSink"); + this.boundTargetSchema = ImmutableList.copyOf(boundTargetSchema); + this.boundPartitionColumns = ImmutableList.copyOf(boundPartitionColumns); + this.boundWriteMetadataIdentity = boundWriteMetadataIdentity; this.dmlCommandType = dmlCommandType; this.rewrite = rewrite; } @@ -78,7 +122,8 @@ public Plan withChildAndUpdateOutput(Plan child) { .map(NamedExpression.class::cast) .collect(ImmutableList.toImmutableList()); return AbstractPlan.copyWithSameId(this, () -> - new LogicalConnectorTableSink<>(database, targetTable, cols, output, + new LogicalConnectorTableSink<>(database, targetTable, boundTargetSchema, boundPartitionColumns, + boundWriteMetadataIdentity, cols, output, dmlCommandType, rewrite, Optional.empty(), Optional.empty(), child)); } @@ -86,13 +131,15 @@ public Plan withChildAndUpdateOutput(Plan child) { public Plan withChildren(List children) { Preconditions.checkArgument(children.size() == 1, "LogicalConnectorTableSink only accepts one child"); return AbstractPlan.copyWithSameId(this, () -> - new LogicalConnectorTableSink<>(database, targetTable, cols, outputExprs, + new LogicalConnectorTableSink<>(database, targetTable, boundTargetSchema, boundPartitionColumns, + boundWriteMetadataIdentity, cols, outputExprs, dmlCommandType, rewrite, Optional.empty(), Optional.empty(), children.get(0))); } public LogicalConnectorTableSink withOutputExprs(List outputExprs) { return AbstractPlan.copyWithSameId(this, () -> - new LogicalConnectorTableSink<>(database, targetTable, cols, outputExprs, + new LogicalConnectorTableSink<>(database, targetTable, boundTargetSchema, boundPartitionColumns, + boundWriteMetadataIdentity, cols, outputExprs, dmlCommandType, rewrite, Optional.empty(), Optional.empty(), child())); } @@ -104,6 +151,18 @@ public ExternalTable getTargetTable() { return targetTable; } + public List getBoundTargetSchema() { + return boundTargetSchema; + } + + public List getBoundPartitionColumns() { + return boundPartitionColumns; + } + + public String getBoundWriteMetadataIdentity() { + return boundWriteMetadataIdentity; + } + public DMLCommandType getDmlCommandType() { return dmlCommandType; } @@ -127,12 +186,17 @@ public boolean equals(Object o) { return dmlCommandType == that.dmlCommandType && rewrite == that.rewrite && Objects.equals(database, that.database) - && Objects.equals(targetTable, that.targetTable) && Objects.equals(cols, that.cols); + && Objects.equals(targetTable, that.targetTable) + && Objects.equals(boundTargetSchema, that.boundTargetSchema) + && Objects.equals(boundPartitionColumns, that.boundPartitionColumns) + && Objects.equals(boundWriteMetadataIdentity, that.boundWriteMetadataIdentity) + && Objects.equals(cols, that.cols); } @Override public int hashCode() { - return Objects.hash(super.hashCode(), database, targetTable, cols, dmlCommandType, rewrite); + return Objects.hash(super.hashCode(), database, targetTable, boundTargetSchema, boundPartitionColumns, + boundWriteMetadataIdentity, cols, dmlCommandType, rewrite); } @Override @@ -141,6 +205,8 @@ public String toString() { "outputExprs", outputExprs, "database", database.getFullName(), "targetTable", targetTable.getName(), + "boundTargetSchema", boundTargetSchema, + "boundPartitionColumns", boundPartitionColumns, "cols", cols, "dmlCommandType", dmlCommandType, "rewrite", rewrite @@ -155,7 +221,8 @@ public R accept(PlanVisitor visitor, C context) { @Override public Plan withGroupExpression(Optional groupExpression) { return AbstractPlan.copyWithSameId(this, () -> - new LogicalConnectorTableSink<>(database, targetTable, cols, outputExprs, + new LogicalConnectorTableSink<>(database, targetTable, boundTargetSchema, boundPartitionColumns, + boundWriteMetadataIdentity, cols, outputExprs, dmlCommandType, rewrite, groupExpression, Optional.of(getLogicalProperties()), child())); } @@ -163,7 +230,8 @@ public Plan withGroupExpression(Optional groupExpression) { public Plan withGroupExprLogicalPropChildren(Optional groupExpression, Optional logicalProperties, List children) { return AbstractPlan.copyWithSameId(this, () -> - new LogicalConnectorTableSink<>(database, targetTable, cols, outputExprs, + new LogicalConnectorTableSink<>(database, targetTable, boundTargetSchema, boundPartitionColumns, + boundWriteMetadataIdentity, cols, outputExprs, dmlCommandType, rewrite, groupExpression, logicalProperties, children.get(0))); } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalExternalRowLevelDeleteSink.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalExternalRowLevelDeleteSink.java index 2b3f28c85abbcf..18ca37bb9b6dc4 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalExternalRowLevelDeleteSink.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalExternalRowLevelDeleteSink.java @@ -45,6 +45,7 @@ public class LogicalExternalRowLevelDeleteSink extends implements Sink, PropagateFuncDeps { private final ExternalDatabase database; private final ExternalTable targetTable; + private final String boundWriteMetadataIdentity; /** * Constructor. @@ -61,31 +62,48 @@ public LogicalExternalRowLevelDeleteSink(ExternalDatabase database, Optional groupExpression, Optional logicalProperties, CHILD_TYPE child) { + this(database, targetTable, null, cols, outputExprs, groupExpression, logicalProperties, child); + } + + /** Builds a row-level sink bound to the same remote generation as its target columns. */ + public LogicalExternalRowLevelDeleteSink(ExternalDatabase database, + ExternalTable targetTable, + String boundWriteMetadataIdentity, + List cols, + List outputExprs, + Optional groupExpression, + Optional logicalProperties, + CHILD_TYPE child) { super(PlanType.LOGICAL_EXTERNAL_ROW_LEVEL_DELETE_SINK, outputExprs, groupExpression, logicalProperties, cols, child); this.database = Objects.requireNonNull(database, "database != null in LogicalExternalRowLevelDeleteSink"); this.targetTable = Objects.requireNonNull(targetTable, "targetTable != null in LogicalExternalRowLevelDeleteSink"); + this.boundWriteMetadataIdentity = boundWriteMetadataIdentity; } + /** Rebuilds the sink after its child has been rewritten and adopts the child's bound output. */ public Plan withChildAndUpdateOutput(Plan child) { List output = child.getOutput().stream() .map(NamedExpression.class::cast) .collect(ImmutableList.toImmutableList()); - return new LogicalExternalRowLevelDeleteSink<>(database, targetTable, cols, output, + return new LogicalExternalRowLevelDeleteSink<>(database, targetTable, boundWriteMetadataIdentity, + cols, output, Optional.empty(), Optional.empty(), child); } @Override public Plan withChildren(List children) { Preconditions.checkArgument(children.size() == 1, "LogicalExternalRowLevelDeleteSink only accepts one child"); - return new LogicalExternalRowLevelDeleteSink<>(database, targetTable, cols, outputExprs, + return new LogicalExternalRowLevelDeleteSink<>(database, targetTable, boundWriteMetadataIdentity, + cols, outputExprs, Optional.empty(), Optional.empty(), children.get(0)); } public LogicalExternalRowLevelDeleteSink withOutputExprs(List outputExprs) { - return new LogicalExternalRowLevelDeleteSink<>(database, targetTable, cols, outputExprs, + return new LogicalExternalRowLevelDeleteSink<>(database, targetTable, boundWriteMetadataIdentity, + cols, outputExprs, Optional.empty(), Optional.empty(), child()); } @@ -97,6 +115,10 @@ public ExternalTable getTargetTable() { return targetTable; } + public String getBoundWriteMetadataIdentity() { + return boundWriteMetadataIdentity; + } + @Override public boolean equals(Object o) { if (this == o) { @@ -111,12 +133,13 @@ public boolean equals(Object o) { LogicalExternalRowLevelDeleteSink that = (LogicalExternalRowLevelDeleteSink) o; return Objects.equals(database, that.database) && Objects.equals(targetTable, that.targetTable) + && Objects.equals(boundWriteMetadataIdentity, that.boundWriteMetadataIdentity) && Objects.equals(cols, that.cols); } @Override public int hashCode() { - return Objects.hash(super.hashCode(), database, targetTable, cols); + return Objects.hash(super.hashCode(), database, targetTable, boundWriteMetadataIdentity, cols); } @Override @@ -136,14 +159,16 @@ public R accept(PlanVisitor visitor, C context) { @Override public Plan withGroupExpression(Optional groupExpression) { - return new LogicalExternalRowLevelDeleteSink<>(database, targetTable, cols, outputExprs, + return new LogicalExternalRowLevelDeleteSink<>(database, targetTable, boundWriteMetadataIdentity, + cols, outputExprs, groupExpression, Optional.of(getLogicalProperties()), child()); } @Override public Plan withGroupExprLogicalPropChildren(Optional groupExpression, Optional logicalProperties, List children) { - return new LogicalExternalRowLevelDeleteSink<>(database, targetTable, cols, outputExprs, + return new LogicalExternalRowLevelDeleteSink<>(database, targetTable, boundWriteMetadataIdentity, + cols, outputExprs, groupExpression, logicalProperties, children.get(0)); } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalExternalRowLevelMergeSink.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalExternalRowLevelMergeSink.java index d2e55779ef5b48..768a284c217a86 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalExternalRowLevelMergeSink.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalExternalRowLevelMergeSink.java @@ -45,6 +45,7 @@ public class LogicalExternalRowLevelMergeSink extends L implements Sink, PropagateFuncDeps { private final ExternalDatabase database; private final ExternalTable targetTable; + private final String boundWriteMetadataIdentity; // True for SQL MERGE INTO, false for UPDATE. MERGE must reject a target row matched by more than one // source row (SQL cardinality rule), which the BE sink can only do when the plan keeps the merge // distribution; UPDATE has no such rule. Read by RequestPropertyDeriver (which otherwise drops the @@ -67,12 +68,27 @@ public LogicalExternalRowLevelMergeSink(ExternalDatabase database, Optional groupExpression, Optional logicalProperties, CHILD_TYPE child) { + this(database, targetTable, null, cols, outputExprs, requireMergeCardinalityCheck, + groupExpression, logicalProperties, child); + } + + /** Builds a row-level sink bound to the same remote generation as its target columns. */ + public LogicalExternalRowLevelMergeSink(ExternalDatabase database, + ExternalTable targetTable, + String boundWriteMetadataIdentity, + List cols, + List outputExprs, + boolean requireMergeCardinalityCheck, + Optional groupExpression, + Optional logicalProperties, + CHILD_TYPE child) { super(PlanType.LOGICAL_EXTERNAL_ROW_LEVEL_MERGE_SINK, outputExprs, groupExpression, logicalProperties, cols, child); this.database = Objects.requireNonNull(database, "database != null in LogicalExternalRowLevelMergeSink"); this.targetTable = Objects.requireNonNull(targetTable, "targetTable != null in LogicalExternalRowLevelMergeSink"); + this.boundWriteMetadataIdentity = boundWriteMetadataIdentity; this.requireMergeCardinalityCheck = requireMergeCardinalityCheck; } @@ -80,19 +96,21 @@ public Plan withChildAndUpdateOutput(Plan child) { List output = child.getOutput().stream() .map(NamedExpression.class::cast) .collect(ImmutableList.toImmutableList()); - return new LogicalExternalRowLevelMergeSink<>(database, targetTable, cols, output, + return new LogicalExternalRowLevelMergeSink<>(database, targetTable, boundWriteMetadataIdentity, cols, output, requireMergeCardinalityCheck, Optional.empty(), Optional.empty(), child); } @Override public Plan withChildren(List children) { Preconditions.checkArgument(children.size() == 1, "LogicalExternalRowLevelMergeSink only accepts one child"); - return new LogicalExternalRowLevelMergeSink<>(database, targetTable, cols, outputExprs, + return new LogicalExternalRowLevelMergeSink<>(database, targetTable, boundWriteMetadataIdentity, + cols, outputExprs, requireMergeCardinalityCheck, Optional.empty(), Optional.empty(), children.get(0)); } public LogicalExternalRowLevelMergeSink withOutputExprs(List outputExprs) { - return new LogicalExternalRowLevelMergeSink<>(database, targetTable, cols, outputExprs, + return new LogicalExternalRowLevelMergeSink<>(database, targetTable, boundWriteMetadataIdentity, + cols, outputExprs, requireMergeCardinalityCheck, Optional.empty(), Optional.empty(), child()); } @@ -104,6 +122,10 @@ public ExternalTable getTargetTable() { return targetTable; } + public String getBoundWriteMetadataIdentity() { + return boundWriteMetadataIdentity; + } + public boolean isRequireMergeCardinalityCheck() { return requireMergeCardinalityCheck; } @@ -122,13 +144,15 @@ public boolean equals(Object o) { LogicalExternalRowLevelMergeSink that = (LogicalExternalRowLevelMergeSink) o; return Objects.equals(database, that.database) && Objects.equals(targetTable, that.targetTable) + && Objects.equals(boundWriteMetadataIdentity, that.boundWriteMetadataIdentity) && Objects.equals(cols, that.cols) && requireMergeCardinalityCheck == that.requireMergeCardinalityCheck; } @Override public int hashCode() { - return Objects.hash(super.hashCode(), database, targetTable, cols, requireMergeCardinalityCheck); + return Objects.hash(super.hashCode(), database, targetTable, boundWriteMetadataIdentity, cols, + requireMergeCardinalityCheck); } @Override @@ -148,14 +172,16 @@ public R accept(PlanVisitor visitor, C context) { @Override public Plan withGroupExpression(Optional groupExpression) { - return new LogicalExternalRowLevelMergeSink<>(database, targetTable, cols, outputExprs, + return new LogicalExternalRowLevelMergeSink<>(database, targetTable, boundWriteMetadataIdentity, + cols, outputExprs, requireMergeCardinalityCheck, groupExpression, Optional.of(getLogicalProperties()), child()); } @Override public Plan withGroupExprLogicalPropChildren(Optional groupExpression, Optional logicalProperties, List children) { - return new LogicalExternalRowLevelMergeSink<>(database, targetTable, cols, outputExprs, + return new LogicalExternalRowLevelMergeSink<>(database, targetTable, boundWriteMetadataIdentity, + cols, outputExprs, requireMergeCardinalityCheck, groupExpression, logicalProperties, children.get(0)); } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalConnectorTableSink.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalConnectorTableSink.java index 277c6986274bf5..aac40e428b0c18 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalConnectorTableSink.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalConnectorTableSink.java @@ -35,8 +35,11 @@ import org.apache.doris.nereids.trees.plans.visitor.PlanVisitor; import org.apache.doris.statistics.Statistics; +import com.google.common.collect.ImmutableList; + import java.util.ArrayList; import java.util.List; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; @@ -46,6 +49,10 @@ */ public class PhysicalConnectorTableSink extends PhysicalBaseExternalTableSink { + private final List boundTargetSchema; + private final List boundPartitionColumns; + private final String boundWriteMetadataIdentity; + // Rewrite (compaction) marker, threaded from LogicalConnectorTableSink.isRewrite. When set, // getRequirePhysicalProperties() short-circuits to GATHER (single writer) so a rewrite_data_files // INSERT-SELECT controls its output file count even on a partitioned table — the override must win @@ -58,13 +65,45 @@ public class PhysicalConnectorTableSink extends Physica */ public PhysicalConnectorTableSink(ExternalDatabase database, ExternalTable targetTable, + List boundTargetSchema, + List cols, + List outputExprs, + Optional groupExpression, + LogicalProperties logicalProperties, + boolean isRewrite, + CHILD_TYPE child) { + this(database, targetTable, boundTargetSchema, ImmutableList.of(), cols, outputExprs, + groupExpression, logicalProperties, isRewrite, child); + } + + public PhysicalConnectorTableSink(ExternalDatabase database, + ExternalTable targetTable, + List boundTargetSchema, + List boundPartitionColumns, List cols, List outputExprs, Optional groupExpression, LogicalProperties logicalProperties, boolean isRewrite, CHILD_TYPE child) { - this(database, targetTable, cols, outputExprs, groupExpression, logicalProperties, + this(database, targetTable, boundTargetSchema, boundPartitionColumns, null, cols, outputExprs, + groupExpression, logicalProperties, isRewrite, child); + } + + /** Builds a physical sink with the write generation captured during sink binding. */ + public PhysicalConnectorTableSink(ExternalDatabase database, + ExternalTable targetTable, + List boundTargetSchema, + List boundPartitionColumns, + String boundWriteMetadataIdentity, + List cols, + List outputExprs, + Optional groupExpression, + LogicalProperties logicalProperties, + boolean isRewrite, + CHILD_TYPE child) { + this(database, targetTable, boundTargetSchema, boundPartitionColumns, boundWriteMetadataIdentity, + cols, outputExprs, groupExpression, logicalProperties, PhysicalProperties.GATHER, null, isRewrite, child); } @@ -73,6 +112,41 @@ public PhysicalConnectorTableSink(ExternalDatabase database, */ public PhysicalConnectorTableSink(ExternalDatabase database, ExternalTable targetTable, + List boundTargetSchema, + List cols, + List outputExprs, + Optional groupExpression, + LogicalProperties logicalProperties, + PhysicalProperties physicalProperties, + Statistics statistics, + boolean isRewrite, + CHILD_TYPE child) { + this(database, targetTable, boundTargetSchema, ImmutableList.of(), cols, outputExprs, + groupExpression, logicalProperties, physicalProperties, statistics, isRewrite, child); + } + + public PhysicalConnectorTableSink(ExternalDatabase database, + ExternalTable targetTable, + List boundTargetSchema, + List boundPartitionColumns, + List cols, + List outputExprs, + Optional groupExpression, + LogicalProperties logicalProperties, + PhysicalProperties physicalProperties, + Statistics statistics, + boolean isRewrite, + CHILD_TYPE child) { + this(database, targetTable, boundTargetSchema, boundPartitionColumns, null, cols, outputExprs, + groupExpression, logicalProperties, physicalProperties, statistics, isRewrite, child); + } + + /** Builds a physical sink with the write generation captured during sink binding. */ + public PhysicalConnectorTableSink(ExternalDatabase database, + ExternalTable targetTable, + List boundTargetSchema, + List boundPartitionColumns, + String boundWriteMetadataIdentity, List cols, List outputExprs, Optional groupExpression, @@ -83,14 +157,19 @@ public PhysicalConnectorTableSink(ExternalDatabase database, CHILD_TYPE child) { super(PlanType.PHYSICAL_CONNECTOR_TABLE_SINK, database, targetTable, cols, outputExprs, groupExpression, logicalProperties, physicalProperties, statistics, child); + this.boundTargetSchema = ImmutableList.copyOf(boundTargetSchema); + this.boundPartitionColumns = ImmutableList.copyOf(boundPartitionColumns); + this.boundWriteMetadataIdentity = boundWriteMetadataIdentity; this.isRewrite = isRewrite; } @Override public Plan withChildren(List children) { return AbstractPlan.copyWithSameId(this, () -> new PhysicalConnectorTableSink<>( - (ExternalDatabase) database, (ExternalTable) targetTable, cols, outputExprs, groupExpression, - getLogicalProperties(), physicalProperties, statistics, isRewrite, children.get(0))); + (ExternalDatabase) database, (ExternalTable) targetTable, boundTargetSchema, + boundPartitionColumns, boundWriteMetadataIdentity, cols, + outputExprs, groupExpression, getLogicalProperties(), physicalProperties, statistics, + isRewrite, children.get(0))); } @Override @@ -101,23 +180,63 @@ public R accept(PlanVisitor visitor, C context) { @Override public Plan withGroupExpression(Optional groupExpression) { return AbstractPlan.copyWithSameId(this, () -> new PhysicalConnectorTableSink<>( - (ExternalDatabase) database, (ExternalTable) targetTable, cols, outputExprs, - groupExpression, getLogicalProperties(), isRewrite, child())); + (ExternalDatabase) database, (ExternalTable) targetTable, boundTargetSchema, boundPartitionColumns, + boundWriteMetadataIdentity, cols, + outputExprs, groupExpression, getLogicalProperties(), isRewrite, child())); } @Override public Plan withGroupExprLogicalPropChildren(Optional groupExpression, Optional logicalProperties, List children) { return AbstractPlan.copyWithSameId(this, () -> new PhysicalConnectorTableSink<>( - (ExternalDatabase) database, (ExternalTable) targetTable, cols, outputExprs, - groupExpression, logicalProperties.get(), isRewrite, children.get(0))); + (ExternalDatabase) database, (ExternalTable) targetTable, boundTargetSchema, boundPartitionColumns, + boundWriteMetadataIdentity, cols, + outputExprs, groupExpression, logicalProperties.get(), isRewrite, children.get(0))); } @Override public PhysicalPlan withPhysicalPropertiesAndStats(PhysicalProperties physicalProperties, Statistics statistics) { return AbstractPlan.copyWithSameId(this, () -> new PhysicalConnectorTableSink<>( - (ExternalDatabase) database, (ExternalTable) targetTable, cols, outputExprs, - groupExpression, getLogicalProperties(), physicalProperties, statistics, isRewrite, child())); + (ExternalDatabase) database, (ExternalTable) targetTable, boundTargetSchema, boundPartitionColumns, + boundWriteMetadataIdentity, cols, + outputExprs, groupExpression, getLogicalProperties(), physicalProperties, statistics, + isRewrite, child())); + } + + public List getBoundTargetSchema() { + return boundTargetSchema; + } + + public List getBoundPartitionColumns() { + return boundPartitionColumns; + } + + public String getBoundWriteMetadataIdentity() { + return boundWriteMetadataIdentity; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + if (!super.equals(o)) { + return false; + } + PhysicalConnectorTableSink that = (PhysicalConnectorTableSink) o; + return isRewrite == that.isRewrite + && Objects.equals(boundTargetSchema, that.boundTargetSchema) + && Objects.equals(boundPartitionColumns, that.boundPartitionColumns) + && Objects.equals(boundWriteMetadataIdentity, that.boundWriteMetadataIdentity); + } + + @Override + public int hashCode() { + return Objects.hash(super.hashCode(), boundTargetSchema, boundPartitionColumns, + boundWriteMetadataIdentity, isRewrite); } /** @@ -152,7 +271,7 @@ public boolean isRewrite() { * {@code BindSink.bindConnectorTableSink} projects the child to full-schema order (any * unmentioned / static-partition columns filled in), exactly like legacy {@code bindMaxComputeTableSink}, * because the BE writer strips the trailing partition columns by position. So {@code child().getOutput()} - * is aligned 1:1 with {@code targetTable.getFullSchema()}, while {@code cols} excludes the static + * is aligned 1:1 with {@code boundTargetSchema}, while {@code cols} excludes the static * partition columns and may be in a different (user-specified) order. Partition columns are therefore * located by their position in the full schema. (An earlier revision indexed by {@code cols}, which * mislocated the dynamic column whenever {@code cols} order diverged from the full schema — the @@ -172,7 +291,7 @@ public PhysicalProperties getRequirePhysicalProperties() { PluginDrivenExternalTable table = (PluginDrivenExternalTable) targetTable; if (table.requirePartitionLocalSortOnWrite()) { - Set partitionNames = table.getPartitionColumns().stream() + Set partitionNames = boundPartitionColumns.stream() .map(Column::getName) .collect(Collectors.toSet()); if (!partitionNames.isEmpty()) { @@ -192,7 +311,7 @@ public PhysicalProperties getRequirePhysicalProperties() { // by the correct (dynamic) column in the partial-static case. Mirrors legacy // PhysicalMaxComputeTableSink. List columnIdx = new ArrayList<>(); - List fullSchema = targetTable.getFullSchema(); + List fullSchema = boundTargetSchema; for (int i = 0; i < fullSchema.size(); i++) { if (partitionNames.contains(fullSchema.get(i).getName())) { columnIdx.add(i); @@ -219,7 +338,7 @@ public PhysicalProperties getRequirePhysicalProperties() { } if (table.requirePartitionHashOnWrite()) { - Set partitionNames = table.getPartitionColumns().stream() + Set partitionNames = boundPartitionColumns.stream() .map(Column::getName) .collect(Collectors.toSet()); if (!partitionNames.isEmpty()) { @@ -230,7 +349,7 @@ public PhysicalProperties getRequirePhysicalProperties() { // Index by full-schema position, which is aligned 1:1 with child output because a connector // declaring requiresPartitionHashWrite also declares requiresFullSchemaWriteOrder. List columnIdx = new ArrayList<>(); - List fullSchema = targetTable.getFullSchema(); + List fullSchema = boundTargetSchema; for (int i = 0; i < fullSchema.size(); i++) { if (partitionNames.contains(fullSchema.get(i).getName())) { columnIdx.add(i); diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalExternalRowLevelDeleteSink.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalExternalRowLevelDeleteSink.java index d3015501113fa7..45b8e93782d6d6 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalExternalRowLevelDeleteSink.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalExternalRowLevelDeleteSink.java @@ -37,6 +37,7 @@ import com.google.common.collect.ImmutableList; import java.util.List; +import java.util.Objects; import java.util.Optional; /** @@ -45,6 +46,7 @@ */ public class PhysicalExternalRowLevelDeleteSink extends PhysicalBaseExternalTableSink { + private final String boundWriteMetadataIdentity; /** * Constructor @@ -56,10 +58,23 @@ public PhysicalExternalRowLevelDeleteSink(ExternalDatabase database, Optional groupExpression, LogicalProperties logicalProperties, CHILD_TYPE child) { - this(database, targetTable, cols, outputExprs, groupExpression, logicalProperties, + this(database, targetTable, null, cols, outputExprs, groupExpression, logicalProperties, PhysicalProperties.GATHER, null, child); } + /** Builds a row-level sink with the write generation captured during logical planning. */ + public PhysicalExternalRowLevelDeleteSink(ExternalDatabase database, + ExternalTable targetTable, + String boundWriteMetadataIdentity, + List cols, + List outputExprs, + Optional groupExpression, + LogicalProperties logicalProperties, + CHILD_TYPE child) { + this(database, targetTable, boundWriteMetadataIdentity, cols, outputExprs, groupExpression, + logicalProperties, PhysicalProperties.GATHER, null, child); + } + /** * Constructor */ @@ -72,15 +87,35 @@ public PhysicalExternalRowLevelDeleteSink(ExternalDatabase database, PhysicalProperties physicalProperties, Statistics statistics, CHILD_TYPE child) { + this(database, targetTable, null, cols, outputExprs, groupExpression, logicalProperties, + physicalProperties, statistics, child); + } + + /** Builds a row-level sink with the write generation captured during logical planning. */ + public PhysicalExternalRowLevelDeleteSink(ExternalDatabase database, + ExternalTable targetTable, + String boundWriteMetadataIdentity, + List cols, + List outputExprs, + Optional groupExpression, + LogicalProperties logicalProperties, + PhysicalProperties physicalProperties, + Statistics statistics, + CHILD_TYPE child) { super(PlanType.PHYSICAL_EXTERNAL_ROW_LEVEL_DELETE_SINK, database, targetTable, cols, outputExprs, groupExpression, logicalProperties, physicalProperties, statistics, child); + this.boundWriteMetadataIdentity = boundWriteMetadataIdentity; + } + + public String getBoundWriteMetadataIdentity() { + return boundWriteMetadataIdentity; } @Override public Plan withChildren(List children) { return new PhysicalExternalRowLevelDeleteSink<>( database, targetTable, - cols, outputExprs, groupExpression, + boundWriteMetadataIdentity, cols, outputExprs, groupExpression, getLogicalProperties(), physicalProperties, statistics, children.get(0)); } @@ -92,7 +127,7 @@ public R accept(PlanVisitor visitor, C context) { @Override public Plan withGroupExpression(Optional groupExpression) { return new PhysicalExternalRowLevelDeleteSink<>( - database, targetTable, cols, outputExprs, + database, targetTable, boundWriteMetadataIdentity, cols, outputExprs, groupExpression, getLogicalProperties(), child()); } @@ -100,14 +135,14 @@ public Plan withGroupExpression(Optional groupExpression) { public Plan withGroupExprLogicalPropChildren(Optional groupExpression, Optional logicalProperties, List children) { return new PhysicalExternalRowLevelDeleteSink<>( - database, targetTable, cols, outputExprs, + database, targetTable, boundWriteMetadataIdentity, cols, outputExprs, groupExpression, logicalProperties.get(), children.get(0)); } @Override public PhysicalPlan withPhysicalPropertiesAndStats(PhysicalProperties physicalProperties, Statistics statistics) { return new PhysicalExternalRowLevelDeleteSink<>( - database, targetTable, cols, outputExprs, + database, targetTable, boundWriteMetadataIdentity, cols, outputExprs, groupExpression, getLogicalProperties(), physicalProperties, statistics, child()); } @@ -119,12 +154,16 @@ public boolean equals(Object o) { if (o == null || getClass() != o.getClass()) { return false; } - return super.equals(o); + if (!super.equals(o)) { + return false; + } + PhysicalExternalRowLevelDeleteSink that = (PhysicalExternalRowLevelDeleteSink) o; + return Objects.equals(boundWriteMetadataIdentity, that.boundWriteMetadataIdentity); } @Override public int hashCode() { - return super.hashCode(); + return Objects.hash(super.hashCode(), boundWriteMetadataIdentity); } /** diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalExternalRowLevelMergeSink.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalExternalRowLevelMergeSink.java index 2809adcde7232b..8e29b25e4c5775 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalExternalRowLevelMergeSink.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalExternalRowLevelMergeSink.java @@ -47,11 +47,13 @@ import com.google.common.collect.ImmutableList; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.TreeMap; +import java.util.stream.Collectors; /** * Physical Iceberg Merge Sink for UPDATE operations. @@ -59,6 +61,7 @@ */ public class PhysicalExternalRowLevelMergeSink extends PhysicalBaseExternalTableSink { + private final String boundWriteMetadataIdentity; // True for SQL MERGE INTO, false for UPDATE; see LogicalExternalRowLevelMergeSink. private final boolean requireMergeCardinalityCheck; @@ -73,10 +76,25 @@ public PhysicalExternalRowLevelMergeSink(ExternalDatabase database, Optional groupExpression, LogicalProperties logicalProperties, CHILD_TYPE child) { - this(database, targetTable, cols, outputExprs, requireMergeCardinalityCheck, + this(database, targetTable, null, cols, outputExprs, requireMergeCardinalityCheck, groupExpression, logicalProperties, PhysicalProperties.GATHER, null, child); } + /** Builds a row-level sink with the write generation captured during logical planning. */ + public PhysicalExternalRowLevelMergeSink(ExternalDatabase database, + ExternalTable targetTable, + String boundWriteMetadataIdentity, + List cols, + List outputExprs, + boolean requireMergeCardinalityCheck, + Optional groupExpression, + LogicalProperties logicalProperties, + CHILD_TYPE child) { + this(database, targetTable, boundWriteMetadataIdentity, cols, outputExprs, + requireMergeCardinalityCheck, groupExpression, logicalProperties, + PhysicalProperties.GATHER, null, child); + } + /** * Constructor */ @@ -90,11 +108,32 @@ public PhysicalExternalRowLevelMergeSink(ExternalDatabase database, PhysicalProperties physicalProperties, Statistics statistics, CHILD_TYPE child) { + this(database, targetTable, null, cols, outputExprs, requireMergeCardinalityCheck, + groupExpression, logicalProperties, physicalProperties, statistics, child); + } + + /** Builds a row-level sink with the write generation captured during logical planning. */ + public PhysicalExternalRowLevelMergeSink(ExternalDatabase database, + ExternalTable targetTable, + String boundWriteMetadataIdentity, + List cols, + List outputExprs, + boolean requireMergeCardinalityCheck, + Optional groupExpression, + LogicalProperties logicalProperties, + PhysicalProperties physicalProperties, + Statistics statistics, + CHILD_TYPE child) { super(PlanType.PHYSICAL_EXTERNAL_ROW_LEVEL_MERGE_SINK, database, targetTable, cols, outputExprs, groupExpression, logicalProperties, physicalProperties, statistics, child); + this.boundWriteMetadataIdentity = boundWriteMetadataIdentity; this.requireMergeCardinalityCheck = requireMergeCardinalityCheck; } + public String getBoundWriteMetadataIdentity() { + return boundWriteMetadataIdentity; + } + public boolean isRequireMergeCardinalityCheck() { return requireMergeCardinalityCheck; } @@ -103,7 +142,7 @@ public boolean isRequireMergeCardinalityCheck() { public Plan withChildren(List children) { return new PhysicalExternalRowLevelMergeSink<>( database, targetTable, - cols, outputExprs, requireMergeCardinalityCheck, groupExpression, + boundWriteMetadataIdentity, cols, outputExprs, requireMergeCardinalityCheck, groupExpression, getLogicalProperties(), physicalProperties, statistics, children.get(0)); } @@ -115,7 +154,7 @@ public R accept(PlanVisitor visitor, C context) { @Override public Plan withGroupExpression(Optional groupExpression) { return new PhysicalExternalRowLevelMergeSink<>( - database, targetTable, cols, outputExprs, + database, targetTable, boundWriteMetadataIdentity, cols, outputExprs, requireMergeCardinalityCheck, groupExpression, getLogicalProperties(), child()); } @@ -123,14 +162,14 @@ public Plan withGroupExpression(Optional groupExpression) { public Plan withGroupExprLogicalPropChildren(Optional groupExpression, Optional logicalProperties, List children) { return new PhysicalExternalRowLevelMergeSink<>( - database, targetTable, cols, outputExprs, + database, targetTable, boundWriteMetadataIdentity, cols, outputExprs, requireMergeCardinalityCheck, groupExpression, logicalProperties.get(), children.get(0)); } @Override public PhysicalPlan withPhysicalPropertiesAndStats(PhysicalProperties physicalProperties, Statistics statistics) { return new PhysicalExternalRowLevelMergeSink<>( - database, targetTable, cols, outputExprs, requireMergeCardinalityCheck, + database, targetTable, boundWriteMetadataIdentity, cols, outputExprs, requireMergeCardinalityCheck, groupExpression, getLogicalProperties(), physicalProperties, statistics, child()); } @@ -146,12 +185,13 @@ public boolean equals(Object o) { return false; } PhysicalExternalRowLevelMergeSink that = (PhysicalExternalRowLevelMergeSink) o; - return requireMergeCardinalityCheck == that.requireMergeCardinalityCheck; + return requireMergeCardinalityCheck == that.requireMergeCardinalityCheck + && Objects.equals(boundWriteMetadataIdentity, that.boundWriteMetadataIdentity); } @Override public int hashCode() { - return Objects.hash(super.hashCode(), requireMergeCardinalityCheck); + return Objects.hash(super.hashCode(), boundWriteMetadataIdentity, requireMergeCardinalityCheck); } /** @@ -197,18 +237,20 @@ public PhysicalProperties getRequirePhysicalProperties() { List insertPartitionExprIds = new ArrayList<>(); List insertPartitionFields = new ArrayList<>(); Integer partitionSpecId = null; - List partitionColumns = targetTable.getPartitionColumns(Optional.empty()); Map columnExprIdMap = buildColumnExprIdMap(outputSlots, nameToExprId); + Map columnIdToExprId = buildColumnIdExprIdMap(outputSlots); boolean insertExprsOk = false; - if (!partitionColumns.isEmpty()) { - insertExprsOk = buildInsertPartitionExprIds(insertPartitionExprIds, partitionColumns, columnExprIdMap); - } InsertPartitionFieldResult fieldResult = getIcebergPartitioning( - insertPartitionFields, targetTable, columnExprIdMap); + insertPartitionFields, targetTable, columnExprIdMap, columnIdToExprId); boolean insertFieldsOk = fieldResult.success; boolean hasNonIdentity = fieldResult.hasNonIdentity; if (insertFieldsOk) { partitionSpecId = fieldResult.partitionSpecId; + insertPartitionFields.stream() + .filter(field -> "identity".equals(field.getTransform())) + .map(DistributionSpecMerge.MergePartitionField::getSourceExprId) + .forEach(insertPartitionExprIds::add); + insertExprsOk = !insertPartitionExprIds.isEmpty(); } boolean insertRandom = !(insertExprsOk || insertFieldsOk); @@ -230,20 +272,6 @@ public PhysicalProperties getRequirePhysicalProperties() { partitionSpecId)); } - private boolean buildInsertPartitionExprIds(List insertPartitionExprIds, - List partitionColumns, - Map columnExprIdMap) { - for (Column column : partitionColumns) { - ExprId exprId = columnExprIdMap.get(column.getName()); - if (exprId == null) { - insertPartitionExprIds.clear(); - return false; - } - insertPartitionExprIds.add(exprId); - } - return insertPartitionExprIds.size() == partitionColumns.size(); - } - private Map buildColumnExprIdMap(List outputSlots, Map nameToExprId) { List visibleColumns = new ArrayList<>(); @@ -263,6 +291,23 @@ private Map buildColumnExprIdMap(List outputSlots, return nameToExprId; } + private Map buildColumnIdExprIdMap(List outputSlots) { + Map result = new HashMap<>(); + List visibleColumns = cols.stream() + .filter(Column::isVisible) + .collect(Collectors.toList()); + List dataSlots = getDataSlots(outputSlots); + if (visibleColumns.size() != dataSlots.size()) { + return result; + } + for (int i = 0; i < visibleColumns.size(); i++) { + if (visibleColumns.get(i).getUniqueId() >= 0) { + result.put(visibleColumns.get(i).getUniqueId(), dataSlots.get(i).getExprId()); + } + } + return result; + } + private List getDataSlots(List outputSlots) { List dataSlots = new ArrayList<>(); for (Slot slot : outputSlots) { @@ -288,9 +333,10 @@ private List getDataSlots(List outputSlots) { private InsertPartitionFieldResult getIcebergPartitioning( List insertPartitionFields, ExternalTable table, - Map columnExprIdMap) { + Map columnExprIdMap, + Map columnIdToExprId) { return buildInsertPartitionFieldsFromConnector( - insertPartitionFields, (PluginDrivenExternalTable) table, columnExprIdMap); + insertPartitionFields, (PluginDrivenExternalTable) table, columnExprIdMap, columnIdToExprId); } /** @@ -304,7 +350,8 @@ private InsertPartitionFieldResult getIcebergPartitioning( private InsertPartitionFieldResult buildInsertPartitionFieldsFromConnector( List insertPartitionFields, PluginDrivenExternalTable table, - Map columnExprIdMap) { + Map columnExprIdMap, + Map columnIdToExprId) { PluginDrivenExternalCatalog catalog = (PluginDrivenExternalCatalog) table.getCatalog(); Connector connector = catalog.getConnector(); ConnectorSession session = catalog.buildConnectorSession(); @@ -322,7 +369,7 @@ private InsertPartitionFieldResult buildInsertPartitionFieldsFromConnector( return new InsertPartitionFieldResult(false, false, null); } ConnectorWritePartitionSpec spec = writePlanProvider.getWritePartitioning(session, handle); - return reconstructPartitionFields(insertPartitionFields, spec, columnExprIdMap); + return reconstructPartitionFields(insertPartitionFields, spec, columnExprIdMap, columnIdToExprId); } /** @@ -349,6 +396,15 @@ static InsertPartitionFieldResult reconstructPartitionFields( List insertPartitionFields, ConnectorWritePartitionSpec spec, Map columnExprIdMap) { + return reconstructPartitionFields(insertPartitionFields, spec, columnExprIdMap, + java.util.Collections.emptyMap()); + } + + static InsertPartitionFieldResult reconstructPartitionFields( + List insertPartitionFields, + ConnectorWritePartitionSpec spec, + Map columnExprIdMap, + Map columnIdToExprId) { if (spec == null) { return new InsertPartitionFieldResult(false, false, null); } @@ -366,7 +422,11 @@ static InsertPartitionFieldResult reconstructPartitionFields( insertPartitionFields.clear(); return new InsertPartitionFieldResult(false, hasNonIdentity, spec.getSpecId()); } - ExprId exprId = columnExprIdMap.get(sourceColumnName); + // Prefer the stable source field id carried by the bind-time schema. A same-name replacement + // must not inherit the old output expression after concurrent Iceberg schema evolution. + ExprId exprId = columnIdToExprId.isEmpty() + ? columnExprIdMap.get(sourceColumnName) + : columnIdToExprId.get(field.getSourceId()); if (exprId == null) { insertPartitionFields.clear(); return new InsertPartitionFieldResult(false, hasNonIdentity, spec.getSpecId()); diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/util/SqlLiteralUtils.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/util/SqlLiteralUtils.java index 5d87bc08f5f56d..0e59cc2c6c1248 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/util/SqlLiteralUtils.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/util/SqlLiteralUtils.java @@ -17,6 +17,7 @@ package org.apache.doris.nereids.util; +import org.apache.doris.common.util.SqlUtils; import org.apache.doris.qe.SqlModeHelper; /** @@ -89,8 +90,6 @@ public static String parseStringLiteral(String text) { * Quote a value as a STRING_LITERAL that can be parsed under the current SQL mode. */ public static String quoteStringLiteral(String value) { - String escaped = SqlModeHelper.hasNoBackSlashEscapes() - ? value : value.replace("\\", "\\\\"); - return "\"" + escaped.replace("\"", "\"\"") + "\""; + return SqlUtils.quoteStringLiteral(value, SqlModeHelper.hasNoBackSlashEscapes()); } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/planner/PluginDrivenTableSink.java b/fe/fe-core/src/main/java/org/apache/doris/planner/PluginDrivenTableSink.java index 3ab86cafff9c6f..ac91ff6b508b4a 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/planner/PluginDrivenTableSink.java +++ b/fe/fe-core/src/main/java/org/apache/doris/planner/PluginDrivenTableSink.java @@ -33,6 +33,8 @@ import org.apache.doris.thrift.TFileFormatType; import org.apache.doris.thrift.TSortInfo; +import com.google.common.collect.ImmutableList; + import java.util.Collections; import java.util.EnumSet; import java.util.List; @@ -58,11 +60,14 @@ public class PluginDrivenTableSink extends BaseExternalTableDataSink { private final ConnectorSession connectorSession; private final ConnectorTableHandle tableHandle; private final List connectorColumns; + private final List boundTargetColumns; // The engine-built BE sort instruction for a connector that declares write-sort columns (iceberg // WRITE ORDERED BY); null when the target needs no write sort. The connector cannot build it (the // bound output exprs live only here), so the translator resolves the connector's declared sort // columns against the sink output and hands the TSortInfo here to thread onto the write handle. private final TSortInfo writeSortInfo; + // Opaque connector metadata generation captured before the engine shaped the physical write. + private final String boundWriteMetadataIdentity; // The DML write operation this sink performs. A plain INSERT sink keeps the default INSERT (the // connector promotes it to OVERWRITE from the handle's isOverwrite() flag); the row-level DML // translator arms (DELETE / UPDATE / MERGE) pass the operation here so the connector's planWrite @@ -121,13 +126,42 @@ public PluginDrivenTableSink(PluginDrivenExternalTable targetTable, ConnectorWritePlanProvider writePlanProvider, ConnectorSession connectorSession, ConnectorTableHandle tableHandle, List connectorColumns, TSortInfo writeSortInfo, WriteOperation writeOperation, boolean requireMergeCardinalityCheck) { + this(targetTable, writePlanProvider, connectorSession, tableHandle, connectorColumns, + connectorColumns, writeSortInfo, writeOperation, requireMergeCardinalityCheck); + } + + /** + * Plan-provider mode with the write subset and complete bind-time target schema carried separately. + */ + public PluginDrivenTableSink(PluginDrivenExternalTable targetTable, + ConnectorWritePlanProvider writePlanProvider, ConnectorSession connectorSession, + ConnectorTableHandle tableHandle, List connectorColumns, + List boundTargetColumns, TSortInfo writeSortInfo, + WriteOperation writeOperation, boolean requireMergeCardinalityCheck) { + this(targetTable, writePlanProvider, connectorSession, tableHandle, connectorColumns, + boundTargetColumns, writeSortInfo, writeOperation, requireMergeCardinalityCheck, null); + } + + /** + * Plan-provider mode with the connector metadata generation that shaped this physical write. + */ + public PluginDrivenTableSink(PluginDrivenExternalTable targetTable, + ConnectorWritePlanProvider writePlanProvider, ConnectorSession connectorSession, + ConnectorTableHandle tableHandle, List connectorColumns, + List boundTargetColumns, TSortInfo writeSortInfo, + WriteOperation writeOperation, boolean requireMergeCardinalityCheck, + String boundWriteMetadataIdentity) { super(); this.targetTable = targetTable; this.writePlanProvider = writePlanProvider; this.connectorSession = connectorSession; this.tableHandle = tableHandle; this.connectorColumns = connectorColumns; + // Keep this immutable bind-time snapshot distinct from the write subset. Re-reading the live + // table here would move the conflict-detection baseline and could miss ordinal schema drift. + this.boundTargetColumns = ImmutableList.copyOf(boundTargetColumns); this.writeSortInfo = writeSortInfo; + this.boundWriteMetadataIdentity = boundWriteMetadataIdentity; this.writeOperation = writeOperation == null ? WriteOperation.INSERT : writeOperation; this.requireMergeCardinalityCheck = requireMergeCardinalityCheck; } @@ -161,8 +195,8 @@ public String getExplainString(String prefix, TExplainLevel explainLevel) { // source-agnostic. This runs before the write plan is bound (planWrite has not run yet for an // EXPLAIN), so the connector derives the detail from the write handle. ConnectorWriteHandle handle = new PluginDrivenWriteHandle( - tableHandle, connectorColumns, false, Collections.emptyMap(), null, Optional.empty(), - writeOperation, requireMergeCardinalityCheck); + tableHandle, connectorColumns, boundTargetColumns, false, Collections.emptyMap(), null, + null, Optional.empty(), writeOperation, requireMergeCardinalityCheck); writePlanProvider.appendExplainInfo(sb, prefix, connectorSession, handle); return sb.toString(); } @@ -188,8 +222,8 @@ public void bindDataSink(Optional insertCtx) branchName = ctx.getBranchName(); } ConnectorWriteHandle handle = new PluginDrivenWriteHandle( - tableHandle, connectorColumns, overwrite, writeContext, writeSortInfo, branchName, - writeOperation, requireMergeCardinalityCheck); + tableHandle, connectorColumns, boundTargetColumns, overwrite, writeContext, writeSortInfo, + boundWriteMetadataIdentity, branchName, writeOperation, requireMergeCardinalityCheck); ConnectorSinkPlan sinkPlan = writePlanProvider.planWrite(connectorSession, handle); this.tDataSink = sinkPlan.getDataSink(); } @@ -205,22 +239,27 @@ public PluginDrivenExternalTable getTargetTable() { private static final class PluginDrivenWriteHandle implements ConnectorWriteHandle { private final ConnectorTableHandle tableHandle; private final List columns; + private final List boundTargetColumns; private final boolean overwrite; private final Map writeContext; private final TSortInfo sortInfo; + private final String boundWriteMetadataIdentity; private final Optional branchName; private final WriteOperation writeOperation; private final boolean requireMergeCardinalityCheck; private PluginDrivenWriteHandle(ConnectorTableHandle tableHandle, List columns, - boolean overwrite, Map writeContext, TSortInfo sortInfo, + List boundTargetColumns, boolean overwrite, + Map writeContext, TSortInfo sortInfo, String boundWriteMetadataIdentity, Optional branchName, WriteOperation writeOperation, boolean requireMergeCardinalityCheck) { this.tableHandle = tableHandle; this.columns = columns; + this.boundTargetColumns = boundTargetColumns; this.overwrite = overwrite; this.writeContext = writeContext; this.sortInfo = sortInfo; + this.boundWriteMetadataIdentity = boundWriteMetadataIdentity; this.branchName = branchName == null ? Optional.empty() : branchName; this.writeOperation = writeOperation == null ? WriteOperation.INSERT : writeOperation; this.requireMergeCardinalityCheck = requireMergeCardinalityCheck; @@ -236,6 +275,11 @@ public TSortInfo getSortInfo() { return sortInfo; } + @Override + public String getBoundWriteMetadataIdentity() { + return boundWriteMetadataIdentity; + } + @Override public Optional getBranchName() { return branchName; @@ -251,6 +295,11 @@ public List getColumns() { return columns; } + @Override + public List getBoundTargetColumns() { + return boundTargetColumns; + } + @Override public boolean isOverwrite() { return overwrite; diff --git a/fe/fe-core/src/test/java/org/apache/doris/common/proc/IndexSchemaProcNodeTest.java b/fe/fe-core/src/test/java/org/apache/doris/common/proc/IndexSchemaProcNodeTest.java index 16d360d3adbb3f..c65d6795d3ed3a 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/common/proc/IndexSchemaProcNodeTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/common/proc/IndexSchemaProcNodeTest.java @@ -27,10 +27,13 @@ import org.apache.doris.catalog.info.TableNameInfo; import org.apache.doris.common.AnalysisException; import org.apache.doris.datasource.InternalCatalog; +import org.apache.doris.qe.SqlModeHelper; import com.google.common.collect.Lists; import org.junit.Assert; import org.junit.Test; +import org.mockito.MockedStatic; +import org.mockito.Mockito; import java.util.List; @@ -82,9 +85,9 @@ public void testCreateResultPreservesNestedRequirednessWithAndWithoutComments() Lists.newArrayList(IndexSchemaProcNode.COMMENT_COLUMN_TITLE)) .getRows().get(0).get(1); Assert.assertTrue(typeWithComments.contains( - "required_value:int not null comment 'required-comment'")); + "required_value:int not null comment \"required-comment\"")); Assert.assertTrue(typeWithComments.contains( - "optional_value:int comment 'optional-comment'")); + "optional_value:int comment \"optional-comment\"")); String typeWithoutComments = IndexSchemaProcNode.createResult( Lists.newArrayList(column), null, Lists.newArrayList()) @@ -93,4 +96,21 @@ public void testCreateResultPreservesNestedRequirednessWithAndWithoutComments() Assert.assertFalse(typeWithoutComments.contains("required-comment")); Assert.assertFalse(typeWithoutComments.contains("optional-comment")); } + + @Test + public void testCreateResultQuotesNestedCommentsAsSqlLiterals() { + StructType structType = new StructType( + new StructField("value", Type.INT, "owner's \\path", true)); + Column column = new Column("info", structType, true, null, true, "", "top-level-comment"); + + try (MockedStatic mockedSqlMode = Mockito.mockStatic(SqlModeHelper.class)) { + mockedSqlMode.when(SqlModeHelper::hasNoBackSlashEscapes).thenReturn(false); + String displayedType = IndexSchemaProcNode.createResult( + Lists.newArrayList(column), null, + Lists.newArrayList(IndexSchemaProcNode.COMMENT_COLUMN_TITLE)) + .getRows().get(0).get(1); + + Assert.assertTrue(displayedType.contains("comment \"owner's \\\\path\"")); + } + } } diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/connector/converter/ConnectorColumnConverterTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/connector/converter/ConnectorColumnConverterTest.java index a55475793595a0..cfd8ccddf253d2 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/connector/converter/ConnectorColumnConverterTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/connector/converter/ConnectorColumnConverterTest.java @@ -41,6 +41,10 @@ class ConnectorColumnConverterTest { void testScalarTypeRoundtrip() { // INT → ConnectorType → Doris Type should roundtrip ConnectorType ct = ConnectorColumnConverter.toConnectorType(ScalarType.INT); + // Parameterless scalars must use the connector's canonical -1/-1 representation. Iceberg publishes + // INT this way; emitting Doris' internal 0/0 defaults makes an unchanged write look like schema drift. + Assertions.assertEquals(-1, ct.getPrecision()); + Assertions.assertEquals(-1, ct.getScale()); Type back = ConnectorColumnConverter.convertType(ct); Assertions.assertEquals(ScalarType.INT, back); } @@ -382,7 +386,7 @@ void convertColumnStampsNestedFieldIdsOntoChildTree() { // them recursively). The connector carries the top-level id on ConnectorColumn.withUniqueId and the // per-child ids on ConnectorType.withChildrenFieldIds; convertColumn must stamp the whole child tree so // SlotTypeReplacer can rewrite the nested access path to ids and BE matches the pruned leaf by id (a -1 - // leaf is skipped -> NULL). MUTATION: dropping the applyNestedFieldIds call leaves children at -1 -> red. + // leaf is skipped -> NULL). MUTATION: dropping applyNestedFieldMetadata leaves children at -1 -> red. // struct, top-level field-id 3 ConnectorType structType = ConnectorType.structOf( Arrays.asList("a", "b"), @@ -424,10 +428,30 @@ void convertColumnStampsDeeplyNestedAndArrayMapFieldIds() { Assertions.assertEquals(10, mapCol.getChildren().get(1).getUniqueId(), "map value carries field-id 10"); } + @Test + void toConnectorColumnPreservesDeeplyNestedFieldIds() { + ConnectorType inner = ConnectorType.structOf( + Arrays.asList("leaf"), Arrays.asList(ConnectorType.of("INT"))) + .withChildrenFieldIds(Arrays.asList(12)); + ConnectorType array = ConnectorType.arrayOf(inner, false).withChildrenFieldIds(Arrays.asList(11)); + Column column = ConnectorColumnConverter.convertColumn( + new ConnectorColumn("payload", array, "", true, null).withUniqueId(10)); + Assertions.assertFalse(column.getChildren().get(0).isAllowNull(), + "the Doris child column must retain the connector element requiredness"); + + ConnectorColumn rebound = ConnectorColumnConverter.toConnectorColumn(column); + + // Write conflict detection needs the full identity path, not only the top-level column id. + Assertions.assertEquals(10, rebound.getUniqueId()); + Assertions.assertEquals(11, rebound.getType().getChildFieldId(0)); + Assertions.assertEquals(12, rebound.getType().getChildren().get(0).getChildFieldId(0)); + Assertions.assertFalse(rebound.getType().isChildNullable(0)); + } + @Test void convertColumnLeavesNestedUniqueIdsUnsetWithoutFieldIds() { // Regression guard: a connector that does NOT carry nested field ids (no withChildrenFieldIds, e.g. - // paimon) must leave every child uniqueId at the default -1 — applyNestedFieldIds must be inert, so a + // paimon) must leave every child uniqueId at the default -1 — field-id propagation must be inert, so a // non-iceberg connector's nested columns are never accidentally stamped. ConnectorType structType = ConnectorType.structOf( Arrays.asList("a", "b"), diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/plugin/PluginDrivenExternalTablePartitionTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/plugin/PluginDrivenExternalTablePartitionTest.java index 82fe28985d8d78..c64329932d4f8a 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/plugin/PluginDrivenExternalTablePartitionTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/plugin/PluginDrivenExternalTablePartitionTest.java @@ -186,7 +186,8 @@ public void testInitSchemaExtractsPartitionColumnsMappingRemoteNames() { new ConnectorColumn("REGION", ConnectorType.of("INT"), "", true, null), new ConnectorColumn("VAL", ConnectorType.of("INT"), "", true, null)), "max_compute", - Collections.singletonMap(ConnectorTableSchema.PARTITION_COLUMNS_KEY, "YEAR,REGION")); + Collections.singletonMap(ConnectorTableSchema.PARTITION_COLUMNS_KEY, "YEAR,REGION"), + Collections.emptySet(), "uuid-u0/schema-1"); Mockito.when(metadata.getTableSchema(session, handle)).thenReturn(tableSchema); // Identifier mapping lowercases the remote names (raw "YEAR" -> mapped "year"). Mockito.when(metadata.fromRemoteColumnName(Mockito.eq(session), Mockito.anyString(), @@ -206,6 +207,8 @@ public void testInitSchemaExtractsPartitionColumnsMappingRemoteNames() { "partition columns must be the MAPPED Doris columns identified via fromRemoteColumnName"); Assertions.assertEquals(Arrays.asList("YEAR", "REGION"), value.getPartitionColumnRemoteNames(), "remote names must be kept raw for addressing connector partition values"); + Assertions.assertEquals("uuid-u0/schema-1", value.getWriteMetadataIdentity(), + "schema caching must preserve the opaque generation captured with the remote columns"); } @Test diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslatorAdmissionGateTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslatorAdmissionGateTest.java index dbaf7e296d113b..5368c62961fcff 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslatorAdmissionGateTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslatorAdmissionGateTest.java @@ -17,25 +17,33 @@ package org.apache.doris.nereids.glue.translator; +import org.apache.doris.analysis.SlotDescriptor; +import org.apache.doris.analysis.SlotRef; +import org.apache.doris.analysis.TupleDescriptor; import org.apache.doris.catalog.Column; import org.apache.doris.catalog.PrimitiveType; import org.apache.doris.common.jmockit.Deencapsulation; import org.apache.doris.connector.api.Connector; +import org.apache.doris.connector.api.ConnectorColumn; import org.apache.doris.connector.api.ConnectorMetadata; import org.apache.doris.connector.api.ConnectorSession; import org.apache.doris.connector.api.ConnectorStatementScope; import org.apache.doris.connector.api.handle.ConnectorTableHandle; import org.apache.doris.connector.api.handle.WriteOperation; import org.apache.doris.connector.api.write.ConnectorWritePlanProvider; +import org.apache.doris.connector.api.write.ConnectorWriteSortColumn; import org.apache.doris.datasource.plugin.PluginDrivenExternalCatalog; import org.apache.doris.datasource.plugin.PluginDrivenExternalTable; import org.apache.doris.nereids.exceptions.AnalysisException; +import org.apache.doris.nereids.trees.expressions.SlotReference; import org.apache.doris.nereids.trees.plans.Plan; import org.apache.doris.nereids.trees.plans.physical.PhysicalConnectorTableSink; import org.apache.doris.nereids.trees.plans.physical.PhysicalExternalRowLevelDeleteSink; +import org.apache.doris.nereids.types.IntegerType; import org.apache.doris.planner.DataSink; import org.apache.doris.planner.PlanFragment; import org.apache.doris.planner.PluginDrivenTableSink; +import org.apache.doris.thrift.TSortInfo; import com.google.common.collect.ImmutableList; import org.junit.jupiter.api.Assertions; @@ -44,6 +52,7 @@ import org.mockito.Mockito; import java.util.EnumSet; +import java.util.List; import java.util.Optional; import java.util.Set; @@ -59,6 +68,9 @@ public class PhysicalPlanTranslatorAdmissionGateTest { private static final Column DATA = new Column("data", PrimitiveType.INT); + private static final Column A = new Column("a", PrimitiveType.INT); + private static final Column B = new Column("b", PrimitiveType.INT); + private static final Column C = new Column("c", PrimitiveType.INT); @Test public void insertGateAllowsConnectorDeclaringInsert() { @@ -129,6 +141,68 @@ public void rowLevelDmlGateRejectsConnectorDeclaringOnlyInsertWithDistinctMessag + ex.getMessage()); } + @Test + public void rowLevelDmlPreservesBindTimeWriteMetadataIdentity() { + PlanTranslatorContext context = new PlanTranslatorContext(); + PlanFragment childFragment = Mockito.mock(PlanFragment.class); + ConnectorWritePlanProvider provider = Mockito.mock(ConnectorWritePlanProvider.class); + PluginDrivenExternalTable table = pluginTable(EnumSet.of(WriteOperation.DELETE), provider); + + @SuppressWarnings("unchecked") + PhysicalExternalRowLevelDeleteSink sink = Mockito.mock(PhysicalExternalRowLevelDeleteSink.class); + Mockito.doReturn(mockChild(childFragment)).when(sink).child(); + Mockito.doReturn(table).when(sink).getTargetTable(); + Mockito.doReturn(ImmutableList.of(DATA)).when(sink).getCols(); + Mockito.doReturn("uuid-u0/schema-1").when(sink).getBoundWriteMetadataIdentity(); + + new PhysicalPlanTranslator(context, null).visitPhysicalExternalRowLevelDeleteSink(sink, context); + + PluginDrivenTableSink pluginSink = capturePluginSink(childFragment); + Assertions.assertEquals("uuid-u0/schema-1", + Deencapsulation.getField(pluginSink, "boundWriteMetadataIdentity")); + // Re-reading at translation time would accept a replacement table as this write's conflict baseline. + Mockito.verify(provider, Mockito.never()).getWriteMetadataIdentity(Mockito.any(), Mockito.any()); + } + + @Test + public void partialInsertResolvesWriteSortAgainstFullOutput() { + assertWriteSortUsesBoundOutputColumn(ImmutableList.of(C), C); + } + + @Test + public void reorderedInsertResolvesWriteSortAgainstFullOutput() { + assertWriteSortUsesBoundOutputColumn(ImmutableList.of(C, A), C); + } + + @Test + public void staticPartitionInsertKeepsSortColumnInFullOutput() { + assertWriteSortUsesBoundOutputColumn(ImmutableList.of(A, C), B); + } + + @Test + public void insertPreservesBindTimeWriteMetadataIdentity() { + PlanTranslatorContext context = new PlanTranslatorContext(); + PlanFragment childFragment = Mockito.mock(PlanFragment.class); + ConnectorWritePlanProvider provider = Mockito.mock(ConnectorWritePlanProvider.class); + PluginDrivenExternalTable table = pluginTable(EnumSet.of(WriteOperation.INSERT), provider); + + @SuppressWarnings("unchecked") + PhysicalConnectorTableSink sink = Mockito.mock(PhysicalConnectorTableSink.class); + Mockito.doReturn(mockChild(childFragment)).when(sink).child(); + Mockito.doReturn(table).when(sink).getTargetTable(); + Mockito.doReturn(ImmutableList.of(DATA)).when(sink).getCols(); + Mockito.doReturn("uuid-u0/schema-1").when(sink).getBoundWriteMetadataIdentity(); + Mockito.doReturn(false).when(sink).isRewrite(); + + new PhysicalPlanTranslator(context, null).visitPhysicalConnectorTableSink(sink, context); + + PluginDrivenTableSink pluginSink = capturePluginSink(childFragment); + Assertions.assertEquals("uuid-u0/schema-1", + Deencapsulation.getField(pluginSink, "boundWriteMetadataIdentity")); + // A translator-time refresh must not move the fence away from the schema used by BindSink. + Mockito.verify(provider, Mockito.never()).getWriteMetadataIdentity(Mockito.any(), Mockito.any()); + } + // ==================== helpers ==================== private static Plan mockChild(PlanFragment childFragment) { @@ -137,15 +211,75 @@ private static Plan mockChild(PlanFragment childFragment) { return child; } + private static void assertWriteSortUsesBoundOutputColumn(List writeColumns, Column sortColumn) { + PlanTranslatorContext context = new PlanTranslatorContext(); + PlanFragment childFragment = Mockito.mock(PlanFragment.class); + ConnectorWritePlanProvider provider = Mockito.mock(ConnectorWritePlanProvider.class); + Mockito.when(provider.getWriteSortColumns(Mockito.any(), Mockito.any(), Mockito.anyList())) + .thenAnswer(invocation -> { + List columns = invocation.getArgument(2); + for (int i = 0; i < columns.size(); i++) { + if (columns.get(i).getName().equals(sortColumn.getName())) { + return ImmutableList.of(new ConnectorWriteSortColumn(i, true, true)); + } + } + return ImmutableList.of(); + }); + PluginDrivenExternalTable table = pluginTable(EnumSet.of(WriteOperation.INSERT), provider); + Mockito.when(table.requiresFullSchemaWriteOrder()).thenReturn(true); + + SlotReference aOutput = new SlotReference("a", IntegerType.INSTANCE); + SlotReference bOutput = new SlotReference("b", IntegerType.INSTANCE); + SlotReference cOutput = new SlotReference("c", IntegerType.INSTANCE); + TupleDescriptor tuple = context.generateTupleDesc(); + SlotRef aSlot = registerLegacySlot(context, tuple, aOutput, A); + SlotRef bSlot = registerLegacySlot(context, tuple, bOutput, B); + SlotRef cSlot = registerLegacySlot(context, tuple, cOutput, C); + + @SuppressWarnings("unchecked") + PhysicalConnectorTableSink sink = Mockito.mock(PhysicalConnectorTableSink.class); + Mockito.doReturn(mockChild(childFragment)).when(sink).child(); + Mockito.doReturn(table).when(sink).getTargetTable(); + Mockito.doReturn(writeColumns).when(sink).getCols(); + Mockito.doReturn(ImmutableList.of(A, B, C)).when(sink).getBoundTargetSchema(); + Mockito.doReturn(ImmutableList.of(aOutput, bOutput, cOutput)).when(sink).getOutput(); + Mockito.doReturn(false).when(sink).isRewrite(); + + new PhysicalPlanTranslator(context, null).visitPhysicalConnectorTableSink(sink, context); + + TSortInfo sortInfo = Deencapsulation.getField(capturePluginSink(childFragment), "writeSortInfo"); + Assertions.assertNotNull(sortInfo); + Assertions.assertEquals(1, sortInfo.getOrderingExprsSize()); + int actualSlotId = sortInfo.getOrderingExprs().get(0).getNodes().get(0).getSlotRef().getSlotId(); + SlotRef expected = sortColumn == A ? aSlot : sortColumn == B ? bSlot : cSlot; + Assertions.assertEquals(expected.getDesc().getId().asInt(), actualSlotId, + "write-sort position must use the same full-schema coordinates as the sink output"); + } + + private static SlotRef registerLegacySlot(PlanTranslatorContext context, TupleDescriptor tuple, + SlotReference output, Column column) { + SlotDescriptor descriptor = context.addSlotDesc(tuple); + descriptor.setColumn(column); + descriptor.setType(column.getType()); + descriptor.setIsNullable(true); + SlotRef slotRef = new SlotRef(descriptor); + context.addExprIdSlotRefPair(output.getExprId(), slotRef); + return slotRef; + } + /** A plugin-driven table whose connector declares exactly the given write operations. */ private static PluginDrivenExternalTable pluginTable(Set ops) { + return pluginTable(ops, Mockito.mock(ConnectorWritePlanProvider.class)); + } + + private static PluginDrivenExternalTable pluginTable(Set ops, + ConnectorWritePlanProvider provider) { ConnectorTableHandle handle = Mockito.mock(ConnectorTableHandle.class); ConnectorMetadata metadata = Mockito.mock(ConnectorMetadata.class); ConnectorSession session = Mockito.mock(ConnectorSession.class); // The write seams now resolve metadata through the per-statement funnel, which reads the session's // statement scope; offline tests use NONE (a fresh getMetadata per call, byte-identical to pre-funnel). Mockito.when(session.getStatementScope()).thenReturn(ConnectorStatementScope.NONE); - ConnectorWritePlanProvider provider = Mockito.mock(ConnectorWritePlanProvider.class); Connector connector = Mockito.mock(Connector.class); Mockito.when(connector.getWritePlanProvider()).thenReturn(provider); // Production selects the write provider per-handle; a plain mock does not run the interface default. diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslatorIcebergRowLevelDmlTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslatorIcebergRowLevelDmlTest.java index a829f03a7d0715..45b765664ed8ba 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslatorIcebergRowLevelDmlTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslatorIcebergRowLevelDmlTest.java @@ -393,5 +393,9 @@ private static void assertConnectorColumnsFromCols(PluginDrivenTableSink pluginS "the connector columns must be derived from the sink's getCols()"); Assertions.assertEquals("data", connectorColumns.get(0).getName(), "the connector column name must carry the sink column name"); + // This is the exact translator output consumed by the provider. Parameterless Doris scalars expose + // internal 0/0 defaults, but the connector schema contract uses the canonical -1/-1 representation. + Assertions.assertEquals(-1, connectorColumns.get(0).getType().getPrecision()); + Assertions.assertEquals(-1, connectorColumns.get(0).getType().getScale()); } } diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/BindConnectorSinkStaticPartitionTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/BindConnectorSinkStaticPartitionTest.java index 4a4c3620629533..8c5ec289625897 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/BindConnectorSinkStaticPartitionTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/BindConnectorSinkStaticPartitionTest.java @@ -22,6 +22,7 @@ import org.apache.doris.datasource.plugin.PluginDrivenExternalTable; import org.apache.doris.nereids.exceptions.AnalysisException; import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.NamedExpression; import org.apache.doris.nereids.trees.expressions.literal.StringLiteral; import com.google.common.collect.ImmutableList; @@ -34,7 +35,6 @@ import java.util.Collections; import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; @@ -61,7 +61,7 @@ public class BindConnectorSinkStaticPartitionTest { private static PluginDrivenExternalTable partitionedTable() { PluginDrivenExternalTable table = Mockito.mock(PluginDrivenExternalTable.class); Mockito.when(table.getBaseSchema(true)).thenReturn(BASE_SCHEMA); - Mockito.when(table.getFullSchema(Optional.empty())).thenReturn(BASE_SCHEMA); + stubWriteSchemaSnapshot(table, BASE_SCHEMA, ImmutableList.of(DS, REGION)); // Model ExternalTable.getColumn, which resolves case-INSENSITIVELY (equalsIgnoreCase) for every // external table. Stubbing only the exact spelling would hide the very behavior under test. Mockito.when(table.getColumn(Mockito.anyString())).thenAnswer(inv -> { @@ -82,23 +82,40 @@ private static Map partitionSpec(String... colNames) { return spec; } - /** - * A table carrying an invisible column after the visible data columns, modelling an iceberg v3 table - * whose row-lineage {@code _row_id} is appended {@code .invisible()} by the connector. - */ - private static PluginDrivenExternalTable tableWithRowLineage() { - Column rowId = new Column("_row_id", PrimitiveType.BIGINT); - rowId.setIsVisible(false); - List schema = ImmutableList.of(ID, VAL, rowId); + private static PluginDrivenExternalTable tableWithRewriteColumns(boolean includeLineage) { + Column rowLocator = new Column("__DORIS_ICEBERG_ROWID_COL__", PrimitiveType.STRING); + rowLocator.setIsVisible(false); + ImmutableList.Builder schemaBuilder = ImmutableList.builder(); + schemaBuilder.add(ID, VAL); + if (includeLineage) { + Column rowId = new Column("_row_id", PrimitiveType.BIGINT); + rowId.setIsVisible(false); + rowId.setReservedPassthrough(true); + Column sequenceNumber = new Column("_last_updated_sequence_number", PrimitiveType.BIGINT); + sequenceNumber.setIsVisible(false); + sequenceNumber.setReservedPassthrough(true); + schemaBuilder.add(rowId, sequenceNumber); + } + schemaBuilder.add(rowLocator); + List schema = schemaBuilder.build(); PluginDrivenExternalTable table = Mockito.mock(PluginDrivenExternalTable.class); Mockito.when(table.getBaseSchema(true)).thenReturn(schema); - Mockito.when(table.getFullSchema(Optional.empty())).thenReturn(schema); + stubWriteSchemaSnapshot(table, schema, Collections.emptyList()); for (Column c : schema) { Mockito.when(table.getColumn(c.getName())).thenReturn(c); } return table; } + private static void stubWriteSchemaSnapshot(PluginDrivenExternalTable table, + List schema, List partitionColumns) { + PluginDrivenExternalTable.WriteSchemaSnapshot snapshot = + Mockito.mock(PluginDrivenExternalTable.WriteSchemaSnapshot.class); + Mockito.when(snapshot.getFullSchema()).thenReturn(schema); + Mockito.when(snapshot.getPartitionColumns()).thenReturn(partitionColumns); + Mockito.when(table.getWriteSchemaSnapshot()).thenReturn(snapshot); + } + private static List names(List columns) { return columns.stream().map(Column::getName).collect(Collectors.toList()); } @@ -163,7 +180,7 @@ public void explicitColumnListUsesLatestTargetSchemaInsteadOfAmbientSourceSnapsh // therefore sees old_name, while an explicit empty pin means the latest write-target schema. Mockito.when(table.getColumn("id")).thenReturn(ID); Mockito.when(table.getColumn("new_name")).thenReturn(null); - Mockito.when(table.getFullSchema(Optional.empty())).thenReturn(ImmutableList.of(ID, newName)); + stubWriteSchemaSnapshot(table, ImmutableList.of(ID, newName), Collections.emptyList()); Mockito.when(table.getBaseSchema(true)).thenReturn(ImmutableList.of(ID, oldName)); List bound = BindSink.selectConnectorSinkBindColumns( @@ -172,6 +189,17 @@ public void explicitColumnListUsesLatestTargetSchemaInsteadOfAmbientSourceSnapsh "a historical source pin must not replace the latest write-target schema"); } + @Test + public void explicitColumnListLoadsLatestTargetSchemaOnce() { + PluginDrivenExternalTable table = partitionedTable(); + + List bound = BindSink.selectConnectorSinkBindColumns( + table, ImmutableList.of("id", "val", "region"), Collections.emptySet(), false); + + Assertions.assertEquals(ImmutableList.of("id", "val", "region"), names(bound)); + Mockito.verify(table, Mockito.times(1)).getWriteSchemaSnapshot(); + } + /** * A column whose value comes from the PARTITION clause must not ALSO be listed in the insert column * list. Encodes WHY: the materialize block re-projects the PARTITION literal over that column, so the @@ -294,21 +322,57 @@ public void explicitColumnListUnknownColumnThrows() { @Test public void noColumnListOrdinaryWriteExcludesInvisibleColumns() { List bound = BindSink.selectConnectorSinkBindColumns( - tableWithRowLineage(), Collections.emptyList(), Collections.emptySet(), false); + tableWithRewriteColumns(true), Collections.emptyList(), Collections.emptySet(), false); Assertions.assertEquals(ImmutableList.of("id", "val"), names(bound), "invisible row-lineage columns must be excluded from an ordinary write target"); } /** - * No column list, rewrite (distributed {@code rewrite_data_files}): invisible columns are RETAINED so - * the engine-managed row-lineage values read from the source rows are preserved through the rewrite, - * mirroring the legacy {@code bindIcebergTableSink} rewrite branch. + * A v2 rewrite under show-hidden carries the request-scoped row locator in the table's full schema, but + * the rewrite sink has no physical field for it. */ @Test - public void noColumnListRewriteRetainsInvisibleColumns() { + public void noColumnListV2RewriteExcludesRequestScopedRowLocator() { List bound = BindSink.selectConnectorSinkBindColumns( - tableWithRowLineage(), Collections.emptyList(), Collections.emptySet(), true); - Assertions.assertEquals(ImmutableList.of("id", "val", "_row_id"), names(bound), - "a rewrite must retain invisible row-lineage columns to preserve their values"); + tableWithRewriteColumns(false), Collections.emptyList(), Collections.emptySet(), true); + Assertions.assertEquals(ImmutableList.of("id", "val"), names(bound), + "a v2 rewrite must not emit the request-scoped row locator"); + } + + /** + * A v3 rewrite preserves persistent lineage fields, while excluding the unrelated request-scoped locator. + */ + @Test + public void noColumnListV3RewriteRetainsLineageButExcludesRequestScopedRowLocator() { + List bound = BindSink.selectConnectorSinkBindColumns( + tableWithRewriteColumns(true), Collections.emptyList(), Collections.emptySet(), true); + Assertions.assertEquals(ImmutableList.of("id", "val", "_row_id", "_last_updated_sequence_number"), + names(bound), "a v3 rewrite must retain only persistent lineage metadata"); + } + + @Test + public void rewriteSourceOutputExcludesRequestScopedRowLocator() { + NamedExpression id = namedExpression("id"); + NamedExpression val = namedExpression("val"); + NamedExpression rowId = namedExpression("_row_id"); + NamedExpression sequenceNumber = namedExpression("_last_updated_sequence_number"); + NamedExpression locator = namedExpression("__DORIS_ICEBERG_ROWID_COL__"); + + List v2Selected = BindSink.selectConnectorRewriteOutputs( + ImmutableList.of(ID, VAL), ImmutableList.of(id, val, locator)); + List v3WriteSchema = BindSink.selectConnectorSinkBindColumns( + tableWithRewriteColumns(true), Collections.emptyList(), Collections.emptySet(), true); + List v3Selected = BindSink.selectConnectorRewriteOutputs( + v3WriteSchema, ImmutableList.of(id, val, rowId, sequenceNumber, locator)); + + Assertions.assertEquals(ImmutableList.of(id, val), v2Selected); + Assertions.assertEquals(ImmutableList.of(id, val, rowId, sequenceNumber), v3Selected, + "v2/v3 rewrite input must use the same physical column set as its sink schema"); + } + + private static NamedExpression namedExpression(String name) { + NamedExpression expression = Mockito.mock(NamedExpression.class); + Mockito.when(expression.getName()).thenReturn(name); + return expression; } } diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/literal/StructLiteralTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/literal/StructLiteralTest.java index c367d8ade27594..1982a6ae67f742 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/literal/StructLiteralTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/literal/StructLiteralTest.java @@ -18,15 +18,23 @@ package org.apache.doris.nereids.trees.expressions.literal; import org.apache.doris.nereids.rules.expression.check.CheckCast; +import org.apache.doris.nereids.trees.expressions.Cast; +import org.apache.doris.nereids.trees.expressions.SlotReference; +import org.apache.doris.nereids.trees.expressions.TryCast; import org.apache.doris.nereids.trees.expressions.functions.scalar.CreateNamedStruct; +import org.apache.doris.nereids.trees.expressions.functions.scalar.CreateStruct; import org.apache.doris.nereids.types.BigIntType; +import org.apache.doris.nereids.types.IntegerType; import org.apache.doris.nereids.types.StringType; import org.apache.doris.nereids.types.StructField; import org.apache.doris.nereids.types.StructType; +import org.apache.doris.qe.SessionVariable; import com.google.common.collect.ImmutableList; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; +import org.mockito.MockedStatic; +import org.mockito.Mockito; public class StructLiteralTest { @@ -65,4 +73,37 @@ public void testNamedStructInfersValueNullability() { StructType nullableType = (StructType) nullable.customSignature().returnType; Assertions.assertTrue(nullableType.getFields().get(0).isNullable()); } + + @Test + public void testStructFunctionsKeepPhysicalCastNullabilityInStrictMode() { + SlotReference requiredString = new SlotReference("metric", StringType.INSTANCE, false); + Cast cast = new Cast(requiredString, IntegerType.INSTANCE); + TryCast tryCast = new TryCast(requiredString, IntegerType.INSTANCE); + + try (MockedStatic mockedSessionVariable = Mockito.mockStatic(SessionVariable.class)) { + mockedSessionVariable.when(SessionVariable::enableStrictCast).thenReturn(true); + // Strict-mode failure semantics do not change the BE column representation: narrowing + // casts still return ColumnNullable with an all-clear map for valid rows. + Assertions.assertTrue(cast.nullable()); + + CreateStruct struct = new CreateStruct(cast); + StructType structType = (StructType) struct.getSignatures().get(0).returnType; + Assertions.assertTrue(structType.getFields().get(0).isNullable()); + + CreateNamedStruct namedStruct = new CreateNamedStruct(new StringLiteral("metric"), cast); + StructType namedStructType = (StructType) namedStruct.customSignature().returnType; + Assertions.assertTrue(namedStructType.getFields().get(0).isNullable()); + + CreateNamedStruct namedTryStruct = new CreateNamedStruct(new StringLiteral("metric"), tryCast); + StructType namedTryStructType = (StructType) namedTryStruct.customSignature().returnType; + Assertions.assertTrue(namedTryStructType.getFields().get(0).isNullable()); + } + + try (MockedStatic mockedSessionVariable = Mockito.mockStatic(SessionVariable.class)) { + mockedSessionVariable.when(SessionVariable::enableStrictCast).thenReturn(false); + CreateStruct struct = new CreateStruct(cast); + StructType structType = (StructType) struct.getSignatures().get(0).returnType; + Assertions.assertTrue(structType.getFields().get(0).isNullable()); + } + } } diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/ExternalRowLevelMergePlanBuilderTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/ExternalRowLevelMergePlanBuilderTest.java index 4f2ae8aa274df2..e95f1d4e522f12 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/ExternalRowLevelMergePlanBuilderTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/ExternalRowLevelMergePlanBuilderTest.java @@ -22,7 +22,7 @@ import org.apache.doris.catalog.ScalarType; import org.apache.doris.common.FeConstants; import org.apache.doris.datasource.ExternalDatabase; -import org.apache.doris.datasource.ExternalTable; +import org.apache.doris.datasource.plugin.PluginDrivenExternalTable; import org.apache.doris.nereids.analyzer.UnboundAlias; import org.apache.doris.nereids.analyzer.UnboundSlot; import org.apache.doris.nereids.trees.expressions.EqualTo; @@ -84,10 +84,14 @@ public void mergeSinkRequestsTheSqlMergeCardinalityCheck() { Column rowId = new Column(Column.ICEBERG_ROWID_COL, ScalarType.createStringType()); rowId.setIsVisible(false); Column data = new Column("c1", ScalarType.createType(PrimitiveType.INT)); - ExternalTable table = Mockito.mock(ExternalTable.class); + PluginDrivenExternalTable table = Mockito.mock(PluginDrivenExternalTable.class); + PluginDrivenExternalTable.WriteSchemaSnapshot writeSchema = + Mockito.mock(PluginDrivenExternalTable.WriteSchemaSnapshot.class); Mockito.when(table.getName()).thenReturn("test_table"); Mockito.doReturn(Mockito.mock(ExternalDatabase.class)).when(table).getDatabase(); - Mockito.doReturn(ImmutableList.of(data)).when(table).getBaseSchema(true); + Mockito.doReturn(ImmutableList.of(data)).when(writeSchema).getBaseSchema(); + Mockito.doReturn("uuid-u0/schema-1").when(writeSchema).getWriteMetadataIdentity(); + Mockito.doReturn(writeSchema).when(table).getWriteSchemaSnapshot(); Mockito.doReturn(ImmutableList.of(data, rowId)).when(table).getFullSchema(); LogicalPlan plan = builder.buildMergePlan(ctx, table); @@ -100,5 +104,9 @@ public void mergeSinkRequestsTheSqlMergeCardinalityCheck() { Assertions.assertTrue(plan instanceof LogicalExternalRowLevelMergeSink); Assertions.assertTrue(((LogicalExternalRowLevelMergeSink) plan).isRequireMergeCardinalityCheck(), "SQL MERGE INTO must request the cardinality validation"); + // The branch projections, sink columns, and conflict fence must come from one generation. + Assertions.assertEquals("uuid-u0/schema-1", + ((LogicalExternalRowLevelMergeSink) plan).getBoundWriteMetadataIdentity()); + Mockito.verify(table, Mockito.times(1)).getWriteSchemaSnapshot(); } } diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/ExternalRowLevelUpdatePlanBuilderTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/ExternalRowLevelUpdatePlanBuilderTest.java index 33be03bc8a0ebf..a92f3c1ee013a6 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/ExternalRowLevelUpdatePlanBuilderTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/ExternalRowLevelUpdatePlanBuilderTest.java @@ -22,7 +22,7 @@ import org.apache.doris.catalog.ScalarType; import org.apache.doris.common.FeConstants; import org.apache.doris.datasource.ExternalDatabase; -import org.apache.doris.datasource.ExternalTable; +import org.apache.doris.datasource.plugin.PluginDrivenExternalTable; import org.apache.doris.nereids.analyzer.UnboundAlias; import org.apache.doris.nereids.analyzer.UnboundSlot; import org.apache.doris.nereids.trees.expressions.NamedExpression; @@ -113,17 +113,25 @@ public void updateSinkCarriesNoSqlMergeCardinalityRequirement() { ExternalRowLevelUpdatePlanBuilder builder = new ExternalRowLevelUpdatePlanBuilder( ImmutableList.of("test_catalog", "test_db", "test_table"), null, ImmutableList.of(), basePlan); - ExternalTable table = Mockito.mock(ExternalTable.class); + PluginDrivenExternalTable table = Mockito.mock(PluginDrivenExternalTable.class); + PluginDrivenExternalTable.WriteSchemaSnapshot writeSchema = + Mockito.mock(PluginDrivenExternalTable.WriteSchemaSnapshot.class); Mockito.when(table.getName()).thenReturn("test_table"); Mockito.doReturn(Mockito.mock(ExternalDatabase.class)).when(table).getDatabase(); Mockito.doReturn(ImmutableList.of(new Column("c1", ScalarType.createType(PrimitiveType.INT)))) - .when(table).getBaseSchema(true); + .when(writeSchema).getBaseSchema(); + Mockito.doReturn("uuid-u0/schema-1").when(writeSchema).getWriteMetadataIdentity(); + Mockito.doReturn(writeSchema).when(table).getWriteSchemaSnapshot(); LogicalPlan plan = builder.buildMergePlan(ctx, basePlan, ImmutableList.of(), table); Assertions.assertTrue(plan instanceof LogicalExternalRowLevelMergeSink); Assertions.assertFalse(((LogicalExternalRowLevelMergeSink) plan).isRequireMergeCardinalityCheck(), "UPDATE must not request the SQL MERGE cardinality validation"); + // Projection, sink metadata, and the conflict fence must share one schema generation. + Assertions.assertEquals("uuid-u0/schema-1", + ((LogicalExternalRowLevelMergeSink) plan).getBoundWriteMetadataIdentity()); + Mockito.verify(table, Mockito.times(1)).getWriteSchemaSnapshot(); } @Test diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/IcebergRowLevelDmlTransformTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/IcebergRowLevelDmlTransformTest.java index ce6acf86d2667b..c2975df5c755ac 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/IcebergRowLevelDmlTransformTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/IcebergRowLevelDmlTransformTest.java @@ -210,10 +210,14 @@ public void synthesizeDeleteOnPluginTableBuildsSinkTargetingIt() { // re-parameterization; the full plan execution is flip-e2e-gated. (Reverting the cast/param back to // IcebergExternalTable would not even compile against this plugin-typed argument.) PluginDrivenExternalTable table = Mockito.mock(PluginDrivenExternalTable.class); + PluginDrivenExternalTable.WriteSchemaSnapshot writeSchema = + Mockito.mock(PluginDrivenExternalTable.WriteSchemaSnapshot.class); ExternalDatabase database = Mockito.mock(ExternalDatabase.class); Mockito.when(table.getDatabase()).thenReturn(database); - Mockito.when(table.getBaseSchema(true)) - .thenReturn(ImmutableList.of(new Column("id", ScalarType.INT))); + // The DELETE projection and conflict fence must be bound to the same metadata generation. + Mockito.doReturn(ImmutableList.of(new Column("id", ScalarType.INT))).when(writeSchema).getBaseSchema(); + Mockito.doReturn("uuid-u0/schema-1").when(writeSchema).getWriteMetadataIdentity(); + Mockito.doReturn(writeSchema).when(table).getWriteSchemaSnapshot(); Mockito.when(table.getId()).thenReturn(TARGET_ID); LogicalPlan query = (LogicalPlan) filterOver(table, "id"); @@ -224,6 +228,9 @@ public void synthesizeDeleteOnPluginTableBuildsSinkTargetingIt() { Assertions.assertTrue(plan instanceof LogicalExternalRowLevelDeleteSink, plan.getClass().getName()); Assertions.assertSame(table, ((LogicalExternalRowLevelDeleteSink) plan).getTargetTable()); + Assertions.assertEquals("uuid-u0/schema-1", + ((LogicalExternalRowLevelDeleteSink) plan).getBoundWriteMetadataIdentity()); + Mockito.verify(table, Mockito.times(1)).getWriteSchemaSnapshot(); } @Test diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/physical/PhysicalConnectorTableSinkTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/physical/PhysicalConnectorTableSinkTest.java index 4e5e5347dd68f6..22a7b4aca04bb3 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/physical/PhysicalConnectorTableSinkTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/physical/PhysicalConnectorTableSinkTest.java @@ -101,6 +101,27 @@ public void dynamicPartitionWriteRequiresHashAndLocalSort() { "local sort must be on the partition column"); } + @Test + public void distributionUsesBindTimeSchemaAfterLiveRefresh() { + SlotReference dataSlot = new SlotReference("data", IntegerType.INSTANCE); + SlotReference partSlot = new SlotReference("part", IntegerType.INSTANCE); + PluginDrivenExternalTable table = table( + true, true, ImmutableList.of(PART), ImmutableList.of(DATA, PART)); + PhysicalConnectorTableSink sink = sink( + table, Arrays.asList(DATA, PART), ImmutableList.of(dataSlot, partSlot)); + + // Simulate a refresh after binding that reuses names at different positions. Distribution must keep + // the coherent schema generation captured by the sink instead of indexing its older output with S1. + Mockito.when(table.getPartitionColumns()).thenReturn(ImmutableList.of(DATA)); + Mockito.when(table.getFullSchema()).thenReturn(ImmutableList.of(PART, DATA)); + + PhysicalProperties props = sink.getRequirePhysicalProperties(); + DistributionSpecHiveTableSinkHashPartitioned dist = + (DistributionSpecHiveTableSinkHashPartitioned) props.getDistributionSpec(); + Assertions.assertEquals(ImmutableList.of(partSlot.getExprId()), dist.getOutputColExprIds()); + Assertions.assertEquals(partSlot, props.getOrderSpec().getOrderKeys().get(0).getExpr()); + } + /** * Pure-dynamic write with a REORDERED explicit column list ({@code INSERT INTO mc (part, data) * SELECT vpart, vdata}, schema [data, part]): the bind layer projects the child to FULL-SCHEMA @@ -324,9 +345,8 @@ private static PluginDrivenExternalTable table(boolean parallelWrite, boolean re /** * Builds a {@link PhysicalConnectorTableSink} exercising only {@code getRequirePhysicalProperties()}. - * Uses CALLS_REAL_METHODS to skip the heavyweight ctor and injects the three fields the method - * reads ({@code targetTable}, {@code cols}, and the single child via the {@code children} field, so - * the real {@code child()} resolves to it). + * Uses CALLS_REAL_METHODS to skip the heavyweight ctor and injects the bind-time schemas, target, + * columns, and single child needed by the real method. */ private static PhysicalConnectorTableSink sink(PluginDrivenExternalTable table, List cols, List childOutput) { @@ -336,6 +356,10 @@ private static PhysicalConnectorTableSink sink(PluginDrivenExternalTable t PhysicalConnectorTableSink sink = Mockito.mock(PhysicalConnectorTableSink.class, Mockito.CALLS_REAL_METHODS); Deencapsulation.setField(sink, "targetTable", table); + // Production carries both lists from one bind-time cache value; seed the real-method mock with + // that immutable view so the test also fails if the implementation re-reads the live table. + Deencapsulation.setField(sink, "boundTargetSchema", table.getFullSchema()); + Deencapsulation.setField(sink, "boundPartitionColumns", table.getPartitionColumns()); Deencapsulation.setField(sink, "cols", cols); Deencapsulation.setField(sink, "children", ImmutableList.of(child)); return sink; diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/physical/PhysicalExternalRowLevelMergeSinkTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/physical/PhysicalExternalRowLevelMergeSinkTest.java index ea06463d5b4f3e..4eb3de5168882f 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/physical/PhysicalExternalRowLevelMergeSinkTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/physical/PhysicalExternalRowLevelMergeSinkTest.java @@ -166,6 +166,20 @@ public void reconstructUnresolvedExprIdHardFailsAndClears() { Assertions.assertEquals(Integer.valueOf(7), result.partitionSpecId); } + @Test + public void reconstructRejectsSameNameWithReplacementFieldId() { + ExprId oldIdExpr = exprId("id"); + List out = new ArrayList<>(); + + InsertPartitionFieldResult result = PhysicalExternalRowLevelMergeSink.reconstructPartitionFields(out, + spec(8, field("identity", null, "id", "id", 2)), map("id", oldIdExpr), + java.util.Collections.singletonMap(1, oldIdExpr)); + + // The live field reused the name but not the bound identity; name fallback would route wrong values. + Assertions.assertFalse(result.success); + Assertions.assertTrue(out.isEmpty()); + } + @Test public void reconstructNonIdentityPrePassSeesFieldsAfterHardFail() { // PARITY-2 independence: the build loop short-circuits on field 0 (null name), but the diff --git a/fe/fe-core/src/test/java/org/apache/doris/planner/PluginDrivenTableSinkTest.java b/fe/fe-core/src/test/java/org/apache/doris/planner/PluginDrivenTableSinkTest.java index a74d87b35829b3..0ec90286b4beea 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/planner/PluginDrivenTableSinkTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/planner/PluginDrivenTableSinkTest.java @@ -106,6 +106,25 @@ public void bindDataSinkDelegatesToWritePlanProvider() throws AnalysisException Assert.assertNull(provider.seenHandle.getSortInfo()); } + @Test + public void bindDataSinkKeepsWriteSubsetSeparateFromBoundTargetSchema() throws AnalysisException { + RecordingWritePlanProvider provider = new RecordingWritePlanProvider( + new ConnectorSinkPlan(new TDataSink(TDataSinkType.ICEBERG_TABLE_SINK))); + ConnectorColumn id = Mockito.mock(ConnectorColumn.class); + ConnectorColumn name = Mockito.mock(ConnectorColumn.class); + List writeColumns = Collections.singletonList(id); + List boundTargetColumns = java.util.Arrays.asList(id, name); + + PluginDrivenTableSink sink = new PluginDrivenTableSink( + null, provider, null, new ConnectorTableHandle() { }, writeColumns, + boundTargetColumns, null, WriteOperation.INSERT, false); + sink.bindDataSink(Optional.empty()); + + Assert.assertSame(writeColumns, provider.seenHandle.getColumns()); + Assert.assertEquals(boundTargetColumns, provider.seenHandle.getBoundTargetColumns()); + Assert.assertNotSame(boundTargetColumns, provider.seenHandle.getBoundTargetColumns()); + } + @Test public void bindDataSinkThreadsEngineBuiltWriteSortInfoToHandle() throws AnalysisException { // WHY: the connector's planWrite cannot build a TSortInfo (the bound output exprs live only in the @@ -125,6 +144,20 @@ public void bindDataSinkThreadsEngineBuiltWriteSortInfoToHandle() throws Analysi Assert.assertSame(engineBuilt, provider.seenHandle.getSortInfo()); } + @Test + public void bindDataSinkThreadsBoundWriteMetadataIdentityToHandle() throws AnalysisException { + RecordingWritePlanProvider provider = new RecordingWritePlanProvider( + new ConnectorSinkPlan(new TDataSink(TDataSinkType.ICEBERG_TABLE_SINK))); + String metadataIdentity = "sort-generation-3/spec-generation-7"; + + PluginDrivenTableSink sink = new PluginDrivenTableSink( + null, provider, null, new ConnectorTableHandle() { }, new ArrayList<>(), + Collections.emptyList(), null, WriteOperation.INSERT, false, metadataIdentity); + sink.bindDataSink(Optional.empty()); + + Assert.assertEquals(metadataIdentity, provider.seenHandle.getBoundWriteMetadataIdentity()); + } + @Test public void bindDataSinkThreadsBranchNameToHandle() throws AnalysisException { // WHY: INSERT INTO t@branch carries the target branch on the PluginDrivenInsertCommandContext; diff --git a/fe/fe-type/src/main/java/org/apache/doris/catalog/Type.java b/fe/fe-type/src/main/java/org/apache/doris/catalog/Type.java index 1bf16ca7a642ee..3c8632e1a7be1d 100644 --- a/fe/fe-type/src/main/java/org/apache/doris/catalog/Type.java +++ b/fe/fe-type/src/main/java/org/apache/doris/catalog/Type.java @@ -17,6 +17,7 @@ package org.apache.doris.catalog; +import org.apache.doris.common.util.SqlUtils; import org.apache.doris.thrift.TColumnType; import org.apache.doris.thrift.TPrimitiveType; import org.apache.doris.thrift.TScalarType; @@ -458,6 +459,11 @@ public String hideVersionForVersionColumn(Boolean isToSql) { } public String hideVersionForVersionColumn(Boolean isToSql, boolean showNestedComment) { + return hideVersionForVersionColumn(isToSql, showNestedComment, false); + } + + public String hideVersionForVersionColumn( + Boolean isToSql, boolean showNestedComment, boolean noBackslashEscapes) { if (isDatetime() || isDatetimeV2()) { StringBuilder typeStr = new StringBuilder("datetime"); if (((ScalarType) this).getScalarScale() > 0) { @@ -487,13 +493,13 @@ public String hideVersionForVersionColumn(Boolean isToSql, boolean showNestedCom return typeStr.toString(); } else if (isArrayType()) { String nestedDesc = ((ArrayType) this).getItemType() - .hideVersionForVersionColumn(isToSql, showNestedComment); + .hideVersionForVersionColumn(isToSql, showNestedComment, noBackslashEscapes); return "array<" + nestedDesc + ">"; } else if (isMapType()) { String keyDesc = ((MapType) this).getKeyType() - .hideVersionForVersionColumn(isToSql, showNestedComment); + .hideVersionForVersionColumn(isToSql, showNestedComment, noBackslashEscapes); String valueDesc = ((MapType) this).getValueType() - .hideVersionForVersionColumn(isToSql, showNestedComment); + .hideVersionForVersionColumn(isToSql, showNestedComment, noBackslashEscapes); return "map<" + keyDesc + "," + valueDesc + ">"; } else if (isStructType()) { List fieldDesc = new ArrayList<>(); @@ -501,7 +507,8 @@ public String hideVersionForVersionColumn(Boolean isToSql, boolean showNestedCom for (int i = 0; i < structType.getFields().size(); i++) { StructField field = structType.getFields().get(i); StringBuilder desc = new StringBuilder(field.getName()).append(":") - .append(field.getType().hideVersionForVersionColumn(isToSql, showNestedComment)); + .append(field.getType().hideVersionForVersionColumn( + isToSql, showNestedComment, noBackslashEscapes)); // Requiredness is schema semantics and must survive independently of whether // nested documentation is requested for DESCRIBE output. if (!field.getContainsNull()) { @@ -509,7 +516,9 @@ public String hideVersionForVersionColumn(Boolean isToSql, boolean showNestedCom } // Nested docs are part of DESCRIBE output only when comments were explicitly requested. if (showNestedComment && field.isCommentSpecified()) { - desc.append(String.format(" comment '%s'", field.getComment())); + // Comments must remain parseable even when they contain quotes or backslashes. + desc.append(" comment ").append( + SqlUtils.quoteStringLiteral(field.getComment(), noBackslashEscapes)); } fieldDesc.add(desc.toString()); }