Skip to content

Conversation

@panbingkun
Copy link
Contributor

@panbingkun panbingkun commented Jun 19, 2024

What changes were proposed in this pull request?

  • Move parser tests from o.a.s.s.c.p.DDLParserSuite and o.a.s.s.e.c.DDLParserSuite to AlterTableSetTblPropertiesParserSuite.
  • Porting and refactoring DS v1 tests from DDLSuite and HiveDDLSuite to v1.AlterTableSetTblPropertiesBase and to v1.AlterTableSetTblPropertiesSuite.
  • Add a test for DSv2 ALTER TABLE .. RENAME to v2.AlterTableSetTblPropertiesSuite.

Why are the changes needed?

  • To improve test coverage.
  • Align with other similar tests, eg: AlterTableRename*

Does this PR introduce any user-facing change?

No.

How was this patch tested?

  • Add new UT & Update existed UT.
  • By running new test suites:
    $ build/sbt -Phive -Phive-thriftserver "test:testOnly *AlterTableSetTblPropertiesSuite"
    $ build/sbt -Phive -Phive-thriftserver "test:testOnly *AlterTableSetTblPropertiesParserSuite"
    

Was this patch authored or co-authored using generative AI tooling?

No.

@github-actions github-actions bot added the SQL label Jun 19, 2024
ifExists = true))
}

// ALTER TABLE table_name SET TBLPROPERTIES ('comment' = new_comment);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only tests related to SET have been moved, and tests related to UNSET will doing in another PR

}

// ALTER TABLE table_name SET TBLPROPERTIES ('comment' = new_comment);
test("alter table: alter table properties") {
Copy link
Contributor Author

@panbingkun panbingkun Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ported from sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala

Map("test" -> "test", "comment" -> "new_comment")))
}

test("alter table - property values must be set") {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ported from sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala

@panbingkun panbingkun marked this pull request as ready for review June 19, 2024 23:22
@panbingkun
Copy link
Contributor Author

panbingkun commented Jun 19, 2024

@cloud-fan Can you take a time to help review? Thank you very much.
Additionally, I have found an issue where we only have the AlterNamespaceSetPropertiesExec and not the AlterNamespaceUnsetPropertiesExec. Do we need to implement it?

@panbingkun
Copy link
Contributor Author

cc @HyukjinKwon

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in cd8bf11 Jun 21, 2024
@cloud-fan
Copy link
Contributor

Yea let's add AlterNamespaceUnsetPropertiesExec for DS v2

@panbingkun
Copy link
Contributor Author

Yea let's add AlterNamespaceUnsetPropertiesExec for DS v2

Thank you for your review!
I will also do Unify v1 and v2 ALTER TABLE .. UNSET TBLPROPERTIES tests later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants