Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -626,7 +626,8 @@ public ConnectorTableSchema getTableSchema(
ConnectorCapability.SUPPORTS_NESTED_COLUMN_PRUNE,
ConnectorCapability.SUPPORTS_FIELD_ID_ACCESS_PATH,
ConnectorCapability.SUPPORTS_STORAGE_PREDICATE_PRUNING,
ConnectorCapability.SUPPORTS_NESTED_COLUMN_SCHEMA_CHANGE));
ConnectorCapability.SUPPORTS_NESTED_COLUMN_SCHEMA_CHANGE,
ConnectorCapability.SUPPORTS_WRITE_ONLY_DEFAULTS));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Reflect this capability for snapshot schemas as well

The two-argument getTableSchema reflects this set, but the snapshot overload still returns the sibling schema directly. For SELECT DEFAULT(c) FROM hms_catalog.db.iceberg_t FOR VERSION AS OF <snapshot_id>, PluginDrivenMvccExternalTable.loadSnapshot publishes that unreflected historical schema. tableCapabilities() reads the ambient pinned schema, while the Hive catalog itself does not declare write-only defaults, so hasWriteOnlyDefaults() is false and the bound rewrite returns NULL for a nullable column instead of rejecting this read-only use. Direct Iceberg catalogs retain the guard through their connector-wide capability. Apply the same reflection to getTableSchema(session, handle, snapshot) and test that overload.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in this PR. HiveConnectorMetadata#getTableSchema(session, handle, snapshot) returns the sibling's schema without reflectSiblingCapabilities for every inherited capability, not only this one (auto-analyze, Top-N lazy materialization, nested column prune, field-id access path, storage predicate pruning, nested column DDL); that predates this PR. So for a time-travel read of an iceberg-on-HMS table, SELECT DEFAULT(c) ... FOR VERSION AS OF keeps master's current answer (NULL) instead of the new error; nothing is written. Reflecting capabilities on the snapshot path changes scan planning for all of them and should come with its own tests, so it is left to a separate change.


/**
* Reflects the {@link #SIBLING_INHERITABLE_CAPABILITIES} subset of the owning sibling connector's
Expand All @@ -636,7 +637,7 @@ public ConnectorTableSchema getTableSchema(
* (hive) connector-wide set OR the table's own set, and NEVER consults the sibling connector directly, so
* without this reflection an iceberg-on-HMS table would silently lose every such capability the iceberg
* sibling declares connector-wide (auto-analyze / Top-N lazy / nested-column prune / storage predicate
* pruning / nested column DDL).
* pruning / nested column DDL / write-only defaults).
* Returns the sibling schema unchanged when nothing is inherited and the sibling declared nothing itself
* (e.g. a hudi sibling, which declares no capabilities at all).
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -525,14 +525,18 @@ public void foreignHandleSchemaReflectsSiblingScanCapabilitiesAsPerTableMarker()
ConnectorCapability.SUPPORTS_TOPN_LAZY_MATERIALIZE,
ConnectorCapability.SUPPORTS_NESTED_COLUMN_PRUNE,
ConnectorCapability.SUPPORTS_FIELD_ID_ACCESS_PATH,
ConnectorCapability.SUPPORTS_STORAGE_PREDICATE_PRUNING);
ConnectorCapability.SUPPORTS_STORAGE_PREDICATE_PRUNING,
ConnectorCapability.SUPPORTS_WRITE_ONLY_DEFAULTS);
HiveConnectorMetadata md = new HiveConnectorMetadata(null, HiveTestProperties.minimal(), new FakeConnectorContext(),
SUPPLIER_MUST_NOT_BE_USED, SUPPLIER_MUST_NOT_BE_USED,
handle -> new SiblingOwner(new CapabilityDeclaringSiblingConnector(siblingCaps),
SiblingOwner.ICEBERG_LABEL));

ConnectorTableSchema schema = md.getTableSchema(session, foreignHandle);
Set<ConnectorCapability> reflected = schema.getTableCapabilities();
Assertions.assertTrue(reflected.contains(ConnectorCapability.SUPPORTS_WRITE_ONLY_DEFAULTS),
"an iceberg-on-HMS table must inherit write-only defaults, or DEFAULT(column) on it answers "
+ "NULL from the read schema");
Assertions.assertTrue(reflected.contains(ConnectorCapability.SUPPORTS_COLUMN_AUTO_ANALYZE),
"auto-analyze must survive the delegation as a per-table capability");
Assertions.assertTrue(reflected.contains(ConnectorCapability.SUPPORTS_TOPN_LAZY_MATERIALIZE),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -911,6 +911,10 @@ public Set<ConnectorCapability> getCapabilities() {
// (CREATE TABLE ... ORDER BY (...)). fe-core admits the ORDER BY clause for a plugin-driven CREATE TABLE
// ONLY under this capability (replacing the legacy engine-name "iceberg" gate); the sort-column validation
// (existence / type / duplicates) runs connector-side in IcebergConnectorMetadata.createTable.
// SUPPORTS_WRITE_ONLY_DEFAULTS: an iceberg write-default applies to new rows only, so it lives in the
// writer schema (IcebergWriteSchemaContext) and not in the read schema. fe-core resolves DEFAULT(column)
// against the writer schema pinned for the write target and rejects it in a plain SELECT, instead of
// answering NULL from the read schema.
EnumSet<ConnectorCapability> capabilities = EnumSet.of(ConnectorCapability.SUPPORTS_MVCC_SNAPSHOT,
ConnectorCapability.SUPPORTS_COLUMN_AUTO_ANALYZE,
ConnectorCapability.SUPPORTS_TOPN_LAZY_MATERIALIZE,
Expand All @@ -921,7 +925,8 @@ public Set<ConnectorCapability> getCapabilities() {
ConnectorCapability.SUPPORTS_STORAGE_PREDICATE_PRUNING,
ConnectorCapability.SUPPORTS_METADATA_PRELOAD,
ConnectorCapability.SUPPORTS_SORT_ORDER,
ConnectorCapability.SUPPORTS_NESTED_COLUMN_SCHEMA_CHANGE);
ConnectorCapability.SUPPORTS_NESTED_COLUMN_SCHEMA_CHANGE,
ConnectorCapability.SUPPORTS_WRITE_ONLY_DEFAULTS);
// SUPPORTS_USER_SESSION: only a REST catalog configured iceberg.rest.session=user projects the querying
// user's delegated credential onto a per-request Iceberg REST SessionCatalog (#63068 re-migration). This
// gates FE credential injection + shared-cache bypass; every other flavor/config authenticates with a
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,18 @@ public void declaresColumnAutoAnalyzeAndTopNLazyMaterializeCapabilities() {
+ "materialization");
}

@Test
public void declaresWriteOnlyDefaultsCapability() {
// WHY: an iceberg write-default is carried by the writer schema only, so the read schema's columns have no
// default. fe-core resolves DEFAULT(column) against the pinned writer schema (and rejects it in a plain
// SELECT) ONLY under this capability. MUTATION: dropping the capability -> INSERT ... SELECT DEFAULT(c)
// writes NULL and UPDATE ... WHERE c = DEFAULT(c) silently matches nothing -> red.
IcebergConnector connector = new IcebergConnector(Collections.emptyMap(), new RecordingConnectorContext());
Assertions.assertTrue(connector.getCapabilities()
.contains(ConnectorCapability.SUPPORTS_WRITE_ONLY_DEFAULTS),
"iceberg must declare SUPPORTS_WRITE_ONLY_DEFAULTS so DEFAULT(column) resolves to the write default");
}

@Test
public void declaresShowCreateDdlCapability() {
// WHY: legacy IcebergExternalTable rendered LOCATION + PROPERTIES + PARTITION BY + ORDER BY in SHOW
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -304,5 +304,20 @@ public enum ConnectorCapability {
* data tables while declining it on the subset of system tables whose readers cannot observe a
* selected snapshot.</p>
*/
SUPPORTS_SCAN_PARAM_OPTIONS
SUPPORTS_SCAN_PARAM_OPTIONS,
/**
* Indicates the connector's column defaults apply to writes only: they are carried by the
* request-scoped writer schema ({@code ConnectorWritePlanProvider.getWriteColumns}), not by the read
* schema (e.g. Iceberg {@code write-default}).
*
* <p>fe-core then resolves {@code DEFAULT(column)} against the writer schema a statement pinned for its
* write target: in INSERT VALUES, by name against the target in INSERT ... SELECT, and in the
* predicates of UPDATE / MERGE on the target. Anywhere else, e.g. in a plain SELECT, {@code
* DEFAULT(column)} on such a table is rejected instead of answering NULL from the read schema.
* Connectors whose read schema carries the column defaults (hive) must NOT declare it.</p>
*
* <p><b>Scope: catalog-wide OR per-table.</b> An iceberg-on-HMS table inherits it through the per-table
* set the gateway reflects from its sibling, exactly like {@link #SUPPORTS_NESTED_COLUMN_PRUNE}.</p>
*/
SUPPORTS_WRITE_ONLY_DEFAULTS
}
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,9 @@ public void connectorApiMajorTracksTheRecordedSurfaceChange() throws IOException
version.load(in);
}
// Major 20 adds deleteUsingMetadata to the connector write surface; major 21 adds
// typed filesystem credential descriptions to that surface.
Assertions.assertEquals("21.0", version.getProperty("api.version"));
// typed filesystem credential descriptions to that surface; major 22 adds the
// ConnectorCapability#SUPPORTS_WRITE_ONLY_DEFAULTS constant.
Assertions.assertEquals("22.0", version.getProperty("api.version"));
}

/** Root entry points plus provider/handle types returned to connector plugins. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ org.apache.doris.connector.spi.ConnectorCapability#enum:SUPPORTS_SYS_TABLE_NESTE
org.apache.doris.connector.spi.ConnectorCapability#enum:SUPPORTS_TOPN_LAZY_MATERIALIZE
org.apache.doris.connector.spi.ConnectorCapability#enum:SUPPORTS_USER_SESSION
org.apache.doris.connector.spi.ConnectorCapability#enum:SUPPORTS_VIEW
org.apache.doris.connector.spi.ConnectorCapability#enum:SUPPORTS_WRITE_ONLY_DEFAULTS
org.apache.doris.connector.spi.ConnectorColumn#equals(java.lang.Object):boolean
org.apache.doris.connector.spi.ConnectorColumn#getComment():java.lang.String
org.apache.doris.connector.spi.ConnectorColumn#getDefaultValue():java.lang.String
Expand Down
2 changes: 1 addition & 1 deletion fe/fe-connector/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ under the License.
of the latter two means bumping this property as well (and fe-extension-spi means bumping
all five families).
-->
<connector.plugin.api.version>21.0</connector.plugin.api.version>
<connector.plugin.api.version>22.0</connector.plugin.api.version>
</properties>

<modules>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,17 @@ public boolean supportsSampleAnalyze() {
return hasCapability(ConnectorCapability.SUPPORTS_SAMPLE_ANALYZE);
}

/**
* Returns whether THIS table's column defaults apply to writes only, i.e. only the request-scoped writer
* schema ({@link #resolveWriteColumns}) carries them. {@code InsertUtils} and {@code RewriteDefaultExpression}
* then resolve {@code DEFAULT(column)} against the writer schema a write statement pinned, and reject it
* anywhere else instead of answering NULL from the read schema. Resolved per-table via {@link #hasCapability}
* so an iceberg-on-HMS table inherits it, mirroring {@link #supportsNestedColumnSchemaChange}.
*/
public boolean hasWriteOnlyDefaults() {
return hasCapability(ConnectorCapability.SUPPORTS_WRITE_ONLY_DEFAULTS);
}

/**
* Whether this table supports a table-scoped capability, resolved connector-wide OR per-table. A
* uniform-format connector (iceberg — every table orc/parquet) declares the capability for all its tables
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@
package org.apache.doris.nereids.rules.expression.rules;

import org.apache.doris.catalog.Column;
import org.apache.doris.catalog.TableIf;
import org.apache.doris.datasource.plugin.PluginDrivenExternalTable;
import org.apache.doris.nereids.CascadesContext;
import org.apache.doris.nereids.StatementContext;
import org.apache.doris.nereids.analyzer.UnboundFunction;
import org.apache.doris.nereids.exceptions.AnalysisException;
import org.apache.doris.nereids.parser.NereidsParser;
Expand Down Expand Up @@ -46,6 +49,8 @@
* 1. DEFAULT(column) with default value -> the actual default value expression
* 2. DEFAULT(column) with no default value but nullable -> NULL
* 3. DEFAULT(column) with no default value and not nullable -> error
* A table whose defaults apply to writes only takes them from the writer schema that the statement
* pinned for its write target; DEFAULT(column) on any other such table is an error.
*/
public class RewriteDefaultExpression implements ExpressionPatternRuleFactory {

Expand Down Expand Up @@ -82,6 +87,24 @@ private static Expression rewrite(ExpressionMatchingContext<Default> context) {
+ column.getName() + "'");
}

Optional<TableIf> originalTable = slotRef.getOriginalTable();
if (originalTable.filter(PluginDrivenExternalTable.class::isInstance)
.filter(table -> ((PluginDrivenExternalTable) table).hasWriteOnlyDefaults())
.isPresent()) {
column = pinnedWriteColumn(context.cascadesContext.getStatementContext(), originalTable.get(), column)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Derive the result type after selecting the writer column

targetType is computed from the read column before this substitution and remains unchanged afterward. The read schema can still be cached as INT after an external Iceberg promotion to LONG, while pinAndGet independently loads current LONG writer columns and a new write-default such as 4900000001. Then UPDATE t SET other = 'x' WHERE DEFAULT(score) = 4900000001 produces Filter(CAST(4900000001 AS INT) = 4900000001) above the cached INT scan, instead of using the BIGINT writer default, causing overflow/error or an incorrect predicate. The writer fences compare the current writer columns/identity and do not validate this stale predicate type. Compute the type from the selected writer column and add a same-field-ID read-INT/write-BIGINT test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in cf75568. RewriteDefaultExpression now takes the result type from the pinned writer column, after the substitution. Test: RewriteDefaultExpressionTest#writeOnlyDefaultTakesTheTypeOfThePinnedWriterColumn pins a BIGINT writer column with default 4900000001 over an INT read column with the same field id and expects a BIGINT literal; with the previous commit the result is cast(4900000001 as INT).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up: superseded in 688e8d5 (see the newer thread on this file). Typing the substituted default as the writer column fixed the DEFAULT node itself but not the expressions bound around it against the read type, so a writer column whose type differs from the read column now fails with a refresh hint instead.

.orElseThrow(() -> new AnalysisException("DEFAULT(" + slotRef.toSql()
+ ") must reference a column of the table that this statement writes"));
// The expressions around DEFAULT(column) were bound against the read column, so a default of another type
// cannot stand in for it. The types differ when the table changed outside Doris after its read schema was
// cached, e.g. by a type promotion.
DataType writeType = DataType.fromCatalogType(column.getType());
if (!writeType.equals(targetType)) {
throw new AnalysisException("DEFAULT(" + slotRef.toSql() + ") reads the column as "
+ targetType.toSql() + " but the table now writes it as " + writeType.toSql()
+ "; the table changed outside Doris, refresh it and retry");
}
}

String defaultValueSql = column.getDefaultValueSql();
if (defaultValueSql == null) {
if (column.isAllowNull()) {
Expand All @@ -101,4 +124,12 @@ private static Expression rewrite(ExpressionMatchingContext<Default> context) {

return TypeCoercionUtils.castIfNotSameType(defaultValueExpr, targetType);
}

private static Optional<Column> pinnedWriteColumn(StatementContext statementContext, TableIf table,
Column readColumn) {
return statementContext.getConnectorWriteSchema(table.getId())
.flatMap(columns -> columns.stream()
.filter(column -> column.getUniqueId() == readColumn.getUniqueId())
.findFirst());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
import org.apache.doris.nereids.trees.expressions.DefaultValueSlot;
import org.apache.doris.nereids.trees.expressions.Expression;
import org.apache.doris.nereids.trees.expressions.SlotReference;
import org.apache.doris.nereids.types.DataType;
import org.apache.doris.nereids.util.TypeCoercionUtils;
import org.apache.doris.qe.ConnectContext;

import com.google.common.collect.ImmutableList;
Expand Down Expand Up @@ -101,7 +103,9 @@ public static Expression resolveDefaultReferences(Expression expression, List<Co
throw new AnalysisException("Cannot find column information for DEFAULT("
+ reference.toSql() + ")");
}
return resolveDefault(column);
// The parsed default takes the literal's own type (-1 is a TINYINT); the expression around DEFAULT(column)
// must bind against the column's type, as the bound DEFAULT rewrite does.
return TypeCoercionUtils.castUnbound(resolveDefault(column), DataType.fromCatalogType(column.getType()));
});
}

Expand Down
Loading
Loading