You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/06/06 16:29:21 UTC
[GitHub] [spark] dtenedor opened a new pull request, #36778: [SPARK-39383][SQL] Support DEFAULT columns in ALTER TABLE ALTER COLUMNS to V2 data sources
dtenedor opened a new pull request, #36778:
URL: https://github.com/apache/spark/pull/36778
### What changes were proposed in this pull request?
Extend DEFAULT column support in ALTER TABLE ALTER COLUMNS commands to include V2 data sources.
(Note: this depends on https://github.com/apache/spark/pull/36771.)
Example:
```
> create or replace table t (a string default 'abc', b string) using $v2Source
> insert into t values (default)
> alter table t alter column b set default 'def'
> insert into t values ("ghi")
> Select * from t
"abc", null,
"ghi", "def"
```
### Why are the changes needed?
This makes V2 data sources easier to use and extend.
### Does this PR introduce _any_ user-facing change?
Yes.
### How was this patch tested?
This PR includes new test coverage.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang commented on a diff in pull request #36778: [SPARK-39383][SQL] Support DEFAULT columns in ALTER TABLE ALTER COLUMNS to V2 data sources
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on code in PR #36778:
URL: https://github.com/apache/spark/pull/36778#discussion_r899668759
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/types/StructType.scala:
##########
@@ -519,6 +519,7 @@ case class StructType(fields: Array[StructField]) extends DataType with Seq[Stru
private[sql] lazy val existenceDefaultValues: Array[Any] = getExistenceDefaultValues(this)
private[sql] lazy val existenceDefaultsBitmask: Array[Boolean] = getExistenceDefaultsBitmask(this)
private[sql] lazy val hasExistenceDefaultValues = existenceDefaultValues.exists(_ != null)
+ private[sql] lazy val defaultColumnAnalyzer: Analyzer = DefaultColumnAnalyzer
Review Comment:
Shall we remove the changes in this file?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #36778: [SPARK-39383][SQL] Support DEFAULT columns in ALTER TABLE ALTER COLUMNS to V2 data sources
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #36778:
URL: https://github.com/apache/spark/pull/36778#issuecomment-1148680929
Can one of the admins verify this patch?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang commented on a diff in pull request #36778: [SPARK-39383][SQL] Support DEFAULT columns in ALTER TABLE ALTER COLUMNS to V2 data sources
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on code in PR #36778:
URL: https://github.com/apache/spark/pull/36778#discussion_r905432456
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala:
##########
@@ -203,6 +203,18 @@ private[sql] object CatalogV2Util {
})
}
+ case update: UpdateColumnDefaultValue =>
+ replace(schema, update.fieldNames, field =>
+ // The new DEFAULT value string will be non-empty for any DDL commands that set the
+ // default value, such as "ALTER TABLE t ALTER COLUMN c SET DEFAULT ..." (this is
+ // enforced by the parser). On the other hand, commands that drop the default value such
+ // as "ALTER TABLE t ALTER COLUMN c DROP DEFAULT" will set this string to empty.
Review Comment:
How about testing "DROP DEFAULT" as well? I will merge this one after the new case added
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang commented on a diff in pull request #36778: [SPARK-39383][SQL] Support DEFAULT columns in ALTER TABLE ALTER COLUMNS to V2 data sources
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on code in PR #36778:
URL: https://github.com/apache/spark/pull/36778#discussion_r904530629
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala:
##########
@@ -203,6 +203,14 @@ private[sql] object CatalogV2Util {
})
}
+ case update: UpdateColumnDefaultValue =>
+ replace(schema, update.fieldNames, field =>
+ if (update.newDefaultValue().nonEmpty) {
Review Comment:
Oh wait, it seems that will be "\"\""
Then when will the default value get cleared?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala:
##########
@@ -203,6 +203,14 @@ private[sql] object CatalogV2Util {
})
}
+ case update: UpdateColumnDefaultValue =>
+ replace(schema, update.fieldNames, field =>
+ if (update.newDefaultValue().nonEmpty) {
Review Comment:
Oh wait, it seems that will be `"\"\""`
Then when will the default value get cleared?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dtenedor commented on a diff in pull request #36778: [SPARK-39383][SQL] Support DEFAULT columns in ALTER TABLE ALTER COLUMNS to V2 data sources
Posted by GitBox <gi...@apache.org>.
dtenedor commented on code in PR #36778:
URL: https://github.com/apache/spark/pull/36778#discussion_r905559251
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala:
##########
@@ -203,6 +203,18 @@ private[sql] object CatalogV2Util {
})
}
+ case update: UpdateColumnDefaultValue =>
+ replace(schema, update.fieldNames, field =>
+ // The new DEFAULT value string will be non-empty for any DDL commands that set the
+ // default value, such as "ALTER TABLE t ALTER COLUMN c SET DEFAULT ..." (this is
+ // enforced by the parser). On the other hand, commands that drop the default value such
+ // as "ALTER TABLE t ALTER COLUMN c DROP DEFAULT" will set this string to empty.
Review Comment:
Sure, good idea. This is done.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang commented on a diff in pull request #36778: [SPARK-39383][SQL] Support DEFAULT columns in ALTER TABLE ALTER COLUMNS to V2 data sources
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on code in PR #36778:
URL: https://github.com/apache/spark/pull/36778#discussion_r904530233
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala:
##########
@@ -203,6 +203,14 @@ private[sql] object CatalogV2Util {
})
}
+ case update: UpdateColumnDefaultValue =>
+ replace(schema, update.fieldNames, field =>
+ if (update.newDefaultValue().nonEmpty) {
Review Comment:
What if the new default value is an empty string? Do we just clear the default value?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang closed pull request #36778: [SPARK-39383][SQL] Support DEFAULT columns in ALTER TABLE ALTER COLUMNS to V2 data sources
Posted by GitBox <gi...@apache.org>.
gengliangwang closed pull request #36778: [SPARK-39383][SQL] Support DEFAULT columns in ALTER TABLE ALTER COLUMNS to V2 data sources
URL: https://github.com/apache/spark/pull/36778
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dtenedor commented on a diff in pull request #36778: [SPARK-39383][SQL] Support DEFAULT columns in ALTER TABLE ALTER COLUMNS to V2 data sources
Posted by GitBox <gi...@apache.org>.
dtenedor commented on code in PR #36778:
URL: https://github.com/apache/spark/pull/36778#discussion_r899727782
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/types/StructType.scala:
##########
@@ -519,6 +519,7 @@ case class StructType(fields: Array[StructField]) extends DataType with Seq[Stru
private[sql] lazy val existenceDefaultValues: Array[Any] = getExistenceDefaultValues(this)
private[sql] lazy val existenceDefaultsBitmask: Array[Boolean] = getExistenceDefaultsBitmask(this)
private[sql] lazy val hasExistenceDefaultValues = existenceDefaultValues.exists(_ != null)
+ private[sql] lazy val defaultColumnAnalyzer: Analyzer = DefaultColumnAnalyzer
Review Comment:
Sure, thanks for catching this, done.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang commented on pull request #36778: [SPARK-39383][SQL] Support DEFAULT columns in ALTER TABLE ALTER COLUMNS to V2 data sources
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on PR #36778:
URL: https://github.com/apache/spark/pull/36778#issuecomment-1165083847
Merging to master
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dtenedor commented on a diff in pull request #36778: [SPARK-39383][SQL] Support DEFAULT columns in ALTER TABLE ALTER COLUMNS to V2 data sources
Posted by GitBox <gi...@apache.org>.
dtenedor commented on code in PR #36778:
URL: https://github.com/apache/spark/pull/36778#discussion_r905247002
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala:
##########
@@ -203,6 +203,14 @@ private[sql] object CatalogV2Util {
})
}
+ case update: UpdateColumnDefaultValue =>
+ replace(schema, update.fieldNames, field =>
+ if (update.newDefaultValue().nonEmpty) {
Review Comment:
Good question. The new DEFAULT value string will be non-empty for any DDL commands that set the default value, such as "ALTER TABLE t ALTER COLUMN c SET DEFAULT ..." (this is enforced by the parser). On the other hand, commands that drop the default value such as "ALTER TABLE t ALTER COLUMN c DROP DEFAULT" will set this string to empty. I added this in a comment here to help explain.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dtenedor commented on pull request #36778: [SPARK-39383][SQL] Support DEFAULT columns in ALTER TABLE ALTER COLUMNS to V2 data sources
Posted by GitBox <gi...@apache.org>.
dtenedor commented on PR #36778:
URL: https://github.com/apache/spark/pull/36778#issuecomment-1162193536
@gengliangwang I have merged in the changes from the base branch, this PR is much simpler now.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang commented on pull request #36778: [SPARK-39383][SQL] Support DEFAULT columns in ALTER TABLE ALTER COLUMNS to V2 data sources
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on PR #36778:
URL: https://github.com/apache/spark/pull/36778#issuecomment-1163946455
LGTM except for one comment
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org