[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2105: IGNITE-19407 Sql. Introduce DDL command for ALTER COLUMN TYPE.

[via GitHub]

AMashenkov commented on code in PR #2105:
URL: https://github.com/apache/ignite-3/pull/2105#discussion_r1211547232


##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/sql/SqlAlterColumnDdlParserTest.java:
##########
@@ -44,69 +46,92 @@ public class SqlAlterColumnDdlParserTest extends 
AbstractDdlParserTest {
     /**
      * Verifies parsing of {@code ALTER TABLE ... ALTER COLUMN ... SET/DROP 
NOT NULL} statement.
      *
-     * <p>The parser is expected to produce a node of {@link 
IgniteSqlAlterColumnNotNull} class with the specified table name and the
+     * <p>The parser is expected to produce a node of {@link 
IgniteSqlAlterColumn} class with the specified table name and the
      * column name.
-     * For the {@code SET NOT NULL} statement, {@link 
IgniteSqlAlterColumnNotNull#notNull()} must return {@code true}.
-     * For the {@code DROP NOT NULL} statement, {@link 
IgniteSqlAlterColumnNotNull#notNull()} must return {@code false}.
+     * For the {@code SET NOT NULL} statement, {@link 
IgniteSqlAlterColumn#notNull()} must return {@code true}.
+     * For the {@code DROP NOT NULL} statement, {@link 
IgniteSqlAlterColumn#notNull()} must return {@code false}.
      */
     @Test
     public void testNotNull() {
-        Class<IgniteSqlAlterColumnNotNull> expCls = 
IgniteSqlAlterColumnNotNull.class;
-
-        assertThat(parseAlterColumn("SET NOT NULL", expCls).notNull(), 
is(true));
-        assertThat(parseAlterColumn("DROP NOT NULL", expCls).notNull(), 
is(false));
+        assertThat(parseAlterColumn("SET NOT NULL").notNull(), is(true));
+        assertThat(parseAlterColumn("DROP NOT NULL").notNull(), is(false));
     }
 
     /**
      * Verifies parsing of {@code ALTER TABLE ... ALTER COLUMN ... SET/DROP 
DEFAULT} statement.
      *
-     * <p>The parser is expected to produce a node of {@link 
IgniteSqlAlterColumnDefault} class with the specified table name and the
+     * <p>The parser is expected to produce a node of {@link 
IgniteSqlAlterColumn} class with the specified table name and the
      * column name.
-     * For {@code SET DEFAULT 'EXPRESSION'}, {@link 
IgniteSqlAlterColumnDefault#expression()} must return expected default 
expression.
-     * For {@code DROP DEFAULT}, {@link 
IgniteSqlAlterColumnDefault#expression()} must return {@code null}.
+     * <ul>
+     *     <li>Command {@code DROP DEFAULT} must be equivalent to {@code SET 
DEFAULT NULL}, and {@link IgniteSqlAlterColumn#expression()}
+     *         in this case must contain SQL literal with type NULL.</li>
+     *     <li>For {@code SET DEFAULT &lt;LITERAL&gt;} {@link 
IgniteSqlAlterColumn#expression()} must contain expected SQL literal.</li>
+     *     <li>For {@code SET DEFAULT &lt;ID&gt;} parser should throw an 
exception.</li>
+     * </ul>
      */
     @Test
     public void testDefault() {
-        Class<IgniteSqlAlterColumnDefault> expCls = 
IgniteSqlAlterColumnDefault.class;
-
-        assertNull(parseAlterColumn("DROP DEFAULT", expCls).expression());
+        checkDefaultIsNull(parseAlterColumn("DROP DEFAULT").expression());
+        checkDefaultIsNull(parseAlterColumn("SET DEFAULT NULL", "DROP 
DEFAULT").expression());
 
-        SqlNode dflt = parseAlterColumn("SET DEFAULT 10", expCls).expression();
+        SqlNode dflt = parseAlterColumn("SET DEFAULT 10").expression();
         assertThat(dflt, instanceOf(SqlLiteral.class));
         assertThat(((SqlLiteral) dflt).getValueAs(Integer.class), equalTo(10));
+
+        assertThrows(SqlException.class, () -> parse(QUERY_PREFIX + "SET 
DEFAULT FUNC"));
     }
 
     /**
      * Verifies parsing of {@code ALTER TABLE ... ALTER COLUMN ... SET DATA 
TYPE} statement.
      *
-     * <p>The parser is expected to produce a node of {@link 
IgniteSqlAlterColumnType} class with the specified {@link
-     * IgniteSqlAlterColumnType#name() table name}, {@link 
IgniteSqlAlterColumnType#columnName() column name}, column {@link
-     * IgniteSqlAlterColumnType#dataType() data type} and an optional {@link 
IgniteSqlAlterColumnType#expression() default expression}.
+     * <p>Parser must support the following syntax {@code SET DATA TYPE 
&lt;new_type&gt; [NOT NULL | NULL] [DEFAULT &lt;default value&gt;]}.

Review Comment:
   `SET DATA TYPE INTEGER NULL`
   NULL here looks ambiguous.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org