Vinai/refactor docs #1210

VinaiRachakonda · 2021-01-12T16:42:24Z

This pr refactors DocsReadWriter to simplify the interface. It then removes many of the methods in environment.go that are related to the drw interface.

…s and WriteDocs

…dating work and makes sure docs update method works

zachmu

This is a good start, but has a long way to go. Keep on trucking.

zachmu · 2021-01-13T22:38:15Z

go/libraries/doltcore/env/actions/reset.go

-	if err != nil {
-		return nil, err
-	}
+func resetDocs(ctx context.Context, dbData env.DbData, headRoot *doltdb.RootValue, staged *doltdb.RootValue, docDetails env.Docs) (newStgRoot *doltdb.RootValue, err error) {


Needs a comment

zachmu · 2021-01-13T22:47:05Z

go/libraries/doltcore/env/repo_state.go

-	PutDocsToStaged(ctx context.Context, docDetails []doltdb.DocDetails) (*doltdb.RootValue, error)
-	ResetWorkingDocsToStagedDocs(ctx context.Context) error
-	GetDocDetail(docName string) (doc doltdb.DocDetails, err error)
+	GetDocDetailOnDisk(docName string) (doc doltdb.DocDetails, err error)


Just get rid of this and add a ...string param to GetDocsOnDisk

Wouldn't the return value still be an array then?

zachmu · 2021-01-13T22:47:59Z

go/libraries/doltcore/env/repo_state.go

@@ -76,6 +76,16 @@ type RepoState struct {
 	Branches map[string]BranchConfig `json:"branches"`
 }

+type RootType int


RepoStateRef?

@reltuk have a better idea for a name or an alternative?

I bet you don't need this anymore once you fully commit to separating the logic into methods that update roots, vs. methods that update repo state

zachmu · 2021-01-13T22:58:11Z

go/libraries/doltcore/env/actions/staged.go

 // appended to a docDetails slice. We return a tuple of tables, docDetails and error.
-func GetTblsAndDocDetails(drw env.DocsReadWriter, tbls []string) (tables []string, docDetails []doltdb.DocDetails, err error) {
+func GetTablesAndDocDetails(drw env.DocsReadWriter, tbls []string) (tables []string, docDetails []doltdb.DocDetails, err error) {


This is a bad method. I had to read this three times and look at usages everywhere to figure out what it's doing and why.

Rename this: GetTablesOrDocs

Rename second param: tablesOrFiles

Have it return ([]string, Docs, err)

Change comment to:

Takes a slice of table or file names. Table names are returned as given. Valid doc names are read from disk and their name replace with the names of the dolt_docs system table in the input slice. Valid Docs are returned in the second return param.

Finally move this out of staged.go, it has nothing to do with staged values. A good home would be docs.go.

zachmu · 2021-01-13T23:00:22Z

go/libraries/doltcore/env/dolt_docs.go

+	}
+
+	for i, doc := range docs {
+		doc, err = doltdb.AddNewerTextToDocFromTbl(ctx, docTbl, &sch, doc)


This should not be a method on doltdb

NewerText is a bad name. Get rid of NewerText from the DocDetails object all together. If we want to compare to versions of a doc, we should use two different DocDetails objects.

Move DocDetails out of the doltdb package in general.

None of the methods that manipulate docs in general should be in the root_val.go file

zachmu · 2021-01-13T23:13:09Z

go/libraries/doltcore/env/dolt_docs.go

@@ -100,3 +103,145 @@ func hasDocFile(fs filesys.ReadWriteFS, file string) bool {
 	exists, isDir := fs.Exists(getDocFile(file))
 	return exists && !isDir
 }
+
+// WorkingRootWithDocs returns a copy of the working root that has been updated with the Dolt docs from the file system.
+func WorkingRootWithDocs(ctx context.Context, dbData DbData) (*doltdb.RootValue, error) {


You've moved these out of environment.go (good) but there are still waaaay too many of them (bad).

There should be only one: UpdateRootWithDocs. It should take a root value and update it with whatever docs are on the file system, then return it.

… shifts to changing docDetails structure

zachmu

This is already looking way, way better.

Another batch of comments. Getting closer now, over halfway to cleaning up this mess.

zachmu · 2021-01-19T16:52:13Z

go/cmd/dolt/commands/diff.go

-	if docDetails == nil {
-		docDetails, _ = dEnv.GetAllValidDocDetails()
+	if currentDocDetails == nil {
+		currentDocDetails, _ = drw.GetDocsOnDisk()


Need to inspect and handle error here

Actually why do we need this entire block? Can't we enforce that we get the current docs before calling this? Then we don't need to pass a drw at all

zachmu · 2021-01-19T17:22:54Z

go/cmd/dolt/commands/diff.go

-		if toTbl != nil {
-			sch1, _ := toTbl.GetSchema(ctx)
-			doc, _ = doltdb.AddNewerTextToDocFromTbl(ctx, toTbl, &sch1, doc)
+	for _, doc := range currentDocDetails {


The error handling in this entire block is missing

zachmu · 2021-01-19T17:28:12Z

go/libraries/doltcore/doltdocs/docs.go

+	LicenseFile = "../LICENSE.md"
+)
+
+type DocDetails struct {


Probably time to rename this to Doc.

And params of this type to doc / docs

zachmu · 2021-01-19T17:28:43Z

go/libraries/doltcore/doltdocs/docs.go

+	{DocPk: doltdb.LicensePk, File: LicenseFile},
+}
+
+func GetDocFile(filename string) string {


GetDocFilePath

zachmu · 2021-01-19T17:28:53Z

go/libraries/doltcore/doltdocs/docs.go

+	{DocPk: doltdb.LicensePk, File: LicenseFile},
+}
+
+func GetDocFile(filename string) string {


Need method doc

zachmu · 2021-01-19T18:11:03Z

go/libraries/doltcore/env/actions/docs.go

@@ -123,17 +125,17 @@ func getUpdatedWorkingAndStagedWithDocs(ctx context.Context, dEnv *env.DoltEnv,
 		root = staged
 	}

-	docs, err := dEnv.GetDocsWithNewerTextFromRoot(ctx, root, docDetails)
+	docs, err := env.GetDocsWithTextFromRoot(ctx, root, docDetails)


This should be a method in the doltdocs package, takes a root and returns docs.

DocsInRoot

zachmu · 2021-01-19T18:13:28Z

go/libraries/doltcore/env/dolt_docs.go

+// UpdateRootWithDocs takes in a root value, a drw, and some docs and writes those docs to the dolt_docs table
+// (perhaps creating it in the process). The table might not necessarily need to be created if there are no docs in the
+// repo yet.
+func UpdateRootWithDocs(ctx context.Context, dbData DbData, root *doltdb.RootValue, rootType RootType, docDetails doltdocs.Docs) (*doltdb.RootValue, error) {


As in other comments: this method goes in doltdocs package

zachmu · 2021-01-19T18:14:01Z

go/libraries/doltcore/env/dolt_docs.go

-			}
+// GetDocsWithTextFromRoot returns Docs with the Text value(s) from the provided root. If docs are provided,
+// only those docs will be retrieved and returned. Otherwise, all valid doc details are returned with the updated Text.
+func GetDocsWithTextFromRoot(ctx context.Context, root *doltdb.RootValue, docs doltdocs.Docs) (doltdocs.Docs, error) {


doltdocs package

zachmu · 2021-01-19T18:16:05Z

go/libraries/doltcore/env/environment.go

-func (dEnv *DoltEnv) GetAllValidDocDetails() (docs []doltdb.DocDetails, err error) {
-	docs = []doltdb.DocDetails{}
-	for _, doc := range *AllValidDocDetails {
+func (dEnv *DoltEnv) GetAllValidDocDetails() (docs doltdocs.Docs, err error) {


This method and the one below belong in doltdocs package

zachmu · 2021-01-19T18:17:49Z

go/libraries/doltcore/sqle/docsTable/dolt_docs_table.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package docsTable


Put this in dtables with other persisted system tables (dolt_query_catalog), not in a new package

Blocked on this because of import loop stemming from status table :/

I'm fine with it being separate for now, not a huge deal

But you should try again at the end once you do everything else, it might unknot itself.

zachmu

This is looking better with every revision. You're getting close, stay with me!

zachmu · 2021-01-20T00:16:38Z

go/cmd/dolt/commands/diff.go

 }

-func printDocDiffs(ctx context.Context, dEnv *env.DoltEnv, fromTbl, toTbl *doltdb.Table, docDetails []doltdb.DocDetails) {
+func printDocDiffs(ctx context.Context, fromTbl, toTbl *doltdb.Table, currentDocDetails doltdocs.Docs) error {


currentDocs

zachmu · 2021-01-20T00:17:48Z

go/libraries/doltcore/diff/docs_diffs.go

+
+// DocDiff returns the added, modified and removed docs when comparing a root value with an other (newer) value. If the other value,
+// is not provided, then we compare the docs on the root value to the docDetails provided.
+func DocDiff(ctx context.Context, root *doltdb.RootValue, other *doltdb.RootValue, docDetails doltdocs.Docs) (added, modified, removed []string, err error) {


zachmu · 2021-01-20T00:18:28Z

go/libraries/doltcore/diff/docs_diffs.go

+
+type docComparison struct {
+	CurrentText []byte
+	DocPk       string


This should be the first field

Should be DocName, not Pk

zachmu · 2021-01-20T00:18:59Z

go/libraries/doltcore/diff/docs_diffs.go

+	return docComparisons, nil
+}
+
+func getDocComparisonObjFromDocDetail(ctx context.Context, tbl *doltdb.Table, sch *schema.Schema, doc doltdocs.Doc) (docComparison, error) {


newDocComparison

zachmu · 2021-01-20T00:19:24Z

go/libraries/doltcore/diff/table_deltas.go

@@ -88,21 +88,21 @@ func (rvu RootValueUnreadable) Error() string {
 }

 // NewDocDiffs returns DocDiffs for Dolt Docs between two roots.
-func NewDocDiffs(ctx context.Context, older *doltdb.RootValue, newer *doltdb.RootValue, docDetails []doltdb.DocDetails) (*DocDiffs, error) {
+func NewDocDiffs(ctx context.Context, older *doltdb.RootValue, newer *doltdb.RootValue, docDetails doltdocs.Docs) (*DocDiffs, error) {


zachmu · 2021-01-20T01:01:26Z

go/libraries/doltcore/env/dolt_docs.go

+// UpdateRootWithDocs takes in a root value, a drw, and some docs and writes those docs to the dolt_docs table
+// (perhaps creating it in the process). The table might not necessarily need to be created if there are no docs in the
+// repo yet.
+func UpdateRootWithDocs(ctx context.Context, dbData DbData, root *doltdb.RootValue, rootType RootType, docDetails doltdocs.Docs) (*doltdb.RootValue, error) {


Wrong abstraction. You're doing too much here. We need two things:

methods that update root values with new tables or doc contents

methods that update the repo state of [working,staged,head] to root value hashes.

You're combining them here.

You don't need, and shouldn't have, methods that update repo state specific to docs contents. If you want to update docs persisted in the repo state, then

modify a root value to have the dolt_docs table you want,

write that thing to staged or head

This method should do 1) only, and it should go in the dolt_docs package.

This method should take (ctx, root, docs) and return (root, error)

zachmu · 2021-01-20T01:06:14Z

go/libraries/doltcore/env/repo_state.go

@@ -76,6 +76,16 @@ type RepoState struct {
 	Branches map[string]BranchConfig `json:"branches"`
 }

+type RootType int


I bet you don't need this anymore once you fully commit to separating the logic into methods that update roots, vs. methods that update repo state

zachmu · 2021-01-20T01:21:38Z

go/libraries/doltcore/sqle/docsTable/dolt_docs_table.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package docsTable


I'm fine with it being separate for now, not a huge deal

zachmu · 2021-01-20T01:21:55Z

go/libraries/doltcore/sqle/docsTable/dolt_docs_table.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package docsTable


But you should try again at the end once you do everything else, it might unknot itself.

zachmu · 2021-01-20T01:22:04Z

go/libraries/doltcore/sqle/docsTable/dolt_docs_table.go

+var DoltDocsSchema = schema.MustSchemaFromCols(doltDocsColumns)
+
+// updateDocsTable takes in docTbl param and updates it with the value in docDetails. It returns the updated table.
+func UpdateDocsTable(ctx context.Context, docTbl *doltdb.Table, docDetails doltdocs.Docs) (*doltdb.Table, error) {


VinaiRachakonda · 2021-01-22T00:42:12Z

go/libraries/doltcore/env/actions/docs.go

+			if err != nil {
+				return nil, nil, err
+			}
+			doc := docAr[0]


Is there a better way to do this?

Not really, but you should do a bound check

zachmu

This looks really good, nice work.

The last big thing I want you to do before checking this in is to move the docs table / row iteration code out of the diff package and into the doltdocs package, and write a new differ algorithm based on two doltdocs.Docs. Then there will be no reason for anyone outside that package to care about how docs are stored in the dolt_docs table.

zachmu · 2021-01-22T22:36:18Z

go/cmd/dolt/commands/diff.go

+		return nil, nil, nil, err
+	}
+
+	workingRoot, err = env.UpdateRootWithDocs(ctx, workingRoot, docs)


This shouldn't be in the env package. Put it in the doltdocs or similar package.

zachmu · 2021-01-22T22:37:11Z

go/libraries/doltcore/diff/docs_diff_test.go

@@ -0,0 +1,174 @@
+// Copyright 2020 Dolthub, Inc.


Might as well update this now

GoLand can automate for you

zachmu · 2021-01-22T22:40:03Z

go/libraries/doltcore/diff/docs_diffs.go

+// DocsDiff returns the added, modified and removed docs when comparing a root value with an other (newer) value. If the other value,
+// is not provided, then we compare the docs on the root value to the docs provided.
+func DocsDiff(ctx context.Context, root *doltdb.RootValue, other *doltdb.RootValue, docs doltdocs.Docs) (added, modified, removed []string, err error) {
+	oldTbl, oldTblFound, err := root.GetTable(ctx, doltdb.DocTableName)


This logic to get the docs table should maybe go into the doltdocs package, but not a big deal

See comments on getDocComparisonsBtwnRoots, you shouldn't need to get any tables or schemas in this method.

zachmu · 2021-01-22T22:56:20Z

go/libraries/doltcore/diff/docs_diffs.go

+
+type docComparison struct {
+	CurrentText []byte
+	DocPk       string


Should be DocName, not Pk

zachmu · 2021-01-22T22:59:42Z

go/libraries/doltcore/diff/docs_diffs.go

+		if err != nil {
+			return nil, err
+		}
+		err = newRows.IterAll(ctx, func(key, val types.Value) error {


This entire loop should take place in doltdocs.

doltdocs.GetAllDocs(*rootValue) Docs

No need for this layer of the business logic to iterate over rows in the storage layer like this. Ask an abstraction to give you all the docs in a root value.

zachmu · 2021-01-23T00:01:47Z

go/libraries/doltcore/doltdocs/docs_table.go

+}
+
+// GetDocTextFromTbl returns the Text field of a doc using the provided table and schema and primary key.
+func GetDocTextFromTbl(ctx context.Context, tbl *doltdb.Table, sch *schema.Schema, docPk string) ([]byte, error) {


zachmu · 2021-01-23T00:06:24Z

go/libraries/doltcore/env/actions/docs.go

+			if err != nil {
+				return nil, nil, err
+			}
+			doc := docAr[0]


Not really, but you should do a bound check

zachmu · 2021-01-23T00:07:11Z

go/libraries/doltcore/env/actions/staged.go

 	if err != nil {
 		return err
 	}

+	if len(docs) > 0 {
+		working, err = env.UpdateRootWithDocs(ctx, working, docs)


This is what i'm talking about, so much cleaner

zachmu · 2021-01-23T00:08:47Z

go/libraries/doltcore/env/dolt_docs.go

-		if doc.DocPk == docName {
-			return true
-		}
+// UpdateRootWithDocs takes in a root value, and some docs and writes those docs to the dolt_docs table


Definitely belongs in the doltdocs package

zachmu · 2021-01-23T00:10:08Z

go/libraries/doltcore/env/repo_state.go

-	PutDocsToStaged(ctx context.Context, docDetails []doltdb.DocDetails) (*doltdb.RootValue, error)
-	ResetWorkingDocsToStagedDocs(ctx context.Context) error
-	GetDocDetail(docName string) (doc doltdb.DocDetails, err error)
+	GetDocsOnDisk(docNames ...string) (doltdocs.Docs, error)


Method doc for both these, describe params

VinaiRachakonda added 6 commits January 11, 2021 18:08

Prelimm methods of drw

9f3913f

Remove the some DocsReadWriter methods and move towards use of GetDoc…

e121d06

…s and WriteDocs

Fix bug with repos that have no docs initialized

6f30212

clean up method

a0b4235

Formatting, comments and remove method:

bf91136

Commit compiles and passes docs test. Factors out much of the root up…

cd85e71

…dating work and makes sure docs update method works

VinaiRachakonda force-pushed the vinai/refactor-docs branch from 1b7f556 to cd85e71 Compare January 12, 2021 21:43

VinaiRachakonda added 3 commits January 12, 2021 17:21

Dump a bunch of methods environment.go

3ef5011

Merge branch 'master' into vinai/refactor-docs

a41b41d

Final updates

efaa0bb

VinaiRachakonda requested a review from zachmu January 12, 2021 22:51

VinaiRachakonda changed the title ~~[WIP] Vinai/refactor docs~~ Vinai/refactor docs Jan 12, 2021

zachmu reviewed Jan 13, 2021

View reviewed changes

VinaiRachakonda added 10 commits January 14, 2021 14:44

Remove a bunch of stuff from root_val

d380a2d

Move some more methods areound and create a docs table package. Focus…

3598fc5

… shifts to changing docDetails structure

Redo diffs.go to not use docs

3f2db74

Redo doc diffs, still failing tests

4160af0

Passing all docs tests...

28bd7db

First step in refactoring out doltdocs package...

5ac7392

type formatting and other things

4ad5617

I think everything is passing

c4aa8f5

Undo bats rewrite

ee97d12

Small fixes

c9c684d

zachmu reviewed Jan 19, 2021

View reviewed changes

VinaiRachakonda added 2 commits January 19, 2021 17:59

More changes

e6f05fd

repo fmt

ad439c8

zachmu reviewed Jan 20, 2021

View reviewed changes

VinaiRachakonda added 3 commits January 21, 2021 13:25

Pr review comments before large drw refactor

ccdfb0f

Iniital changes

9443ca6

Method changes

531b88a

VinaiRachakonda added 2 commits January 21, 2021 18:25

additional changes

a345da2

some refactoring :)

76e3f1f

VinaiRachakonda commented Jan 22, 2021

View reviewed changes

VinaiRachakonda linked an issue Jan 22, 2021 that may be closed by this pull request

Refactor Docs methods out of environment.go #1211

Closed

zachmu approved these changes Jan 23, 2021

View reviewed changes

VinaiRachakonda added 4 commits January 23, 2021 13:46

rewrite the diffs alg

b4a329e

Make significant refacotring changes. Need to rewrite cli diff

288f01b

More changes

5ecabe6

Fix bc bug

69356ca

VinaiRachakonda merged commit 3342b8d into master Jan 25, 2021

Hydrocharged deleted the vinai/refactor-docs branch February 19, 2021 11:39

Vinai/refactor docs #1210

Vinai/refactor docs #1210

Conversation

VinaiRachakonda commented Jan 12, 2021 • edited

zachmu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachmu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachmu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachmu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinaiRachakonda commented Jan 12, 2021 •

edited