-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-5931][CORE] Use consistent naming for time properties #5236
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
Changes from 1 commit
59bf9e1
404f8c3
7db6d2a
4933fda
c9f5cad
21ef3dd
064ebd6
7320c87
272c215
3352d34
3f1cfc8
6d1518e
2fcc91c
5181597
c6a0095
cde9bff
42477aa
9a29d8d
34f87c2
8f741e1
9e2547c
499bdf0
5232a36
3a12dd8
68f4e93
70ac213
647b5ac
1c0c07c
8613631
bac9edf
1858197
3b126e1
39164f9
b2fc965
dd0a680
bf779b0
76cfa27
5193d5f
6387772
19c31af
ff40bfe
28187bf
1465390
cbf41db
d4efd26
4e48679
1a1122c
cbd2ca6
6f651a8
7d19cdd
dc7bd08
69fedcc
8927e66
642a06d
25d3f52
bc04e05
951ca2d
f5fafcd
de3bff9
4526c81
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1022,17 +1022,18 @@ private[spark] object Utils extends Logging { | |
| * Convert a passed time string (e.g. 50s, 100ms, or 250us) to a microsecond count for | ||
| * internal use. If no suffix is provided a direct conversion is attempted. | ||
| */ | ||
| @throws(classOf[NumberFormatException]) | ||
| private def parseTimeString(str: String) : (Option[TimeUnit], Long) = { | ||
| val timeError = "Time must be specified as seconds (s), " + | ||
| "milliseconds (ms), or microseconds (us) e.g. 50s, 100ms, or 250us." | ||
| "milliseconds (ms), microseconds (us), minutes (min) hour (h), or day(d). " + | ||
| "E.g. 50s, 100ms, or 250us." | ||
|
|
||
| try { | ||
| val lower = str.toLowerCase.trim() | ||
| var suffix: String = "" | ||
| timeSuffixes.foreach(s => { | ||
| if(lower.endsWith(s._1)) | ||
| if(lower.endsWith(s._1)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: space after |
||
| suffix = s._1 | ||
| } | ||
| }) | ||
|
|
||
| (timeSuffixes.get(suffix), str.substring(0, str.length - suffix.length).toLong) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -135,12 +135,19 @@ private static long timeStringToUs(String str) throws NumberFormatException { | |
| return Long.parseLong(lower.substring(0, lower.length() - 2)); | ||
| } else if (lower.endsWith("s")) { | ||
| return Long.parseLong(lower.substring(0, lower.length() - 1)) * 1000 * 1000; | ||
| } else {// Invalid suffix, force correct formatting | ||
| } else if (lower.endsWith("min")) { | ||
| return Long.parseLong(lower.substring(0, lower.length() - 1)) * 1000 * 1000 * 60; | ||
| } else if (lower.endsWith("h")) { | ||
| return Long.parseLong(lower.substring(0, lower.length() - 1)) * 1000 * 1000 * 60 * 60; | ||
| } else if (lower.endsWith("d")) { | ||
| return Long.parseLong(lower.substring(0, lower.length() - 1)) * 1000 * 1000 * 60 * 60 * 24; | ||
| } else {// No suffix, default selected by calling function | ||
| return Long.parseLong(lower); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This whole method is really dense. Should we do something like the following instead so it's easier to follow:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ugh, that was much longer than expected. It would have been a lot more concise in Scala without sacrificing any readability
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using regexes this would be pretty short. Or something along those lines.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, do that! |
||
| } catch(NumberFormatException e) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: space before |
||
| throw new NumberFormatException("Time must be specified as seconds (s), " + | ||
| "milliseconds (ms), or microseconds (us) e.g. 50s, 100ms, or 250us."); | ||
| "milliseconds (ms), microseconds (us), minutes (min) hour (h), or day(d). " + | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. space before |
||
| "E.g. 50s, 100ms, or 250us.\n" + e.toString()); | ||
| } | ||
|
|
||
| } | ||
|
|
||
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: indentation is wrong