-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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
[SPARK-48720][SQL] Align the command ALTER TABLE ... UNSET TBLPROPERTIES ...
in v1 and v2
#47097
Conversation
… TBLPROPERTIES ...` in v2
spark/docs/sql-ref-syntax-ddl-alter-table.md Line 244 in baf461b
@cloud-fan What do you think? |
Yea UNSET non-existing property should be noop. BTW we should throw table not found error if table does not exist. |
Okay, let me update it. |
IF EXISTS
ignored for ALTER TABLE ... UNSET … TBLPROPERTIES ...
in v2ALTER TABLE ... UNSET TBLPROPERTIES ...
in v1 and v2 in doc docs/sql-ref-syntax-ddl-alter-table.md
.
ALTER TABLE ... UNSET TBLPROPERTIES ...
in v1 and v2 in doc docs/sql-ref-syntax-ddl-alter-table.md
.ALTER TABLE ... UNSET TBLPROPERTIES ...
in v1 and v2 in doc
ALTER TABLE ... UNSET TBLPROPERTIES ...
in v1 and v2 in docALTER TABLE ... UNSET TBLPROPERTIES ...
diff between v1 and v2 in doc
@@ -1075,25 +1075,6 @@ class DDLParserSuite extends AnalysisTest { | |||
ifExists = true)) | |||
} | |||
|
|||
// ALTER TABLE table_name UNSET TBLPROPERTIES [IF EXISTS] ('comment', 'key'); |
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.
The tests have been migrated to AlterTableUnsetTblPropertiesParserSuite
@@ -107,18 +107,6 @@ class DDLParserSuite extends AnalysisTest with SharedSparkSession { | |||
stop = 98)) | |||
} | |||
|
|||
test("alter table unset properties - property values must NOT be set") { |
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.
The test has been migrated to AlterTableUnsetTblPropertiesParserSuite
sql("ALTER TABLE tab1 UNSET TBLPROPERTIES IF EXISTS ('c', 'xyz')") | ||
assert(getProps == Map("x" -> "y")) | ||
} | ||
|
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.
The above test was migrated to AlterTableUnsetTblPropertiesSuite*
and split into table to alter does not exist
, alter table unset properties
and alter table unset non-existent properties
, as alter table unset non-existent properties
behaves differently in v1 and v2.
// property to unset does not exist | ||
checkError( | ||
exception = intercept[AnalysisException] { | ||
sql(s"ALTER TABLE $t UNSET TBLPROPERTIES ('k3', 'k4')") |
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.
This scenario uses different behaviors in v1
and v2
.
@cloud-fan It's ready now, thanks. |
|
||
-- Unset Table Properties | ||
**Note:** If the specified property key does not exist, when you use the v1 command and do not specify `IF EXISTS`, | ||
it will throw the error-condition `UNSET_NONEXISTENT_PROPERTIES` and finally `failed`, |
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.
shall we change the v1 behavior as well? It's not a breaking change to change from failure to no failure.
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.
and we should avoid mentioning v1/v2 in the doc. It's an internal concept and should not be exposed to end users
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.
shall we change the v1 behavior as well? It's not a breaking change to change from failure to no failure.
Sure, let me update it.
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.
and we should avoid mentioning v1/v2 in the doc. It's an internal concept and should not be exposed to end users
Okay.
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.
shall we change the v1 behavior as well? It's not a breaking change to change from failure to no failure.
Done.
ALTER TABLE ... UNSET TBLPROPERTIES ...
diff between v1 and v2 in docALTER TABLE ... UNSET TBLPROPERTIES ...
in v1 and v2
##### Syntax | ||
|
||
```sql | ||
-- Unset Properties | ||
ALTER TABLE table_identifier UNSET TBLPROPERTIES [ IF EXISTS ] ( key1, key2, ... ) |
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.
let's remove [ IF EXISTS ]
as a way to deprecate. It's no longer effective.
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.
Done.
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.
+1, LGTM
…TIES ...` in v1 and v2 ### What changes were proposed in this pull request? The pr aims to: - align the command `ALTER TABLE ... UNSET TBLPROPERTIES ...` in v1 and v2. (this means that in the v1, regardless of whether `IF EXISTS` is specified or not, when unset a `non-existent` property, it is `ignored` and no longer `fails`.) - update the description of `ALTER TABLE ... UNSET TBLPROPERTIES ...` in the doc `docs/sql-ref-syntax-ddl-alter-table.md`. - unify v1 and v2 `ALTER TABLE ... UNSET TBLPROPERTIES ...` tests. - Add the following `scenario` for `ALTER TABLE ... SET TBLPROPERTIES ...` testing A.`table to alter does not exist` B.`alter table set reserved properties` ### Why are the changes needed? - align the command `ALTER TABLE ... UNSET TBLPROPERTIES ...` in v1 and v2, avoid confusing end-users. - to improve test coverage. - align with other similar tests, eg: `AlterTableSetTblProperties*` ### Does this PR introduce _any_ user-facing change? Yes, in the `v1`, regardless of whether `IF EXISTS` is specified or not, when unset a `non-existent` property, it is `ignored` and no longer `fails` ### How was this patch tested? Update some UT & Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47097 from panbingkun/alter_unset_table. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: yangjie01 <yangjie01@baidu.com>
…TIES ...` in v1 and v2 ### What changes were proposed in this pull request? The pr aims to: - align the command `ALTER TABLE ... UNSET TBLPROPERTIES ...` in v1 and v2. (this means that in the v1, regardless of whether `IF EXISTS` is specified or not, when unset a `non-existent` property, it is `ignored` and no longer `fails`.) - update the description of `ALTER TABLE ... UNSET TBLPROPERTIES ...` in the doc `docs/sql-ref-syntax-ddl-alter-table.md`. - unify v1 and v2 `ALTER TABLE ... UNSET TBLPROPERTIES ...` tests. - Add the following `scenario` for `ALTER TABLE ... SET TBLPROPERTIES ...` testing A.`table to alter does not exist` B.`alter table set reserved properties` ### Why are the changes needed? - align the command `ALTER TABLE ... UNSET TBLPROPERTIES ...` in v1 and v2, avoid confusing end-users. - to improve test coverage. - align with other similar tests, eg: `AlterTableSetTblProperties*` ### Does this PR introduce _any_ user-facing change? Yes, in the `v1`, regardless of whether `IF EXISTS` is specified or not, when unset a `non-existent` property, it is `ignored` and no longer `fails` ### How was this patch tested? Update some UT & Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47097 from panbingkun/alter_unset_table. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: yangjie01 <yangjie01@baidu.com>
…TIES ...` in v1 and v2 ### What changes were proposed in this pull request? The pr aims to: - align the command `ALTER TABLE ... UNSET TBLPROPERTIES ...` in v1 and v2. (this means that in the v1, regardless of whether `IF EXISTS` is specified or not, when unset a `non-existent` property, it is `ignored` and no longer `fails`.) - update the description of `ALTER TABLE ... UNSET TBLPROPERTIES ...` in the doc `docs/sql-ref-syntax-ddl-alter-table.md`. - unify v1 and v2 `ALTER TABLE ... UNSET TBLPROPERTIES ...` tests. - Add the following `scenario` for `ALTER TABLE ... SET TBLPROPERTIES ...` testing A.`table to alter does not exist` B.`alter table set reserved properties` ### Why are the changes needed? - align the command `ALTER TABLE ... UNSET TBLPROPERTIES ...` in v1 and v2, avoid confusing end-users. - to improve test coverage. - align with other similar tests, eg: `AlterTableSetTblProperties*` ### Does this PR introduce _any_ user-facing change? Yes, in the `v1`, regardless of whether `IF EXISTS` is specified or not, when unset a `non-existent` property, it is `ignored` and no longer `fails` ### How was this patch tested? Update some UT & Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47097 from panbingkun/alter_unset_table. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: yangjie01 <yangjie01@baidu.com>
What changes were proposed in this pull request?
The pr aims to:
ALTER TABLE ... UNSET TBLPROPERTIES ...
in v1 and v2.(this means that in the v1, regardless of whether
IF EXISTS
is specified or not, when unset anon-existent
property, it isignored
and no longerfails
.)ALTER TABLE ... UNSET TBLPROPERTIES ...
in the docdocs/sql-ref-syntax-ddl-alter-table.md
.ALTER TABLE ... UNSET TBLPROPERTIES ...
tests.scenario
forALTER TABLE ... SET TBLPROPERTIES ...
testingA.
table to alter does not exist
B.
alter table set reserved properties
Why are the changes needed?
ALTER TABLE ... UNSET TBLPROPERTIES ...
in v1 and v2, avoid confusing end-users.AlterTableSetTblProperties*
Does this PR introduce any user-facing change?
Yes, in the
v1
, regardless of whetherIF EXISTS
is specified or not, when unset anon-existent
property, it isignored
and no longerfails
How was this patch tested?
Update some UT & Pass GA.
Was this patch authored or co-authored using generative AI tooling?
No.