-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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-12446][SQL] Add unit tests for JDBCRDD internal functions #10409
Conversation
Test build #48097 has finished for PR 10409 at commit
|
Test build #48142 has finished for PR 10409 at commit
|
@@ -168,6 +168,35 @@ private[sql] object JDBCRDD extends Logging { | |||
} | |||
|
|||
/** | |||
* Converts value to SQL expression. | |||
*/ | |||
private def compileValue(value: Any): Any = value match { |
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.
did you just move this code block, or was there any changes made?
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.
I just moved compileValue
, escapeSql
, and compileFilter
in this companion object area.
Let me know once you address my comments. This looks pretty good, assuming you mostly just moved the code to become static functions. |
*/ | ||
private def compileValue(value: Any): Any = value match { | ||
case stringValue: String => s"'${escapeSql(stringValue)}'" | ||
case timestampValue: Timestamp => "'" + timestampValue + "'" |
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.
@rxin `s"'$timestampValue'"`` is better here?
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.
both works - i wouldn't worry about 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.
Okay.
@rxin Finished. |
LGTM. We should merge this as soon as tests pass. |
Test build #2244 has finished for PR 10409 at commit
|
Thanks - I've merged this. |
Test build #2245 has finished for PR 10409 at commit
|
Test build #2246 has finished for PR 10409 at commit
|
…functions No tests done for JDBCRDD#compileFilter. Author: Takeshi YAMAMURO <linguin.m.sgmail.com> Closes #10409 from maropu/AddTestsInJdbcRdd. (cherry picked from commit 8c1b867) Author: Takeshi YAMAMURO <[email protected]> Closes #16124 from dongjoon-hyun/SPARK-12446-BRANCH-1.6.
…functions No tests done for JDBCRDD#compileFilter. Author: Takeshi YAMAMURO [email protected] Closes apache#10409 from maropu/AddTestsInJdbcRdd. (cherry picked from commit 8c1b867)
No tests done for JDBCRDD#compileFilter.