-
Notifications
You must be signed in to change notification settings - Fork 115
Refactor Content.rec(..) function to support tail-optimisation #402
Conversation
Tracking Issue: microsoft#400 Parent Issue: N/A Dependencies: N/A What changes were proposed in this pull request? Refactor Content.rec(..) function to support tail-optimisation Does this PR introduce any user-facing change? No How was this patch tested? Existing unit tests should be enough, because it is just refactoring without changing functionality.
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.
Thanks for the PR. A couple of things to address:
- Please use
scalafmt
for formatting - see https://github.com/microsoft/hyperspace/blob/master/docs/coding-guidelines/scala-coding-style.md. - Please add some unit tests on the
rec
method.
Hi @andrei-ionescu ,
|
- Move rec function outside Content case class. - Rename it to 'recFilesApply' - fixed formatting Cover recFilesApply with tests
src/main/scala/com/microsoft/hyperspace/index/IndexLogEntry.scala
Outdated
Show resolved
Hide resolved
* @param directory Root directory | ||
* @param func function which would apply to current prefix and file | ||
* @tparam T | ||
* @return |
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.
Could you add some comment for this? e.g.
Result sequence of the given func for each file.
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
func: (FileInfo, Path) => T): Seq[T] = { | ||
@tailrec | ||
def recAcc[A]( | ||
dirMap: List[(Path, Seq[Directory])], |
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.
Could you check if scalafmt is set properly?
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.
You are right, it works strangely: some scalafmt versions are ignored by idea (1.5, 2.0.0) , while others are too aggressive - look on latest commit with 2.7.5 scalafmt.
Am I missing anything?
acc: Seq[A] = Seq.empty): Seq[A] = { | ||
dirMap match { | ||
case Nil => acc | ||
case (curPrefixPath, curDirs) :: xs => |
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.
Could you explain why it's xs
?
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 have started with common unapplied list x::xs
and simplified the head later)
xs is plural for x.
I will rename it to otherDirs
for readability.
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.
fixed
src/test/scala/com/microsoft/hyperspace/index/IndexLogEntryTest.scala
Outdated
Show resolved
Hide resolved
@@ -240,6 +240,47 @@ class IndexLogEntryTest extends SparkFunSuite with SQLHelper with BeforeAndAfter | |||
assert(actual.sourceFilesSizeInBytes == 200L) | |||
} | |||
|
|||
test("Content.recFilesApply apply function to all files ") { |
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
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.
fixed
Co-authored-by: EJ Song <[email protected]>
…t.scala Co-authored-by: EJ Song <[email protected]>
@dmytroDragan Sorry but, could you revert unnecessary changes from scalafmt to reduce the diff? It can be handled with a different PR (would be appreciated if you could push a new PR!), so that we could review & merge only required change for tailrec. |
Hi @sezruby! I`m using 2.7.5 (latest) scalamft with dev conf:
If anyone gets different result for this files (like old once), please reach out to me and we could try to figure out. I could make a PR with only scalafmt of this two files, so my PR will cover only my changes after merge. What do you think? |
@dmytroDragan, Here is my I usually do The formatting is correct, the only annoying thing is that it order the functions by name too. I will be great if you could double check the diffs before pushing the changes. |
Thank you @andrei-ionescu. I have rechecked my configuration and applied result formatting. |
} | ||
} | ||
|
||
object Content { | ||
|
||
val createFileInfoWithPrefix: (FileInfo, Path) => FileInfo = (f, prefix) => |
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.
just impl. in fileInfos
- there's no other use case.
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.
Correct. I`m just not keen of formatted:
(
f,
prefix) =>
FileInfo(new Path(prefix, f.name).toString, f.size, f.modifiedTime, f.id)).toSet
Moving it to separate function makes it more readable.
Th problem was with 1.5.1 vs 2.7.5 scalafmt. |
What is the context for this pull request?
What changes were proposed in this pull request?
Refactor Content.rec(..) function to support tail-optimisation
Does this PR introduce any user-facing change?
No
How was this patch tested?
Existing unit tests should be enough, because it is just refactoring without changing functionality.