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/07/22 23:25:31 UTC
[GitHub] [spark] dtenedor opened a new pull request, #37256: [SPARK-39844][SQL] Restrict adding DEFAULT columns for existing tables to allowlist of supported data source types
dtenedor opened a new pull request, #37256:
URL: https://github.com/apache/spark/pull/37256
### What changes were proposed in this pull request?
Restrict adding DEFAULT columns for existing tables to allowlist of supported data source types.
### Why are the changes needed?
When `ALTER TABLE ... ADD COLUMNS` commands assign column DEFAULT values, Spark constant-folds these values and stores the result in the `EXISTS_DEFAULT` column metadata. This allows the target data source to substitute this value for rows where the corresponding field is not present in storage. This responsibility is up to each data source.
In order to grant flexibility to certain data sources that are not yet ready to support this functionality, in this PR we add the new SQLConf `ADD_DEFAULT_COLUMN_EXISTING_TABLE_BANNED_PROVIDERS` as `spark.sql.defaultColumn.addColumnExistingTableBannedProviders`. Adding a table provider to this list as well as `DEFAULT_COLUMN_ALLOWED_PROVIDERS` will allow Spark to assign default column values only upon either (1) initial table creation or (2) later with `ALTER TABLE ... SET DEFAULT ...` commands.
### Does this PR introduce _any_ user-facing change?
Yes.
### How was this patch tested?
Existing column DEFAULT test coverage + new unit 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] dtenedor commented on pull request #37256: [SPARK-39844][SQL] Update SQLConf for DEFAULT column providers to allow/deny "ALTER TABLE ... ADD COLUMN" commands separately
Posted by GitBox <gi...@apache.org>.
dtenedor commented on PR #37256:
URL: https://github.com/apache/spark/pull/37256#issuecomment-1197410871
Thanks @gengliangwang, addressed the comments and updated the PR description.
--
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] amaliujia commented on a diff in pull request #37256: [SPARK-39844][SQL] Restrict adding DEFAULT columns for existing tables to allowlist of supported data source types
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #37256:
URL: https://github.com/apache/spark/pull/37256#discussion_r930307735
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2919,6 +2919,17 @@ object SQLConf {
.stringConf
.createWithDefault("csv,json,orc,parquet")
+ val ADD_DEFAULT_COLUMN_EXISTING_TABLE_BANNED_PROVIDERS =
+ buildConf("spark.sql.defaultColumn.addColumnExistingTableBannedProviders")
+ .internal()
+ .doc("List of table providers wherein SQL commands are NOT permitted to assign DEFAULT " +
+ "values to new columns in existing tables, such as when using the ALTER TABLE ... " +
+ "ADD COLUMNS command in SQL. Comma-separated list, whitespace ignored, case-insensitive.")
Review Comment:
To me, it's ok to have a mixed known list of data formats that might cover many usages (or re-use `DEFAULT_COLUMN_ALLOWED_PROVIDERS`) while leave room for unknown or new but working data format checking.
I honestly don't have a good idea on this. Will also be happy to hear for different opinions from Spark community side.
--
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] amaliujia commented on a diff in pull request #37256: [SPARK-39844][SQL] Restrict adding DEFAULT columns for existing tables to allowlist of supported data source types
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #37256:
URL: https://github.com/apache/spark/pull/37256#discussion_r930312146
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2919,6 +2919,17 @@ object SQLConf {
.stringConf
.createWithDefault("csv,json,orc,parquet")
+ val ADD_DEFAULT_COLUMN_EXISTING_TABLE_BANNED_PROVIDERS =
+ buildConf("spark.sql.defaultColumn.addColumnExistingTableBannedProviders")
+ .internal()
+ .doc("List of table providers wherein SQL commands are NOT permitted to assign DEFAULT " +
+ "values to new columns in existing tables, such as when using the ALTER TABLE ... " +
+ "ADD COLUMNS command in SQL. Comma-separated list, whitespace ignored, case-insensitive.")
Review Comment:
thanks! +1 to reuse the existing conf if there is no strong objection or obvious drawback.
--
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 #37256: [SPARK-39844][SQL] Restrict adding DEFAULT columns for existing tables to allowlist of supported data source types
Posted by GitBox <gi...@apache.org>.
dtenedor commented on PR #37256:
URL: https://github.com/apache/spark/pull/37256#issuecomment-1194253505
Looking at the CI result, it appears to be internal infrastructure breakage; no actual tests appear to have failed. Pushing an empty commit to re-run them again.
--
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 #37256: [SPARK-39844][SQL] Restrict adding DEFAULT columns for existing tables to allowlist of supported data source types
Posted by GitBox <gi...@apache.org>.
dtenedor commented on code in PR #37256:
URL: https://github.com/apache/spark/pull/37256#discussion_r929421727
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2919,6 +2919,17 @@ object SQLConf {
.stringConf
.createWithDefault("csv,json,orc,parquet")
+ val ADD_DEFAULT_COLUMN_EXISTING_TABLE_BANNED_PROVIDERS =
+ buildConf("spark.sql.defaultColumn.addColumnExistingTableBannedProviders")
+ .internal()
+ .doc("List of table providers wherein SQL commands are NOT permitted to assign DEFAULT " +
+ "values to new columns in existing tables, such as when using the ALTER TABLE ... " +
+ "ADD COLUMNS command in SQL. Comma-separated list, whitespace ignored, case-insensitive.")
Review Comment:
I checked, it looks like not currently: today we support integers, floating-point numbers, booleans, strings, time values, and byte buffers.
I hear your concern and definitely open to ideas though :)
--
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 #37256: [SPARK-39844][SQL] Restrict adding DEFAULT columns for existing tables to allowlist of supported data source types
Posted by GitBox <gi...@apache.org>.
dtenedor commented on code in PR #37256:
URL: https://github.com/apache/spark/pull/37256#discussion_r930310022
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2919,6 +2919,17 @@ object SQLConf {
.stringConf
.createWithDefault("csv,json,orc,parquet")
+ val ADD_DEFAULT_COLUMN_EXISTING_TABLE_BANNED_PROVIDERS =
+ buildConf("spark.sql.defaultColumn.addColumnExistingTableBannedProviders")
+ .internal()
+ .doc("List of table providers wherein SQL commands are NOT permitted to assign DEFAULT " +
+ "values to new columns in existing tables, such as when using the ALTER TABLE ... " +
+ "ADD COLUMNS command in SQL. Comma-separated list, whitespace ignored, case-insensitive.")
Review Comment:
I ended up reusing `DEFAULT_COLUMN_ALLOWED_PROVIDERS` with an optional asterisk at the end of any table provider name to indicate that `ALTER TABLE ADD COLUMN` commands are not allowed. This seems to help reduce duplication by retaining only one SQLConf, I think.
--
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 #37256: [SPARK-39844][SQL] Restrict adding DEFAULT columns for existing tables to allowlist of supported data source types
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on code in PR #37256:
URL: https://github.com/apache/spark/pull/37256#discussion_r931552049
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2914,7 +2914,10 @@ object SQLConf {
buildConf("spark.sql.defaultColumn.allowedProviders")
.internal()
.doc("List of table providers wherein SQL commands are permitted to assign DEFAULT column " +
- "values. Comma-separated list, whitespace ignored, case-insensitive.")
+ "values. Comma-separated list, whitespace ignored, case-insensitive. If an asterisk " +
+ "appears after any table provider in this list, any command may assign DEFAULT column " +
+ "except ALTER TABLE ... ADD COLUMN. Otherwise, if no asterisk appears, all commands are " +
Review Comment:
nit: let's quote the command with '`'
```
except `ALTER TABLE ... ADD COLUMN`
```
--
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 #37256: [SPARK-39844][SQL] Update SQLConf for DEFAULT column providers to allow/deny "ALTER TABLE ... ADD COLUMN" commands separately
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on PR #37256:
URL: https://github.com/apache/spark/pull/37256#issuecomment-1197685044
@dtenedor Thanks for the work! 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 #37256: [SPARK-39844][SQL] Restrict adding DEFAULT columns for existing tables to allowlist of supported data source types
Posted by GitBox <gi...@apache.org>.
dtenedor commented on code in PR #37256:
URL: https://github.com/apache/spark/pull/37256#discussion_r929421727
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2919,6 +2919,17 @@ object SQLConf {
.stringConf
.createWithDefault("csv,json,orc,parquet")
+ val ADD_DEFAULT_COLUMN_EXISTING_TABLE_BANNED_PROVIDERS =
+ buildConf("spark.sql.defaultColumn.addColumnExistingTableBannedProviders")
+ .internal()
+ .doc("List of table providers wherein SQL commands are NOT permitted to assign DEFAULT " +
+ "values to new columns in existing tables, such as when using the ALTER TABLE ... " +
+ "ADD COLUMNS command in SQL. Comma-separated list, whitespace ignored, case-insensitive.")
Review Comment:
I checked, it looks like not currently: today we support integers, floating-point numbers, booleans, strings, time values, and byte buffers [1].
I hear your concern and definitely open to ideas though :)
[1] https://src.dev.databricks.com/databricks/runtime@master/-/blob/core/src/main/scala/org/apache/spark/internal/config/ConfigBuilder.scala?L183&subtree=true
--
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 #37256: [SPARK-39844][SQL] Restrict adding DEFAULT columns for existing tables to allowlist of supported data source types
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on code in PR #37256:
URL: https://github.com/apache/spark/pull/37256#discussion_r929561166
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2919,6 +2919,17 @@ object SQLConf {
.stringConf
.createWithDefault("csv,json,orc,parquet")
+ val ADD_DEFAULT_COLUMN_EXISTING_TABLE_BANNED_PROVIDERS =
Review Comment:
It seems that having DEFAULT_COLUMN_ALLOWED_PROVIDERS is enough. Why do we need to have a new one here?
--
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 #37256: [SPARK-39844][SQL] Restrict adding DEFAULT columns for existing tables to allowlist of supported data source types
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #37256:
URL: https://github.com/apache/spark/pull/37256#issuecomment-1193219134
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 pull request #37256: [SPARK-39844][SQL] Restrict adding DEFAULT columns for existing tables to allowlist of supported data source types
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on PR #37256:
URL: https://github.com/apache/spark/pull/37256#issuecomment-1197341603
@dtenedor we should update the PR description as well
--
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 #37256: [SPARK-39844][SQL] Restrict adding DEFAULT columns for existing tables to allowlist of supported data source types
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on code in PR #37256:
URL: https://github.com/apache/spark/pull/37256#discussion_r929562347
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2919,6 +2919,17 @@ object SQLConf {
.stringConf
.createWithDefault("csv,json,orc,parquet")
+ val ADD_DEFAULT_COLUMN_EXISTING_TABLE_BANNED_PROVIDERS =
+ buildConf("spark.sql.defaultColumn.addColumnExistingTableBannedProviders")
+ .internal()
+ .doc("List of table providers wherein SQL commands are NOT permitted to assign DEFAULT " +
+ "values to new columns in existing tables, such as when using the ALTER TABLE ... " +
+ "ADD COLUMNS command in SQL. Comma-separated list, whitespace ignored, case-insensitive.")
Review Comment:
We do support enum configurations via checking string values. You can check STORE_ASSIGNMENT_POLICY as an example.
For data sources, we can't make it enum since there are external data sources, e.g. Delta, BigQuery, etc.
--
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 #37256: [SPARK-39844][SQL] Restrict adding DEFAULT columns for existing tables to allowlist of supported data source types
Posted by GitBox <gi...@apache.org>.
dtenedor commented on code in PR #37256:
URL: https://github.com/apache/spark/pull/37256#discussion_r930162598
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2919,6 +2919,17 @@ object SQLConf {
.stringConf
.createWithDefault("csv,json,orc,parquet")
+ val ADD_DEFAULT_COLUMN_EXISTING_TABLE_BANNED_PROVIDERS =
+ buildConf("spark.sql.defaultColumn.addColumnExistingTableBannedProviders")
+ .internal()
+ .doc("List of table providers wherein SQL commands are NOT permitted to assign DEFAULT " +
+ "values to new columns in existing tables, such as when using the ALTER TABLE ... " +
+ "ADD COLUMNS command in SQL. Comma-separated list, whitespace ignored, case-insensitive.")
Review Comment:
These are good points. It is not ideal to duplicate the table provider strings in two SQLConf entries.
@gengliangwang and @amaliujia what do you think about simply reusing `DEFAULT_COLUMN_ALLOWED_PROVIDERS` SQLConf and adding some simple rule to mark a table provider as "does not support `ALTER TABLE ADD COLUMN` commands"? For example, we could allow an asterisk after the table provider name to indicate this. For example, we could have `"csv,json,orc,parquet,myv2datasource*"` as the SQLConf value for a new V2 data source.
Edit: I went ahead and implemented this change, the logic is 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] amaliujia commented on a diff in pull request #37256: [SPARK-39844][SQL] Restrict adding DEFAULT columns for existing tables to allowlist of supported data source types
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #37256:
URL: https://github.com/apache/spark/pull/37256#discussion_r929415930
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2919,6 +2919,17 @@ object SQLConf {
.stringConf
.createWithDefault("csv,json,orc,parquet")
+ val ADD_DEFAULT_COLUMN_EXISTING_TABLE_BANNED_PROVIDERS =
+ buildConf("spark.sql.defaultColumn.addColumnExistingTableBannedProviders")
+ .internal()
+ .doc("List of table providers wherein SQL commands are NOT permitted to assign DEFAULT " +
+ "values to new columns in existing tables, such as when using the ALTER TABLE ... " +
+ "ADD COLUMNS command in SQL. Comma-separated list, whitespace ignored, case-insensitive.")
Review Comment:
So there is no ENUM choice for this conf? Strings to specify table providers might not be robust (e.g. typos, etc.)
--
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 #37256: [SPARK-39844][SQL] Restrict adding DEFAULT columns for existing tables to allowlist of supported data source types
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on PR #37256:
URL: https://github.com/apache/spark/pull/37256#issuecomment-1197360723
LGTM overall. I will merge it after comments addressed
--
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 #37256: [SPARK-39844][SQL] Restrict adding DEFAULT columns for existing tables to allowlist of supported data source types
Posted by GitBox <gi...@apache.org>.
dtenedor commented on code in PR #37256:
URL: https://github.com/apache/spark/pull/37256#discussion_r931610468
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2914,7 +2914,10 @@ object SQLConf {
buildConf("spark.sql.defaultColumn.allowedProviders")
.internal()
.doc("List of table providers wherein SQL commands are permitted to assign DEFAULT column " +
- "values. Comma-separated list, whitespace ignored, case-insensitive.")
+ "values. Comma-separated list, whitespace ignored, case-insensitive. If an asterisk " +
+ "appears after any table provider in this list, any command may assign DEFAULT column " +
+ "except ALTER TABLE ... ADD COLUMN. Otherwise, if no asterisk appears, all commands are " +
Review Comment:
Good ideas, thanks for the suggestions. Updated the 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
[GitHub] [spark] gengliangwang closed pull request #37256: [SPARK-39844][SQL] Update SQLConf for DEFAULT column providers to allow/deny "ALTER TABLE ... ADD COLUMN" commands separately
Posted by GitBox <gi...@apache.org>.
gengliangwang closed pull request #37256: [SPARK-39844][SQL] Update SQLConf for DEFAULT column providers to allow/deny "ALTER TABLE ... ADD COLUMN" commands separately
URL: https://github.com/apache/spark/pull/37256
--
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 #37256: [SPARK-39844][SQL] Restrict adding DEFAULT columns for existing tables to allowlist of supported data source types
Posted by GitBox <gi...@apache.org>.
dtenedor commented on PR #37256:
URL: https://github.com/apache/spark/pull/37256#issuecomment-1194733692
Looks like the Apache Spark CI is broken in general temporarily .___. will wait a bit.
--
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 #37256: [SPARK-39844][SQL] Restrict adding DEFAULT columns for existing tables to allowlist of supported data source types
Posted by GitBox <gi...@apache.org>.
dtenedor commented on code in PR #37256:
URL: https://github.com/apache/spark/pull/37256#discussion_r930162598
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2919,6 +2919,17 @@ object SQLConf {
.stringConf
.createWithDefault("csv,json,orc,parquet")
+ val ADD_DEFAULT_COLUMN_EXISTING_TABLE_BANNED_PROVIDERS =
+ buildConf("spark.sql.defaultColumn.addColumnExistingTableBannedProviders")
+ .internal()
+ .doc("List of table providers wherein SQL commands are NOT permitted to assign DEFAULT " +
+ "values to new columns in existing tables, such as when using the ALTER TABLE ... " +
+ "ADD COLUMNS command in SQL. Comma-separated list, whitespace ignored, case-insensitive.")
Review Comment:
These are good points. It is not ideal to duplicate the table provider strings in two SQLConf entries.
@gengliangwang and @amaliujia what do you think about simply reusing `DEFAULT_COLUMN_ALLOWED_PROVIDERS` SQLConf and adding some simple rule to mark a table provider as "does not support `ALTER TABLE ADD COLUMN` commands"? For example, we could allow an asterisk after the table provider name to indicate this. For example, we could have `"csv,json,orc,parquet,myv2datasource*"` as the SQLConf value for a new V2 data source.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2919,6 +2919,17 @@ object SQLConf {
.stringConf
.createWithDefault("csv,json,orc,parquet")
+ val ADD_DEFAULT_COLUMN_EXISTING_TABLE_BANNED_PROVIDERS =
Review Comment:
Good Q: we want to differentiate between which table providers can allow column DEFAULT values with all commands, vs. which ones support all commands *except* `ALTER TABLE ADD COLUMN`. The reason is that the latter command requires the corresponding data source to implement specific support for filling in the existence default values during future scans, while all other commands simply use the `ResolveDefaultColumns` analysis rule in a generic way.
We've implemented this data source scan support for CSV, JSON, Orc, and Parquet file formats, but not for any V2 data sources yet. If someone wants to extend a V2 data source to support column DEFAULT values, they'll have to block `ALTER TABLE ADD COLUMN` commands at first. This PR provides a way to do that.
--
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 #37256: [SPARK-39844][SQL] Restrict adding DEFAULT columns for existing tables to allowlist of supported data source types
Posted by GitBox <gi...@apache.org>.
dtenedor commented on PR #37256:
URL: https://github.com/apache/spark/pull/37256#issuecomment-1193006515
Hi @gengliangwang, this PR allows Spark administrators to configure table providers that allow assigning column DEFAULT values in all cases except for adding new columns to existing tables. This lets data sources that do not yet support existence default value scanning to otherwise work with column DEFAULT values in DML commands.
--
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 #37256: [SPARK-39844][SQL] Restrict adding DEFAULT columns for existing tables to allowlist of supported data source types
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on code in PR #37256:
URL: https://github.com/apache/spark/pull/37256#discussion_r931552510
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2914,7 +2914,10 @@ object SQLConf {
buildConf("spark.sql.defaultColumn.allowedProviders")
.internal()
.doc("List of table providers wherein SQL commands are permitted to assign DEFAULT column " +
- "values. Comma-separated list, whitespace ignored, case-insensitive.")
+ "values. Comma-separated list, whitespace ignored, case-insensitive. If an asterisk " +
+ "appears after any table provider in this list, any command may assign DEFAULT column " +
+ "except ALTER TABLE ... ADD COLUMN. Otherwise, if no asterisk appears, all commands are " +
Review Comment:
Also, it would be great if we can have a simple explanation about why "alter table add column" is special.
--
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