-
Notifications
You must be signed in to change notification settings - Fork 619
[spark] Support ADD COLUMN in Spark catalog #4174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ import org.apache.fluss.exception.InvalidAlterTableException | |
| import org.apache.fluss.metadata._ | ||
| import org.apache.fluss.types.{DataTypes, RowType} | ||
|
|
||
| import org.apache.spark.SparkException | ||
| import org.apache.spark.sql.{AnalysisException, Row} | ||
| import org.apache.spark.sql.catalyst.analysis.PartitionsAlreadyExistException | ||
| import org.apache.spark.sql.connector.catalog.Identifier | ||
|
|
@@ -36,6 +37,87 @@ class SparkCatalogTest extends FlussSparkTestBase { | |
|
|
||
| protected def lakeFormat: Option[DataLakeFormat] = None | ||
|
|
||
| test("Catalog: add columns") { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. test not cover ColumnPosition, comments, nullable, please enrich test cases
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for the catch~ added. |
||
| withTable("t") { | ||
| sql("CREATE TABLE t (id int, name string)") | ||
|
|
||
| // add column without comment, default is nullable and appended at the end | ||
| sql("ALTER TABLE t ADD COLUMN age bigint") | ||
| // add column with comment | ||
| sql("ALTER TABLE t ADD COLUMN addr string COMMENT 'address of the user'") | ||
| checkAnswer( | ||
| sql("DESC t"), | ||
| Row("id", "int", null) :: | ||
| Row("name", "string", null) :: | ||
| Row("age", "bigint", null) :: | ||
| Row("addr", "string", null) :: Nil) | ||
|
|
||
| val table = admin.getTableInfo(createTablePath("t")).get() | ||
| assertThat(table.getRowType.getFieldCount).isEqualTo(4) | ||
| assertThatList(table.getRowType.getFieldNames).containsExactly("id", "name", "age", "addr") | ||
| // the comment of the added column should be persisted | ||
| assertThat(table.getSchema.getColumn("addr").getComment.get).isEqualTo("address of the user") | ||
| } | ||
| } | ||
|
|
||
| test("Catalog: add columns with unsupported position") { | ||
| withTable("t") { | ||
| sql("CREATE TABLE t (id int, name string)") | ||
|
|
||
| // only the last position is supported: FIRST/AFTER fail at the Fluss RPC serialization | ||
| // layer (ColumnPositionType only knows LAST), surfaced by Spark as a SparkException | ||
|
Comment on lines
+67
to
+68
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just throw exception in |
||
| val firstException = intercept[SparkException] { | ||
| sql("ALTER TABLE t ADD COLUMN age bigint FIRST") | ||
| } | ||
| assertThat(firstException).hasMessageContaining("Unsupported ColumnPositionType: FIRST") | ||
| val afterException = intercept[SparkException] { | ||
| sql("ALTER TABLE t ADD COLUMN age bigint AFTER id") | ||
| } | ||
| assertThat(afterException).hasMessageContaining("Unsupported ColumnPositionType: AFTER") | ||
|
|
||
| val table = admin.getTableInfo(createTablePath("t")).get() | ||
| assertThatList(table.getRowType.getFieldNames).containsExactly("id", "name") | ||
| } | ||
| } | ||
|
|
||
| test("Catalog: add non-nullable column is not supported") { | ||
| withTable("t") { | ||
| sql("CREATE TABLE t (id int, name string)") | ||
|
|
||
| // fluss only supports adding nullable columns currently, the server rejects the change | ||
| // and the IllegalArgumentException is wrapped as UnknownServerException over the RPC | ||
|
Comment on lines
+87
to
+88
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. guard this behavior in conversion instead of test case |
||
| val notNullException = intercept[ExecutionException] { | ||
| sql("ALTER TABLE t ADD COLUMN age bigint NOT NULL") | ||
| } | ||
| assertThat(notNullException).hasMessageContaining("Column age must be nullable.") | ||
|
|
||
| val table = admin.getTableInfo(createTablePath("t")).get() | ||
| assertThatList(table.getRowType.getFieldNames).containsExactly("id", "name") | ||
| } | ||
| } | ||
|
|
||
| test("Catalog: column default value is not supported") { | ||
| // Creating a table with a column default value is rejected by Spark, because Fluss does not | ||
| // implement SupportsColumnDefaultValue. | ||
| val createException = intercept[AnalysisException] { | ||
| sql("CREATE TABLE t (id int, name string DEFAULT 'abc')") | ||
| } | ||
| assertThat(createException) | ||
| .hasMessageContaining( | ||
| "Table `fluss_catalog`.`fluss`.`t` does not support column default value") | ||
|
|
||
| withTable("t") { | ||
| sql("CREATE TABLE t (id int, name string)") | ||
|
|
||
| // ALTER TABLE ADD COLUMN with a DEFAULT clause behaves differently: Spark silently drops | ||
| // the default value (the catalog does not support it) and still adds a nullable column at | ||
| // the end. | ||
| sql("ALTER TABLE t ADD COLUMN age bigint DEFAULT 18") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why this sql can success? does |
||
| val table = admin.getTableInfo(createTablePath("t")).get() | ||
| assertThatList(table.getRowType.getFieldNames).containsExactly("id", "name", "age") | ||
| } | ||
| } | ||
|
|
||
| test("Catalog: namespaces") { | ||
| // Always a default database 'fluss'. | ||
| checkAnswer(sql("SHOW DATABASES"), Row(DEFAULT_DATABASE) :: Nil) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need support spark ColumnDefaultValue here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fluss doesn't support column defaults (no field anywhere in TableChange/Schema), and since we don't implement SupportsColumnDefaultValue
I added a guard, so a future SupportsColumnDefaultValue would fail loudly rather than drop defaults.
any case you think we should handle here?