Copilot commented on code in PR #2559:
URL: https://github.com/apache/phoenix/pull/2559#discussion_r3789334374
##########
phoenix-core-client/src/main/java/org/apache/phoenix/index/IndexMaintainer.java:
##########
@@ -1770,6 +1776,16 @@ public Set<ColumnReference> getCoveredColumns() {
return coveredColumnsMap.keySet();
}
+ /**
+ * Returns the map from data table column references to their counterparts
in the maintained
+ * table. TransformMaintainer shadows {@link #coveredColumnsMap} with its
own field, so callers in
+ * code paths shared with TransformMaintainer must use this accessor rather
than reading the field
+ * directly to avoid dereferencing the uninitialized copy.
Review Comment:
This accessor fix leaves another shared transform path reading the shadowed
fields directly. `buildDeleteColumnMutation` still uses
`getIndexStorageScheme()` and `coveredColumnsMap` at lines 1565-1583; for a
`TransformMaintainer`, the inherited storage-scheme field and map are
uninitialized. `IndexRegionObserver.java:1294` invokes that method for
transform maintainers, so an `ONE_CELL_PER_COLUMN` transform can silently skip
the delete markers needed for covered columns set to null. Route that method
through virtual accessors as well (including an effective target-storage-scheme
accessor on `TransformMaintainer`).
##########
phoenix-core-client/src/main/java/org/apache/phoenix/schema/MetaDataClient.java:
##########
@@ -4947,6 +4947,22 @@ public MutationState addColumn(PTable table,
List<ColumnDef> origColumnDefs,
throw new
SQLExceptionInfo.Builder(CANNOT_TRANSFORM_TRANSACTIONAL_TABLE)
.setSchemaName(schemaName).setTableName(tableName).build().buildException();
}
+ // SINGLE_CELL_ARRAY_WITH_OFFSETS is only valid for immutable
tables. Rather than
+ // silently downgrading the requested storage scheme to
ONE_CELL_PER_COLUMN, reject the
+ // transform unless the table is already immutable or is being made
immutable in this
+ // same ALTER TABLE statement.
+ boolean willBeImmutableForScheme =
+ Boolean.TRUE.equals(metaPropertiesEvaluated.getIsImmutableRows())
+ || (metaPropertiesEvaluated.getIsImmutableRows() == null &&
table.isImmutableRows());
Review Comment:
The same-statement exception is not implemented end to end. For `ALTER
TABLE` on a mutable table that sets both `IMMUTABLE_ROWS=true` and SCAWO, this
check passes, but `TransformClient.addTransform` is called with the original
`table` at `MetaDataClient.java:5213`, and its builder hard-codes
`.setImmutableRows(table.isImmutableRows())` at `TransformClient.java:259`. The
transforming table therefore remains mutable and schema extraction can
downgrade SCAWO again. Pass the effective immutable value into the
transform-table builder, or reject this combination until it is supported; add
coverage for the combined ALTER.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
