Skip to content

Commit c84ad0e

Browse files
minleejaeminleejae
authored
fix: parse MySQL partition options after ALTER TABLE actions (#2865)
MySQL lets PARTITION BY or REMOVE PARTITIONING end the ALTER TABLE action list without a comma. The parser accepted them only as the first action or after a comma. After a column definition the column options took the clause, so ADD COLUMN d INT PARTITION BY HASH (id) gave one ADD action with the clause as raw column options, without an error. After a table option, ADD INDEX or DROP COLUMN it failed. A column definition now stops before these clauses, and AlterTable reads one of them after the action list as an AlterExpressionPartition. Both renderers write a space instead of a comma before it through Alter.separatorBefore, because MySQL rejects a comma there. Signed-off-by: minleejae <mmj9808@gmail.com>
1 parent b392e4a commit c84ad0e

4 files changed

Lines changed: 124 additions & 17 deletions

File tree

‎src/main/java/net/sf/jsqlparser/statement/alter/Alter.java‎

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
import java.util.ArrayList;
1313
import java.util.Collection;
1414
import java.util.Collections;
15-
import java.util.Iterator;
1615
import java.util.List;
1716
import java.util.Optional;
1817

@@ -117,21 +116,34 @@ public String toString() {
117116
StringBuilder b = new StringBuilder();
118117
appendTargetTo(b);
119118

120-
Iterator<AlterExpression> altIter = alterExpressions.iterator();
121-
122-
while (altIter.hasNext()) {
123-
b.append(altIter.next().toString());
124-
125-
// Need to append whitespace after each ADD or DROP statement
126-
// but not the last one
127-
if (altIter.hasNext()) {
128-
b.append(", ");
119+
for (int i = 0; i < alterExpressions.size(); i++) {
120+
AlterExpression action = alterExpressions.get(i);
121+
if (i > 0) {
122+
b.append(separatorBefore(action));
129123
}
124+
b.append(action);
130125
}
131126

132127
return b.toString();
133128
}
134129

130+
/**
131+
* Whether the action is MySQL's PARTITION BY or REMOVE PARTITIONING, which may end the action
132+
* list without a comma.
133+
*/
134+
public static boolean isTrailingPartitionOption(AlterExpression action) {
135+
return action != null && (action.getOperation() == AlterOperation.PARTITION_BY
136+
|| action.getOperation() == AlterOperation.REMOVE_PARTITIONING);
137+
}
138+
139+
/**
140+
* The separator written before an action that follows another: a comma, or a space before
141+
* PARTITION BY and REMOVE PARTITIONING, which MySQL does not accept after a comma.
142+
*/
143+
public static String separatorBefore(AlterExpression action) {
144+
return isTrailingPartitionOption(action) ? " " : ", ";
145+
}
146+
135147
public Alter withTable(Table table) {
136148
this.setTable(table);
137149
return this;

‎src/main/java/net/sf/jsqlparser/util/deparser/AlterDeParser.java‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
import net.sf.jsqlparser.statement.alter.AlterExpressionPrimaryKey;
1818
import net.sf.jsqlparser.statement.alter.RelationAlterAction;
1919
import net.sf.jsqlparser.statement.select.PlainSelect;
20-
import java.util.Iterator;
2120
import net.sf.jsqlparser.statement.create.table.IndexOptionSource;
2221

2322
public class AlterDeParser extends AbstractDeParser<Alter> {
@@ -35,12 +34,13 @@ public AlterDeParser(StringBuilder buffer, ExpressionVisitor<StringBuilder> expr
3534
@Override
3635
public void deParse(Alter alter) {
3736
alter.appendTargetTo(builder);
38-
for (Iterator<AlterExpression> iterator = alter.getAlterExpressions().iterator(); iterator
39-
.hasNext();) {
40-
deParseAction(iterator.next());
41-
if (iterator.hasNext()) {
42-
builder.append(", ");
37+
boolean first = true;
38+
for (AlterExpression action : alter.getAlterExpressions()) {
39+
if (!first) {
40+
builder.append(Alter.separatorBefore(action));
4341
}
42+
deParseAction(action);
43+
first = false;
4444
}
4545
}
4646

‎src/main/jjtree/net/sf/jsqlparser/parser/JSqlParserCC.jjt‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2506,6 +2506,15 @@ public class CCJSqlParser extends AbstractJSqlParser<CCJSqlParser> {
25062506
|| isKeywordAhead("MAX_ROWS") || isKeywordAhead("MIN_ROWS");
25072507
}
25082508

2509+
/**
2510+
* PARTITION BY or REMOVE PARTITIONING: the MySQL partition options that may end the ALTER TABLE
2511+
* action list without a comma. A column definition does not take them as column options.
2512+
*/
2513+
private boolean isTrailingPartitionOptionAhead() {
2514+
return getToken(1).kind == K_PARTITION && getToken(2).kind == K_BY
2515+
|| getToken(1).kind == K_REMOVE && getToken(2).kind == K_PARTITIONING;
2516+
}
2517+
25092518
private boolean isMySqlTableOptionAction(AlterExpression action) {
25102519
return action instanceof AlterExpressionTableOption
25112520
&& ((AlterExpressionTableOption) action).getStructuredTableOption() != null
@@ -20227,7 +20236,8 @@ AlterExpression.ColumnDataType AlterExpressionColumnDataType():
2022720236
useSetData=TypeChangePrefix() { withType = true; } ]
2022820237
( LOOKAHEAD(2) dataType = ColDataType() )?
2022920238
( LOOKAHEAD(2, { !(withType && getToken(1).kind == K_USING)
20230-
&& getToken(1).kind != K_FIRST && !isKeywordAhead("AFTER") })
20239+
&& getToken(1).kind != K_FIRST && !isKeywordAhead("AFTER")
20240+
&& !isTrailingPartitionOptionAhead() })
2023120241
option = ColumnDefinitionOption() {
2023220242
columnOptions.add(option);
2023320243
} )*
@@ -22269,6 +22279,16 @@ Alter AlterTable(boolean foreign):
2226922279
alterExp=AlterSharedTableOption() { alter.addAlterExpression(alterExp); }
2227022280
)*
2227122281

22282+
// MySQL partition_options end the action list without a comma
22283+
[
22284+
LOOKAHEAD({ isTrailingPartitionOptionAhead() })
22285+
{
22286+
requireDdlSyntax(!Alter.isTrailingPartitionOption(alterExp),
22287+
"PARTITION BY and REMOVE PARTITIONING cannot follow each other");
22288+
}
22289+
alterExp=AlterExpressionPartitionOp() { alter.addAlterExpression(alterExp); }
22290+
]
22291+
2227222292
{
2227322293
for (AlterExpression action : alter.getAlterExpressions()) {
2227422294
requireDdlSyntax(foreign || !(action instanceof AlterForeignDataOptions), "OPTIONS requires ALTER FOREIGN TABLE");

‎src/test/java/net/sf/jsqlparser/statement/alter/MySQLAlterTablePartitionTest.java‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,16 +19,20 @@
1919
import static org.junit.jupiter.api.Assertions.assertTrue;
2020

2121
import java.util.List;
22+
import java.util.function.Consumer;
23+
import java.util.stream.Collectors;
2224
import java.util.stream.Stream;
2325
import net.sf.jsqlparser.JSQLParserException;
2426
import net.sf.jsqlparser.parser.AbstractJSqlParser.Dialect;
27+
import net.sf.jsqlparser.parser.CCJSqlParser;
2528
import net.sf.jsqlparser.parser.CCJSqlParserUtil;
2629
import net.sf.jsqlparser.schema.Table;
2730
import net.sf.jsqlparser.statement.TableMaintenanceStatement.Modifier;
2831
import net.sf.jsqlparser.statement.create.table.PartitionDefinition;
2932
import net.sf.jsqlparser.statement.create.table.TablePartitioning;
3033
import org.junit.jupiter.api.Test;
3134
import org.junit.jupiter.params.ParameterizedTest;
35+
import org.junit.jupiter.params.provider.Arguments;
3236
import org.junit.jupiter.params.provider.MethodSource;
3337

3438
/** MySQL 8.4 {@code ALTER TABLE} partition syntax verified against the MySQL manual. */
@@ -263,6 +267,77 @@ public void testPartitionByAst() throws JSQLParserException {
263267
assertEquals(partitioning.getPartitionDefinitions(), expression.getPartitionDefinitions());
264268
}
265269

270+
@ParameterizedTest
271+
@MethodSource("partitionOptionsAfterActions")
272+
public void testPartitionOptionsEndTheActionListWithoutComma(String sql,
273+
List<AlterOperation> operations) throws JSQLParserException {
274+
// with the MySQL dialect and without a dialect
275+
for (Consumer<CCJSqlParser> mode : List.<Consumer<CCJSqlParser>>of(
276+
parser -> parser.withDialect(Dialect.MYSQL), parser -> {
277+
})) {
278+
Alter alter =
279+
assertInstanceOf(Alter.class, assertSqlCanBeParsedAndDeparsed(sql, true, mode));
280+
281+
assertEquals(operations, alter.getAlterExpressions().stream()
282+
.map(AlterExpression::getOperation).collect(Collectors.toList()));
283+
}
284+
}
285+
286+
static Stream<Arguments> partitionOptionsAfterActions() {
287+
return Stream.of(
288+
Arguments.of("ALTER TABLE t1 ADD COLUMN d INT PARTITION BY HASH (id) PARTITIONS 4",
289+
List.of(AlterOperation.ADD, AlterOperation.PARTITION_BY)),
290+
Arguments.of("ALTER TABLE t1 ENGINE = InnoDB REMOVE PARTITIONING",
291+
List.of(AlterOperation.ENGINE, AlterOperation.REMOVE_PARTITIONING)),
292+
Arguments.of("ALTER TABLE t1 ENGINE = InnoDB PARTITION BY KEY (id) PARTITIONS 2",
293+
List.of(AlterOperation.ENGINE, AlterOperation.PARTITION_BY)),
294+
Arguments.of("ALTER TABLE t1 ALGORITHM = INPLACE, ADD COLUMN x INT NOT NULL "
295+
+ "DEFAULT 0 PARTITION BY KEY (id) PARTITIONS 2",
296+
List.of(AlterOperation.ALGORITHM, AlterOperation.ADD,
297+
AlterOperation.PARTITION_BY)),
298+
Arguments.of("ALTER TABLE t1 MODIFY COLUMN d BIGINT REMOVE PARTITIONING",
299+
List.of(AlterOperation.MODIFY, AlterOperation.REMOVE_PARTITIONING)),
300+
Arguments.of(
301+
"ALTER TABLE t1 CHANGE COLUMN d e INT AFTER id PARTITION BY RANGE (id) "
302+
+ "(PARTITION p0 VALUES LESS THAN (10), "
303+
+ "PARTITION p1 VALUES LESS THAN MAXVALUE)",
304+
List.of(AlterOperation.CHANGE, AlterOperation.PARTITION_BY)),
305+
Arguments.of("ALTER TABLE t1 ADD INDEX i (a) PARTITION BY HASH (id)",
306+
List.of(AlterOperation.ADD, AlterOperation.PARTITION_BY)),
307+
Arguments.of("ALTER TABLE t1 DROP COLUMN c REMOVE PARTITIONING",
308+
List.of(AlterOperation.DROP, AlterOperation.REMOVE_PARTITIONING)));
309+
}
310+
311+
@Test
312+
public void testColumnDefinitionStopsBeforePartitionOptions() throws JSQLParserException {
313+
Alter alter = (Alter) CCJSqlParserUtil.parse(
314+
"ALTER TABLE t1 ADD COLUMN d INT PARTITION BY HASH (id) PARTITIONS 4",
315+
parser -> parser.withDialect(Dialect.MYSQL));
316+
317+
AlterExpression add = alter.getAlterExpressions().get(0);
318+
assertEquals("d INT", add.getColDataTypeList().get(0).toString());
319+
assertTrue(add.getColDataTypeList().get(0).getColumnOptions().isEmpty());
320+
AlterExpressionPartition partitionBy =
321+
assertInstanceOf(AlterExpressionPartition.class,
322+
alter.getAlterExpressions().get(1));
323+
assertEquals(TablePartitioning.Type.HASH, partitionBy.getPartitioning().getType());
324+
assertEquals(4L, partitionBy.getPartitioning().getPartitions());
325+
}
326+
327+
@Test
328+
public void testPartitionOptionsAreNotPrecededByComma() throws JSQLParserException {
329+
// MySQL rejects a comma before PARTITION BY; the rendering drops it
330+
Alter alter = (Alter) CCJSqlParserUtil.parse(
331+
"ALTER TABLE t1 ADD COLUMN d INT, PARTITION BY HASH (id)");
332+
assertEquals("ALTER TABLE t1 ADD COLUMN d INT PARTITION BY HASH (id)", alter.toString());
333+
assertSqlCanBeParsedAndDeparsed(alter.toString(), true);
334+
assertEquals(" ", Alter.separatorBefore(alter.getAlterExpressions().get(1)));
335+
assertEquals(", ", Alter.separatorBefore(alter.getAlterExpressions().get(0)));
336+
assertThrows(JSQLParserException.class, () -> CCJSqlParserUtil.parse(
337+
"ALTER TABLE t1 PARTITION BY HASH (id) REMOVE PARTITIONING",
338+
parser -> parser.withDialect(Dialect.MYSQL)));
339+
}
340+
266341
@Test
267342
public void testProgrammaticConstruction() {
268343
AlterExpressionPartition expression = new AlterExpressionPartition()

0 commit comments

Comments
 (0)