Refactor Content.rec(..) function to support tail-optimisation by dmytroDragan · Pull Request #402 · microsoft/hyperspace

dmytroDragan · 2021-03-30T10:31:19Z

What is the context for this pull request?

Tracking Issue: Content.rec(..) in LogEntry is recursive function, but does not use tail rec optimisation #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.

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.

ghost · 2021-03-30T10:31:31Z

All CLA requirements met.

andrei-ionescu

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.

dmytroDragan · 2021-03-30T14:10:26Z

Hi @andrei-ionescu ,

Sure (might make sense to add also scalafmt version for alignment)
There are already tests for Content class like "Content.files api lists all files from Content object" which rely on it, but I could move rec function to Content object and add more.

- 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

sezruby · 2021-04-01T02:33:34Z

src/main/scala/com/microsoft/hyperspace/index/IndexLogEntry.scala

+   * @param directory Root directory
+   * @param func function which would apply to current prefix and file
+   * @tparam T
+   * @return


Could you add some comment for this? e.g.
Result sequence of the given func for each file.

sezruby · 2021-04-01T02:39:12Z

src/main/scala/com/microsoft/hyperspace/index/IndexLogEntry.scala

+              func: (FileInfo, Path) => T): Seq[T] = {
+    @tailrec
+    def recAcc[A](
+                   dirMap: List[(Path, Seq[Directory])],


Could you check if scalafmt is set properly?

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?

sezruby · 2021-04-01T20:08:57Z

src/main/scala/com/microsoft/hyperspace/index/IndexLogEntry.scala

+                   acc: Seq[A] = Seq.empty): Seq[A] = {
+      dirMap match {
+        case Nil => acc
+        case (curPrefixPath, curDirs) :: xs =>


Could you explain why it's xs?

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.

src/test/scala/com/microsoft/hyperspace/index/IndexLogEntryTest.scala

sezruby · 2021-04-01T20:16:42Z

src/test/scala/com/microsoft/hyperspace/index/IndexLogEntryTest.scala

    assert(actual.sourceFilesSizeInBytes == 200L)
  }

+  test("Content.recFilesApply apply function to all files ") {


Co-authored-by: EJ Song <51077614+sezruby@users.noreply.github.com>

…t.scala Co-authored-by: EJ Song <51077614+sezruby@users.noreply.github.com>

sezruby · 2021-04-09T06:19:24Z

@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.

dmytroDragan · 2021-04-09T09:24:58Z

Hi @sezruby!
I can revert changes from scalafmt, but it makes a usage of scalafmt useless in first place.

I`m using 2.7.5 (latest) scalamft with dev conf:

# The following configs are taken from https://github.com/apache/spark/blob/master/dev/.scalafmt.conf
align = none
align.openParenDefnSite = false
align.openParenCallSite = false
align.tokens = []
optIn = {
  configStyleArguments = false
}
danglingParentheses = false
docstrings = JavaDoc
maxColumn = 98

# The following are specific to Hyperspace.
importSelectors = singleLine

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?

andrei-ionescu · 2021-04-09T11:37:28Z

@dmytroDragan, Here is my scalafmt setup in IntelliJ Idea (I have 1.5.1 version):

I usually do Command+Shift+Alt+L to reformat the file and use the following options in the dialog box:

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.

dmytroDragan · 2021-04-09T14:01:28Z

Thank you @andrei-ionescu. I have rechecked my configuration and applied result formatting.
I have created PR with just scalafmt formated code for this 2 files, so reviewing it could be easier:
#412

sezruby · 2021-04-10T02:22:57Z

src/main/scala/com/microsoft/hyperspace/index/IndexLogEntry.scala


 object Content {

+  val createFileInfoWithPrefix: (FileInfo, Path) => FileInfo = (f, prefix) =>


just impl. in fileInfos - there's no other use case.

Correct. I`m just not keen of formatted:

( f, prefix) => FileInfo(new Path(prefix, f.name).toString, f.size, f.modifiedTime, f.id)).toSet

see: https://github.com/microsoft/hyperspace/pull/412/files#diff-5c50f07a41423f4cb5bfe60ac7c30b9c534c4bfc71eeb03d43040047a3994e83

Moving it to separate function makes it more readable.

dmytroDragan · 2021-04-14T12:50:02Z

Th problem was with 1.5.1 vs 2.7.5 scalafmt.
Fixed version is done as new PR:
#417

andrei-ionescu reviewed Mar 30, 2021

View reviewed changes

Refactoring:

c36ed37

- Move rec function outside Content case class. - Rename it to 'recFilesApply' - fixed formatting Cover recFilesApply with tests

sezruby assigned dmytroDragan Mar 30, 2021

sezruby reviewed Apr 1, 2021

View reviewed changes

sezruby requested a review from imback82 April 1, 2021 20:18

dmytroDragan and others added 3 commits April 2, 2021 09:36

Update src/main/scala/com/microsoft/hyperspace/index/IndexLogEntry.scala

bce31e2

Co-authored-by: EJ Song <51077614+sezruby@users.noreply.github.com>

Update src/test/scala/com/microsoft/hyperspace/index/IndexLogEntryTes…

a342ae1

…t.scala Co-authored-by: EJ Song <51077614+sezruby@users.noreply.github.com>

improve readability of recAcc function

add47fd

dmytroDragan dismissed a stale review via add47fd April 2, 2021 08:25

ddrag-softserve added 2 commits April 2, 2021 13:09

Align with description format

1226937

Removed unneeded space

3a073e7

dmytroDragan and others added 3 commits April 9, 2021 14:22

Merge branch 'master' into issue400

84ea232

reformat with latest master version

23fe8f1

reformat with latest master version after checking scalafmt options

1948e0c

dmytroDragan mentioned this pull request Apr 9, 2021

Issue400 just scalafmt formated #412

Closed

sezruby reviewed Apr 10, 2021

View reviewed changes

dmytroDragan closed this Apr 14, 2021

dmytroDragan deleted the issue400 branch April 14, 2021 12:52


		object Content {

		val createFileInfoWithPrefix: (FileInfo, Path) => FileInfo = (f, prefix) =>

Conversation

dmytroDragan commented Mar 30, 2021

What is the context for this pull request?

What changes were proposed in this pull request?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

ghost commented Mar 30, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrei-ionescu left a comment

Choose a reason for hiding this comment

Uh oh!

dmytroDragan commented Mar 30, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmytroDragan Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmytroDragan Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sezruby commented Apr 9, 2021

Uh oh!

dmytroDragan commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrei-ionescu commented Apr 9, 2021

Uh oh!

dmytroDragan commented Apr 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmytroDragan commented Apr 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ghost commented Mar 30, 2021 •

edited by ghost

Loading

dmytroDragan Apr 2, 2021 •

edited

Loading

dmytroDragan Apr 2, 2021 •

edited

Loading

dmytroDragan commented Apr 9, 2021 •

edited

Loading