Skip to content

Commit aba25a4

Browse files
fix: report ALTER TABLE actions that delete rows as MODIFIES_DATA (#2894)
StatementFeatureVisitor classified every ALTER TABLE as schema-only, so TRUNCATE PARTITION, DROP PARTITION, EXCHANGE PARTITION and DISCARD or IMPORT TABLESPACE passed a guard checking mayModifyData(), which is the guard TRUNCATE and DROP carry MODIFIES_DATA for. They now report it as certain, REPAIR PARTITION as possible like REPAIR TABLE, and an action the grammar keeps as raw tokens (e.g. ClickHouse ALTER TABLE t DELETE WHERE ..) as possible with the "unsupported" marker.
1 parent bb93fc3 commit aba25a4

3 files changed

Lines changed: 71 additions & 2 deletions

File tree

‎src/main/java/net/sf/jsqlparser/statement/StatementFeatureVisitor.java‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@
2525
import net.sf.jsqlparser.expression.TranscodingFunction;
2626
import net.sf.jsqlparser.parser.CCJSqlParserUtil;
2727
import net.sf.jsqlparser.statement.alter.Alter;
28+
import net.sf.jsqlparser.statement.alter.AlterExpression;
29+
import net.sf.jsqlparser.statement.alter.AlterOperation;
2830
import net.sf.jsqlparser.statement.alter.AlterCollation;
2931
import net.sf.jsqlparser.statement.alter.AlterRoutine;
3032
import net.sf.jsqlparser.statement.alter.AlterTrigger;
@@ -171,6 +173,16 @@
171173
@SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.TooManyMethods", "PMD.ExcessiveImports"})
172174
public class StatementFeatureVisitor extends StatementVisitorAdapter<Void> {
173175

176+
/**
177+
* ALTER TABLE actions that delete, swap or replace the rows of a table or partition. Like
178+
* TRUNCATE and DROP they carry MODIFIES_DATA as well as MODIFIES_SCHEMA.
179+
*/
180+
private static final Set<AlterOperation> ALTER_ACTIONS_MODIFYING_DATA =
181+
EnumSet.of(AlterOperation.DROP_PARTITION, AlterOperation.TRUNCATE_PARTITION,
182+
AlterOperation.EXCHANGE_PARTITION, AlterOperation.DISCARD_PARTITION,
183+
AlterOperation.DISCARD_TABLESPACE, AlterOperation.IMPORT_PARTITION,
184+
AlterOperation.IMPORT_TABLESPACE);
185+
174186
private final Analysis analysis;
175187

176188
// ---- construction -----------------------------------------------------------------------
@@ -672,7 +684,25 @@ public <S> Void visit(PurgeStatement purgeStatement, S context) {
672684

673685
@Override
674686
public <S> Void visit(Alter alter, S context) {
675-
return schemaOnly();
687+
schemaOnly();
688+
if (alter.getAlterExpressions() == null) {
689+
return null;
690+
}
691+
for (AlterExpression action : alter.getAlterExpressions()) {
692+
AlterOperation operation = action.getOperation();
693+
if (ALTER_ACTIONS_MODIFYING_DATA.contains(operation)) {
694+
analysis.certain(StmtFeature.MODIFIES_DATA);
695+
} else if (operation == AlterOperation.REPAIR_PARTITION) {
696+
// like REPAIR TABLE, it may rewrite or discard rows
697+
analysis.possible(StmtFeature.MODIFIES_DATA);
698+
} else if (operation == AlterOperation.UNSPECIFIC) {
699+
// an action the grammar does not model is kept as raw tokens and may write
700+
// rows, e.g. ClickHouse's ALTER TABLE t DELETE WHERE ..
701+
analysis.possible(StmtFeature.MODIFIES_DATA);
702+
analysis.unresolved("unsupported");
703+
}
704+
}
705+
return null;
676706
}
677707

678708
@Override

‎src/site/sphinx/usage.rst‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1209,7 +1209,7 @@ The features
12091209
- ``SELECT``, ``DELETE .. RETURNING``, ``SHOW``, ``DESCRIBE``, ``EXPLAIN``
12101210
* - ``MODIFIES_DATA``
12111211
- rows are written or destroyed
1212-
- ``INSERT``, ``UPDATE``, ``DELETE``, ``MERGE``, ``UPSERT``, ``TRUNCATE``, ``DROP``
1212+
- ``INSERT``, ``UPDATE``, ``DELETE``, ``MERGE``, ``UPSERT``, ``TRUNCATE``, ``DROP``, ``ALTER TABLE .. TRUNCATE PARTITION``
12131213
* - ``MODIFIES_SCHEMA``
12141214
- the catalogue changes
12151215
- ``CREATE``, ``ALTER``, ``DROP``, ``TRUNCATE``, ``GRANT``, ``COMMENT``

‎src/test/java/net/sf/jsqlparser/statement/StatementFeatureVisitorTest.java‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -255,6 +255,45 @@ void destructiveDdlAlsoModifiesData() throws JSQLParserException {
255255
}
256256
}
257257

258+
/**
259+
* The ALTER TABLE actions that delete, swap or replace rows are destructive DDL in the same
260+
* sense: {@code TRUNCATE PARTITION ALL} empties a partitioned table just like TRUNCATE.
261+
*/
262+
@ParameterizedTest
263+
@ValueSource(strings = {"ALTER TABLE t TRUNCATE PARTITION ALL",
264+
"ALTER TABLE t TRUNCATE PARTITION p0", "ALTER TABLE t DROP PARTITION p0, p1",
265+
"ALTER TABLE t ADD COLUMN c INT, DROP PARTITION p0",
266+
"ALTER TABLE t EXCHANGE PARTITION p0 WITH TABLE u",
267+
"ALTER TABLE t DISCARD TABLESPACE", "ALTER TABLE t DISCARD PARTITION p0 TABLESPACE",
268+
"ALTER TABLE t IMPORT TABLESPACE", "ALTER TABLE t IMPORT PARTITION p0 TABLESPACE"})
269+
void destructiveAlterActionsAlsoModifyData(String sql) throws JSQLParserException {
270+
StatementFeatures f = analyse(sql);
271+
assertThat(f.modifiesSchema()).isTrue();
272+
assertThat(f.modifiesData()).isTrue();
273+
assertThat(f.returnsResultSet()).isFalse();
274+
}
275+
276+
/**
277+
* An ALTER TABLE action the grammar does not model, such as ClickHouse's DELETE and UPDATE
278+
* mutations, is kept as raw text, so a write cannot be excluded.
279+
*/
280+
@ParameterizedTest
281+
@ValueSource(strings = {"ALTER TABLE t DELETE WHERE id > 0",
282+
"ALTER TABLE t UPDATE c = 1 WHERE id > 0"})
283+
void unmodelledAlterActionMayModifyData(String sql) throws JSQLParserException {
284+
StatementFeatures f = analyse(sql);
285+
assertThat(f.modifiesSchema()).isTrue();
286+
assertThat(f.mayModifyData()).isTrue();
287+
assertThat(f.getUnresolvedReferences()).containsExactly("unsupported");
288+
}
289+
290+
@Test
291+
void repairPartitionMayModifyDataLikeRepairTable() throws JSQLParserException {
292+
StatementFeatures f = analyse("ALTER TABLE t REPAIR PARTITION p0");
293+
assertThat(f.modifiesData()).isFalse();
294+
assertThat(f.mayModifyData()).isTrue();
295+
}
296+
258297
// ---- the cases a flag union gets wrong --------------------------------------------------
259298

260299
@Test

0 commit comments

Comments
 (0)