Repository navigation
feat(plugin-iceberg): Support ALTER COLUMN SET/DROP NOT NULL - #28040
Joe-Abraham wants to merge 2 commits into
Conversation
Reviewer's GuideAdds safe bidirectional NOT NULL alteration for Iceberg by validating manifest null counts across branch heads and snapshot transitions, extends engine enforcement and authorization to UPDATE and DROP NOT NULL, and provides comprehensive documentation and Java/native integration coverage. Sequence diagram for Iceberg SET NOT NULL validationsequenceDiagram
participant User
participant Presto as Presto
participant Metadata as IcebergAbstractMetadata
participant Table as IcebergTable
participant Manifests as IcebergManifests
User->>Presto: ALTER TABLE ALTER COLUMN SET NOT NULL
Presto->>Metadata: addConstraint
Metadata->>Table: updateSchema
Metadata->>Manifests: findDataFileWithNullValues
loop Each branch head
Manifests-->>Metadata: NULL counts from data files
end
alt NULL values or missing NULL count
Metadata-->>Presto: CONSTRAINT_VIOLATION
Presto-->>User: Reject schema change
else No violating files
Metadata->>Table: requireColumn
Table-->>Metadata: commit schema update
Metadata-->>Presto: Success
Presto-->>User: Column is NOT NULL
end
Sequence diagram for UPDATE enforcement on NOT NULL columnssequenceDiagram
participant User
participant Planner as QueryPlanner
participant Writer as UpdateWriter
User->>Planner: UPDATE SET not_null_column = expression
Planner->>Planner: checkNotNullColumns
Planner->>Planner: failIfNull
alt Expression evaluates to NULL
Planner-->>User: Fail with constraint error
else Non-NULL value
Planner->>Writer: Execute checked update
Writer-->>User: Update succeeds
end
Flow diagram for Iceberg snapshot and branch movesflowchart TD
A[Snapshot or branch move] --> B[Resolve target snapshot]
B --> C[verifyNoNullValuesInRequiredColumns]
C --> D[Read target data files with current schema]
D --> E{Required column can hold NULL?}
E -- Yes --> F[Reject move with CONSTRAINT_VIOLATION]
E -- No --> G[Commit rollback, branch creation, or fast-forward]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
f60b643 to
57ca3ba
Compare
1727833 to
13992f5
Compare
68d80a5 to
1c903d1
Compare
1c903d1 to
2f03f4a
Compare
2f03f4a to
c540b29
Compare
6b3d49d to
d936008
Compare
Summary: ## Why The native (Prestissimo) worker currently accepts NULLs into NOT NULL columns silently, while the Java engine rejects them in `TableWriterOperator` before any file is written — a data-integrity and Java-parity gap (prestodb/presto#28156). ## What Table writes can now enforce NOT NULL constraints on selected columns. Writing a null into a column marked NOT NULL fails with, for example: `NULL value not allowed for NOT NULL column: c1`. `InsertTableHandle` carries the list of NOT NULL column names (empty means unconstrained), so the constraint travels with the write-request descriptor and round-trips through plan serialization. `TableWriteNode` validates at construction that each name exists in the target schema. `TableWriter` maps each constrained column to its input column and checks it with `DecodedVector::hasNulls()`, which catches nulls behind dictionary or constant encoding as well as flat vectors. The check is opt-in and only scans the listed columns. Part of prestodb/presto#28156. E2E protocol changes: prestodb/presto#28040. X-link: facebookincubator/velox#18135 Reviewed By: shrinidhijoshi Differential Revision: D116681749 Pulled By: Yuhta fbshipit-source-id: dc23981cc3e24cda03185c44137e32312e3f726a
d936008 to
96723dd
Compare
Joe-Abraham
changed the title
6bac416 to
cd4ed35
Compare
steveburnett
left a comment
steveburnett
left a comment
There was a problem hiding this comment.
LGTM! (docs)
Pull branch, local doc build, looks good. Thanks!
|
@aditi-pandit @hantangwangd @nmahadevuni @imjalpreet Can you please have a look when you get some time, Thanks! |
|
Thanks @Joe-Abraham Critical
Suggestions
Nits
Questions
Testing Recommendations
|
mohsaka
left a comment
mohsaka
left a comment
There was a problem hiding this comment.
Just a nit otherwise looks good. Thanks!
09d43af to
4f853af
Compare
…b#28040) Implement addConstraint and dropConstraint in the Iceberg connector on top of Iceberg's UpdateSchema, and declare the ALTER_COLUMN capability so AlterColumnNotNullTask routes the statements to the connector. SET NOT NULL maps to requireColumn() with allowIncompatibleChanges(), and DROP NOT NULL maps to makeColumnOptional(). Only top-level columns can be altered; named constraints, branch tables, and the _row_id and _last_updated_sequence_number metadata columns are rejected. SET NOT NULL does not rewrite data files, so files written before a column became required can still hold NULLs, while Iceberg folds IS NULL on a required field to false and IS NOT NULL to true without reading data. A null-sensitive domain on a field that is required in one table schema, but optional or absent in another (the field or an enclosing struct), is therefore neither pushed into Iceberg scans and deletes nor treated as enforced by partition pruning. Columns that are NOT NULL in every schema keep full pushdown and metadata delete. Part of prestodb#28156.
4f853af to
f4f7f75
Compare
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@nmahadevuni Thanks for the detailed review. Here's how each point was addressed: Critical 1 (time travel). Fixed. Rather than tracking the schema each scan binds against at every call site, Suggestion 1 (columns created Suggestion 2 (concurrent schema change). Left as is, matching Nits
Question 1. A snapshot read uses the snapshot's schema: Testing recommendations
|
This comment was marked as off-topic.
This comment was marked as off-topic.
nmahadevuni
left a comment
nmahadevuni
left a comment
There was a problem hiding this comment.
Thanks @Joe-Abraham , looks good. Please document the behavior change for v1 tables in the description so they can go in the commit message. Please fix the merge conflict.
aditi-pandit
left a comment
aditi-pandit
left a comment
There was a problem hiding this comment.
Thanks for the thorough work here. I traced each changed path (both pushdown modes, metadata delete, rewrite_data_files, aggregation folding, transaction staging, mixed-case names) and did not find a correctness bug inside Presto. Transaction staging matches addColumn/renameColumn, mixed-case handle names line up with findField, the verify in addConstraint is a real invariant (AddConstraintTask gates UNIQUE/PK by capability), and the access-control fix is well tested. Inline comments are mostly suggestions and tests.
Main design concern: SET NOT NULL without validating existing data
Iceberg's own Spark DDL docs state: "It is not possible to change a nullable column to a non-nullable column with SET NOT NULL because Iceberg doesn't know whether there is existing data with null values." This PR protects Presto's planner, but other engines (Spark, Trino, Flink) will still bind c IS NULL on the required field to alwaysFalse and silently drop the legacy NULL rows. The docs warning acknowledges this, but the statement still succeeds unchecked.
Have you considered validating before commit instead? For example, reject SET NOT NULL if any data file in the current snapshot has null_value_counts > 0 for the column, or has no metrics for it (or fall back to a scan). That keeps required truthful for every reader, and it would let most of the null-sensitive plumbing go: isNullSensitiveOnRequiredField plus the schemas argument on all toIcebergExpression callers, the metadata-delete verify, the rewrite_data_files guard, the v1 DELETE behavior change, and the lost pruning on columns declared NOT NULL at CREATE TABLE (see the inline comment on IcebergFilterPushdown). The remaining risk would be a concurrent writer still on the old schema, which is much narrower. If metadata-only is the intended semantics (as in Hive), it would help to explain in the description why that is acceptable for Iceberg when Iceberg itself refuses it.
Other tests worth adding
count(c),min(c),max(c)on a SET NOT NULL column with legacy NULLs (aggregation folding).DELETE ... WHERE region IS NULLwithpushdown_filter_enabled=true(native), to show the metadata-delete verify is unreachable.- SET NOT NULL twice: the metadata location / snapshot does not change.
| TupleDomain<ColumnHandle> unenforcedConstraint = TupleDomain.withColumnDomains(Maps.filterKeys(constraint.getSummary().getDomains().get(), not(Predicates.in(partitionColumns)))); | ||
| Map<ColumnHandle, Domain> constraintDomains = constraint.getSummary().getDomains().get(); | ||
| Collection<Schema> schemas = icebergTable.schemas().values(); | ||
| Predicate<ColumnHandle> enforcedByPartitionPruning = column -> partitionColumns.contains(column) |
There was a problem hiding this comment.
This now applies to every NOT NULL column, including ones declared NOT NULL at CREATE TABLE that never held NULLs. WHERE pk IS NOT NULL on a NOT NULL identity partition column used to be enforced and now keeps a filter, and IS NULL used to prune to zero files and now scans everything. The release notes mention this, but these tables pay the cost without using SET NOT NULL. Another argument for validating at SET time (see review body).
| } | ||
|
|
||
| Expression filter = toIcebergExpression(predicate); | ||
| Expression filter = toIcebergExpression(predicate, table.schemas().values()); |
There was a problem hiding this comment.
Question: with pushdown_filter_enabled=true, there is no FilterNode, and the null-sensitive domain stays in getValidPredicate() while toIcebergExpression now drops it. If this rule can see that layout, count(*) WHERE region IS NULL would fold over all files and return a wrong count. Can you confirm the rule ordering makes this unreachable, or add a native test for it?
| } | ||
| // Any domain left here is on a partition column, and reaches domainPredicate only when it admits or | ||
| // excludes only NULL on a NOT NULL column, which partition pruning cannot enforce. | ||
| String unenforcedPartitionColumns = layoutHandle.getDomainPredicate() |
There was a problem hiding this comment.
Suggestion: this walks getDomainPredicate().getDomains() a second time, and relies on the comment's invariant that only partition columns are left. Collecting the root names once and branching (any non-partition column -> existing error, else non-empty -> this error) is a single pass and does not depend on the invariant.
| .orElse(""); | ||
| if (!unenforcedPartitionColumns.isEmpty()) { | ||
| throw new PrestoException(NOT_SUPPORTED, format( | ||
| "rewrite_data_files does not support IS NULL or IS NOT NULL filters on NOT NULL partition column %s when pushdown_filter_enabled=true", |
There was a problem hiding this comment.
Nit: with more than one column this reads "partition column a, b". Consider "column(s)" or pluralizing.
| if (!required && icebergTable.schema().identifierFieldIds().contains(field.get().fieldId())) { | ||
| throw new PrestoException(NOT_SUPPORTED, format("Cannot drop NOT NULL on column '%s': it is an identifier field of table '%s'", columnName, tableName)); | ||
| } | ||
| String fieldName = field.get().name(); |
There was a problem hiding this comment.
SET NOT NULL on a column that is already required still goes through updateSchema().commit(). Consider returning early when required && field.get().isRequired(), so it does not write new metadata or add a commit to the transaction. The docs say this case "does not change the schema"; a test that the metadata location stays the same would lock that in.
| * by it when the field is required in any of {@code schemas}: a scan of a snapshot other than the current | ||
| * one binds against that snapshot's schema. | ||
| */ | ||
| public static boolean isNullSensitiveOnRequiredField(Collection<Schema> schemas, String columnName, Domain domain) |
There was a problem hiding this comment.
Matching by name across historical schemas is conservative, but a dropped and re-added column with the same name gives false positives, and renames make it fragile. For top-level columns, matching by field id (IcebergColumnHandle.getId(), with schema.findField(int)) is more robust. Name matching could stay for pushed-down subfields.
| } | ||
| Expression expression = toIcebergExpression(TupleDomain.withColumnDomains( | ||
| ImmutableMap.of(LAST_UPDATED_SEQUENCE_NUMBER_COLUMN_HANDLE, domain))); | ||
| ImmutableMap.of(LAST_UPDATED_SEQUENCE_NUMBER_COLUMN_HANDLE, domain)), ImmutableList.of(LINEAGE_ONLY_SCHEMA)); |
There was a problem hiding this comment.
Nit: LAST_UPDATED_SEQUENCE_NUMBER is optional in LINEAGE_ONLY_SCHEMA, so this argument has no effect. A short comment would save readers from checking.
| assertQueryFails(rewriteAll, "NULL value not allowed for NOT NULL column: c2"); | ||
| assertHasDataFiles(loadTable(tableName).currentSnapshot(), 2); | ||
|
|
||
| assertUpdate("UPDATE " + tableName + " SET c2 = 0 WHERE c2 IS NULL", 1); |
There was a problem hiding this comment.
This covers an UPDATE that repairs the NULL. What about an UPDATE of a different column on a row with a legacy NULL, e.g. UPDATE t SET c1 = 9 WHERE c1 = 1? With merge-on-read the full row is rewritten with NULL in the NOT NULL column, so I'd expect the writer to reject it. If so, that blocks any update of those rows and is worth a test and a docs note next to the rewrite_data_files one.
| // --- helpers --- | ||
|
|
||
| @Test | ||
| public void testColumnRequiredInEverySchema() |
There was a problem hiding this comment.
Nit: the new tests land below the // --- helpers --- marker. Move them above it.
| * Dropping ``NOT NULL`` on a column that does not have the constraint fails. | ||
|
|
||
| Unlike ``SET NOT NULL``, relaxing a column from required to optional is a compatible schema | ||
| change in Iceberg, so this direction is always safe. |
There was a problem hiding this comment.
"Always safe" conflicts with the identifier-field case a few lines up, where DROP NOT NULL fails. Suggest something like "is a compatible schema change in Iceberg, so existing data stays valid".
|
@sourcery-ai review |
|
AI session: The AI reviewer completed the code review and posted the findings in the pull request review timeline. |
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="presto-main-base/src/main/java/com/facebook/presto/sql/planner/QueryPlanner.java" line_range="1050-1052" />
<code_context>
+ */
+ private PlanAndMappings checkNotNullColumns(Update node, PlanAndMappings planAndMappings, List<String> columnNames, List<Expression> columnValues, List<ColumnMetadata> updatedColumns)
+ {
+ Map<String, ColumnMetadata> notNullColumns = updatedColumns.stream()
+ .filter(column -> !column.isNullable())
+ .collect(toImmutableMap(ColumnMetadata::getName, Function.identity()));
+ if (notNullColumns.isEmpty()) {
+ return planAndMappings;
</code_context>
<issue_to_address>
**Iceberg updates miss null checks**
When the UPDATE analyzer obtains the assigned column metadata through Iceberg’s per-column lookup for a NOT NULL column, iceberg UPDATE analysis supplies column metadata through `getColumnMetadata(session, tableHandle, columnHandle)`, which builds metadata without setting nullability; `ColumnMetadata` therefore defaults to nullable. The planner filters those columns out here, so an UPDATE can still write NULL into a NOT NULL Iceberg column.
Ensure the UPDATE analysis receives Iceberg nullability from the table schema, or have the connector’s per-column metadata lookup set nullability correctly.
</issue_to_address>
### Comment 2
<location path="presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergUtil.java" line_range="663" />
<code_context>
+ // Without the field in the snapshot's schema, no data file of the snapshot has the column, so every row reads
+ // the field's initial default, or NULL if it has none.
+ if (snapshotField != null) {
+ scan = scan.filter(Expressions.isNull(snapshotField.name()));
+ }
+ Optional<DataFile> file = findDataFileWithNullValues(scan, field);
</code_context>
<issue_to_address>
**Renamed snapshots are checked incorrectly**
When a required column is renamed after the target snapshot, especially when its old name is reused, `verifyNoNullValuesInRequiredColumns` builds a filter using `snapshotField.name()`, but `newScan()` resolves it against the current schema. The filter either fails to bind and rejects a safe move, or binds to a different column and prunes files containing NULLs, so the caller sees a rejected safe move or an invalid move that succeeds.
Resolve the filter to the required field by its field ID in the current schema before scanning.
Also at `presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergUtil.java:664`.
</issue_to_address>
### Comment 3
<location path="presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java" line_range="3150" />
<code_context>
+ }
+ String fieldName = field.get().name();
+ // The NULL check runs after the update is opened, so it reads the metadata the schema change commits on.
+ UpdateSchema updateSchema = icebergTable.updateSchema();
+ if (required) {
+ verifyNoNullValues(icebergTable, field.get(), columnName, tableName);
</code_context>
<issue_to_address>
**Failed alter blocks other-table writes**
When an explicit transaction runs SET NOT NULL on a column whose data fails validation, then attempts to write to a different table before ending the transaction, `updateSchema()` registers a transaction for this table before `verifyNoNullValues` can reject the change. The failed statement leaves that table in `txByTable`, so a later write to another table in the same transaction is rejected with a multi-table transaction conflict even though the ALTER made no change.
Avoid registering the table transaction until validation succeeds, or undo the empty transaction registration when validation fails.
</issue_to_address>| Map<String, ColumnMetadata> notNullColumns = updatedColumns.stream() | ||
| .filter(column -> !column.isNullable()) | ||
| .collect(toImmutableMap(ColumnMetadata::getName, Function.identity())); |
There was a problem hiding this comment.
🟠 High · Iceberg updates miss null checks
When the UPDATE analyzer obtains the assigned column metadata through Iceberg’s per-column lookup for a NOT NULL column, iceberg UPDATE analysis supplies column metadata through getColumnMetadata(session, tableHandle, columnHandle), which builds metadata without setting nullability; ColumnMetadata therefore defaults to nullable. The planner filters those columns out here, so an UPDATE can still write NULL into a NOT NULL Iceberg column.
Ensure the UPDATE analysis receives Iceberg nullability from the table schema, or have the connector’s per-column metadata lookup set nullability correctly.
Prompt for AI agents
In `presto-main-base/src/main/java/com/facebook/presto/sql/planner/QueryPlanner.java` at lines 1050-1052:
**Iceberg updates miss null checks**
When the UPDATE analyzer obtains the assigned column metadata through Iceberg’s per-column lookup for a NOT NULL column, iceberg UPDATE analysis supplies column metadata through `getColumnMetadata(session, tableHandle, columnHandle)`, which builds metadata without setting nullability; `ColumnMetadata` therefore defaults to nullable. The planner filters those columns out here, so an UPDATE can still write NULL into a NOT NULL Iceberg column.
Ensure the UPDATE analysis receives Iceberg nullability from the table schema, or have the connector’s per-column metadata lookup set nullability correctly.| } | ||
| String fieldName = field.get().name(); | ||
| // The NULL check runs after the update is opened, so it reads the metadata the schema change commits on. | ||
| UpdateSchema updateSchema = icebergTable.updateSchema(); |
There was a problem hiding this comment.
🟡 Medium · Failed alter blocks other-table writes
When an explicit transaction runs SET NOT NULL on a column whose data fails validation, then attempts to write to a different table before ending the transaction, updateSchema() registers a transaction for this table before verifyNoNullValues can reject the change. The failed statement leaves that table in txByTable, so a later write to another table in the same transaction is rejected with a multi-table transaction conflict even though the ALTER made no change.
Avoid registering the table transaction until validation succeeds, or undo the empty transaction registration when validation fails.
Prompt for AI agents
In `presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java` at line 3150:
**Failed alter blocks other-table writes**
When an explicit transaction runs SET NOT NULL on a column whose data fails validation, then attempts to write to a different table before ending the transaction, `updateSchema()` registers a transaction for this table before `verifyNoNullValues` can reject the change. The failed statement leaves that table in `txByTable`, so a later write to another table in the same transaction is rejected with a multi-table transaction conflict even though the ALTER made no change.
Avoid registering the table transaction until validation succeeds, or undo the empty transaction registration when validation fails.
prestodb-ci
left a comment
prestodb-ci
left a comment
There was a problem hiding this comment.
No actionable findings found.
Description
Adds
ALTER TABLE ... ALTER COLUMN ... SET NOT NULLandDROP NOT NULLto the Iceberg connector.SET NOT NULLfirst checks that the column has no NULLs. It reads the NULL counts stored in the manifests at every branch head, without reading any data files. It fails if a file has NULLs in the column or has no NULL count for it. Delete files don't update those counts. So after you delete the NULL rows, runrewrite_data_fileswithrewrite-all, unless the delete removed whole files (for example, a whole partition).One exception applies on Iceberg V3. Data files written before a column was added with
DEFAULThave no NULL count for it, but they read the default. They are accepted while the table still has a snapshot from before the column was added.Branches are read with the current schema. So
rollback_to_snapshot,rollback_to_timestamp,set_current_snapshot,fast_forwardandCREATE [OR REPLACE] BRANCH ... AS OFrun the same check on the snapshot they move to.Other rules:
SET NOT NULLworks only on primitive types.DROP NOT NULLfails on a column that is already nullable (as in Hive) and on Iceberg identifier fields.This PR also fixes
DROP NOT NULLto check the drop-constraint privilege instead of the add-constraint privilege. It's an engine change, so it applies to Hive as well.hive.allow-add-constraintandhive.allow-drop-constraintare now listed in the Hive security docs.It also makes
UPDATEfail when it sets aNOT NULLcolumn toNULL. Before, onlyINSERTandMERGEchecked. On the Java path, such anUPDATEstored theNULL, and in a Parquet table the column could no longer be read. The planner now puts eachSETvalue of aNOT NULLcolumn through the checkMERGEuses, on the Java and native paths. This is an engine change too.Design change
The first version made
SET NOT NULLmetadata-only. To handle the old NULLs, it stopped trustingNOT NULLfor pruning, aggregation folding and metadata deletes. That kept Presto correct but nothing else that reads the table. Checking the data up front keepsNOT NULLtrue for every reader. All of those workarounds are gone, and reads behave exactly as they do on master.Motivation and Context
Part of #28156. Until now,
NOT NULLcould only be set inCREATE TABLE. Iceberg supports both directions, and the SPI hooks already exist. Native write enforcement landed in #28450.Impact
NOT NULLcolumn.DROP NOT NULLneeds the drop-constraint privilege on every connector.UPDATEthat sets aNOT NULLcolumn toNULLnow fails on every connector.Known gaps:
SET NOT NULLcommitted isn't checked.none) can't be madeNOT NULLonce it has data.DEFAULT, once the snapshots from before the column expire,SET NOT NULLfails untilrewrite_data_fileswithrewrite-allrewrites the older files.Test Plan
IcebergDistributedSmokeTestBase: set/drop, the NULL check (Parquet, ORC, partitions, deletes and rewrites, branches vs. tags, missing counts), branch moves, transactions (including a NULL committed after the transaction read the table) and error cases.IcebergDistributedTestBase.testUpdateNullIntoNotNullColumn:UPDATEintoNOT NULLcolumns.TestIcebergV3: columns added withDEFAULT, including rollback to a snapshot from before the column, NULLs or missing counts in later files, a write default only, and expired snapshots.TestRollbackToTimestampProcedure,TestFastForwardBranchProcedure,TestIcebergTableChangelog.TestIcebergAlterColumnNotNull: native vs. Java, and nativeUPDATEintoNOT NULLcolumns.TestHiveIntegrationSmokeTest.testAlterColumnNotNullAccessControl.Contributor checklist
Release Notes
Summary by Sourcery
Enable safe Iceberg NOT NULL constraint changes while enforcing nullability consistently across writes, snapshot operations and security checks.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: