-
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
[MINOR][SQL] Combine the same codes in test cases #23194
Conversation
Good catch |
ok to test |
@@ -37,7 +37,7 @@ import org.apache.spark.internal.Logging | |||
* tasks was performed by the ShuffleMemoryManager. | |||
* | |||
* @param lock a [[MemoryManager]] instance to synchronize on | |||
* @param memoryMode the type of memory tracked by this pool (on- or off-heap) | |||
* @param memoryMode the type of memory tracked by this pool (on-heap or off-heap) |
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.
Is this change related to this PR?
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.
No, I think it can be modified here,so I changed it by the way.
@@ -28,7 +28,7 @@ import org.apache.spark.storage.memory.MemoryStore | |||
* (caching). | |||
* | |||
* @param lock a [[MemoryManager]] instance to synchronize on | |||
* @param memoryMode the type of memory tracked by this pool (on- or off-heap) | |||
* @param memoryMode the type of memory tracked by this pool (on-heap or off-heap) |
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.
ditto
Test build #99551 has finished for PR 23194 at commit
|
eff036c
to
126ca0d
Compare
Test build #99587 has finished for PR 23194 at commit
|
LGTM, cc @HyukjinKwon. |
test("CTAS a managed table with the existing empty directory") { | ||
val tableLoc = new File(spark.sessionState.catalog.defaultTablePath(TableIdentifier("tab1"))) | ||
try { | ||
protected def withTableLocation(tableNames: String)(f: File => Unit): Unit = { |
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.
How about withEmptyDirInTablePath(dirName: String)
?
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.
Ok,thank you !
protected def withTableLocation(tableNames: String)(f: File => Unit): Unit = { | ||
val tableLoc = | ||
new File(spark.sessionState.catalog.defaultTablePath(TableIdentifier(tableNames))) | ||
try |
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.
nit: try {
} | ||
} | ||
|
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.
nit: revert this
126ca0d
to
867ee5e
Compare
@@ -377,41 +377,41 @@ abstract class DDLSuite extends QueryTest with SQLTestUtils { | |||
} | |||
} | |||
|
|||
test("CTAS a managed table with the existing empty directory") { | |||
val tableLoc = new File(spark.sessionState.catalog.defaultTablePath(TableIdentifier("tab1"))) | |||
protected def withEmptyDirInTablePath(dirName: String)(f: File => Unit): Unit = { |
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.
private?
LGTM except for minor comments |
867ee5e
to
877751b
Compare
Test build #99644 has finished for PR 23194 at commit
|
Test build #99649 has finished for PR 23194 at commit
|
877751b
to
ac2b004
Compare
Test build #99652 has finished for PR 23194 at commit
|
retest this please |
Test build #99656 has finished for PR 23194 at commit
|
thanks, merging to master! |
## What changes were proposed in this pull request? In the DDLSuit, there are four test cases have the same codes , writing a function can combine the same code. ## How was this patch tested? existing tests. Closes apache#23194 from CarolinePeng/Update_temp. Authored-by: 彭灿00244106 <00244106@zte.intra> Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org>
What changes were proposed in this pull request?
In the DDLSuit, there are four test cases have the same codes , writing a function can combine the same code.
How was this patch tested?
existing tests.