Support common prefix listing of diffs (uncommitted, commit view, compare view) #2051

ozkatz · 2021-06-02T15:12:02Z

Addressing some of #2029 - When working with large diffs, paging across multiple data objects is not optimal.
This PR adds the ability (a toggle in the UI) to view diffs with a directory structure. Should make answering "which tables/partitions were modifed" much easier.

lgtm-com · 2021-06-02T15:33:25Z

This pull request introduces 1 alert when merging 15869ed into 50391bb - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

lgtm-com · 2021-06-02T16:45:59Z

This pull request introduces 1 alert when merging a9752a4 into 50391bb - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

nopcoder

Great! minor style comments

nopcoder · 2021-06-02T21:56:06Z

pkg/api/controller.go

+	if params.Delimiter == nil {
+		delimiter = ""
+	} else {
+		delimiter = *params.Delimiter
+	}


keep it simple

Suggested change

if params.Delimiter == nil {

delimiter = ""

} else {

delimiter = *params.Delimiter

}

if params.Delimiter != nil {

delimiter = *params.Delimiter

}

nopcoder · 2021-06-02T21:57:27Z

pkg/api/controller.go

+	if params.Delimiter == nil {
+		delimiter = ""
+	} else {
+		delimiter = *params.Delimiter
+	}


Suggested change

if params.Delimiter == nil {

delimiter = ""

} else {

delimiter = *params.Delimiter

}

if params.Delimiter != nil {

delimiter = *params.Delimiter

}

nopcoder · 2021-06-03T09:30:38Z

pkg/catalog/catalog.go

+}
+
+func GetStartPos(prefix, after, delimiter string) string {
+	// find the correct place to start iterating from based on delimiter, prefix, after


convert to godoc for this func

nopcoder · 2021-06-03T10:55:14Z

webui/src/lib/components/repository/changes.jsx

@@ -50,7 +52,10 @@ export const ChangeEntryRow = ({ entry, showActions, onRevert }) => {
            break;
    }

-    const pathText = entry.path;
+    let pathText = entry.path;
+    if (pathText.indexOf(relativeTo) === 0) {


Suggested change

if (pathText.indexOf(relativeTo) === 0) {

if (pathText.startsWith(relativeTo)) {

nopcoder · 2021-06-03T11:06:02Z

pkg/catalog/catalog.go

+		if delimiter != "" {
+			// common prefix logic goes here.
+			// for every path, after trimming "prefix", take the string upto-and-including the delimiter
+			// if the received path == the entire remainder, add that object as is
+			// if it's just a part of the name, add it as a "common prefix" entry -
+			//   and skip to next record following all those starting with this prefix


Consider reduce the code block size inside this loop by splitting it to extractCommonPrefix, checking if there is a common prefix, and fall to the part where we append and check the limit)

nopcoder · 2021-06-03T11:28:35Z

webui/src/pages/repositories/repository/changes.jsx

+                    after: (!!after) ? after : "",
+                    prefix: (!!prefix) ? prefix : "",
+                    delimiter: (!!delimiter) ? delimiter : "",


no need to convert to boolean when we evaluate an expression already

Suggested change

after: (!!after) ? after : "",

prefix: (!!prefix) ? prefix : "",

delimiter: (!!delimiter) ? delimiter : "",

after: after ? after : "",

prefix: prefix ? prefix : "",

delimiter: delimiter ? delimiter : "",

nopcoder · 2021-06-03T11:29:30Z

webui/src/pages/repositories/repository/changes.jsx

            after={(!!after) ? after : ""}
+            prefix={(!!prefix) ? prefix : ""}
+            delimiter={(!!delimiter) ? delimiter : ""}


Suggested change

after={(!!after) ? after : ""}

prefix={(!!prefix) ? prefix : ""}

delimiter={(!!delimiter) ? delimiter : ""}

after={after ? after : ""}

prefix={prefix ? prefix : ""}

delimiter={delimiter ? delimiter : ""}

nopcoder · 2021-06-03T11:32:14Z

webui/src/lib/components/repository/tree.jsx

+    return {pathname: '/repositories/:repoId/objects', params, query};
+};
+
+export const URINavigator = ({ repo, reference, path, relativeTo = "", pathURLBuilder = buildPathURL }) => {


Suggested change

export const URINavigator = ({ repo, reference, path, relativeTo = "", pathURLBuilder = buildPathURL }) => {

export const URINavigator = ({ repo, reference, path, relativeTo="", pathURLBuilder=buildPathURL }) => {

arielshaqed

Please note LGTM.com comments -- they are sometimes worthwhile, and in any case I'd rather keep the number of open comments down.

Thanks, it looks great and my comments are minor.

api/swagger.yml

pkg/catalog/catalog.go

arielshaqed · 2021-06-06T06:19:18Z

pkg/catalog/catalog_test.go

+		})
+	}
+}
+
 func TestCatalog_ListRepositories(t *testing.T) {


Can/should we also add tests for listings with empty prefixes, i.e. paths such as a/b///c, a/b//c, a/b/c?

pkg/catalog/diff.go

webui/src/lib/components/repository/changes.jsx

johnnyaug

That was fast

johnnyaug · 2021-06-06T09:12:58Z

pkg/api/controller.go

+
+	// discern between an empty delimiter and no delimiter being passed at all
+	// by default, go-swagger will use the default value ("/") even if we pass
+	// a delimiter param that is explicitly empty. This overrides this (wrong) behavior.


This behavior is not wrong, adding allowEmptyValue: true to the parameter definition should solve this.

Well, this uncovered a weird behavior in our system. Once upon a time, there was no delimiter in the object listing API.
It worked as if there's always a delimiter that is "/". When we added a configuratble one, we tried being backwards compatible. IMO, this was probably a mistake. It worked like this:

if no delimiter is passed, assume delimiter = /

if delimiter is passed but is empty (i.e. ?delimiter=), assume no delimiter

if delimiter is passed and is any other value, use that.

while it retained the previous behavior it's a weird choice. it means that if I add a delimiter parameter to diff endpoints I can choose between:

doing the same thing and now assume that no delimiter actually means / - this would now break existing diff clients.

make the API inconsistent by having different defaults: for objects, no delimiter means /, for diff it means no delimiter.

make the API consistent by fixing object listing (and breaking it): no delimiter means no delimiter.

I went with the painful option 3.

johnnyaug · 2021-06-06T09:15:58Z

pkg/api/transform.go

@@ -10,7 +10,7 @@ func transformDifferenceTypeToString(d catalog.DifferenceType) string {
 		return "added"
 	case catalog.DifferenceTypeRemoved:
 		return "removed"
-	case catalog.DifferenceTypeChanged:
+	case catalog.DifferenceTypeChanged, catalog.DifferenceTypeCommonPrefix:


So common prefixes are always shown as changed, regardless of what happened underneath? Worth adding a comment here.

Removed this diff type as it isn't a diff type.

johnnyaug · 2021-06-06T09:36:26Z

webui/src/styles/globals.css

@@ -188,7 +188,7 @@
 }

 .diff-changed {
-  background-color: #d1ecf1;
+  background-color: #ffeaa7;


johnnyaug · 2021-06-06T10:32:49Z

pkg/catalog/catalog.go

+		return ""
+	case delimiter != "" && after != "":
+		// if we have a delimiter and after is not empty, start at the next common prefix after "after"
+		return string(graveler.UpperBoundForPrefix([]byte(after)))


It is not clear to me why the presence of a delimiter changes the return value of this function.

with a normal after, you'd want to start the page with the next lexicographically sorted entry after after.
Once a delimiter is present, you'd want the next entry (or prefix) that doesn't start with after.

Example:

after = a/b/
without a delimiter, a/b/c would be a valid first entry
with a delimiter /, a/b/c is wrong, as it is already folded into a/b/ - the first valid entry would be the next entry not starting with a/b/

johnnyaug · 2021-06-06T10:37:57Z

pkg/catalog/catalog.go

-func listDiffHelper(it EntryDiffIterator, limit int, after string) (Differences, bool, error) {
+const commonPrefixSplitParts = 2
+
+func listDiffHelper(it EntryDiffIterator, prefix, delimiter string, limit int, after string) (Differences, bool, error) {


I think it's time to add unit tests for catalog diff/compare.

Suppose the diff contains two entries b and c.
Suppose after=a, and prefix=c.

The returned diff should be c. I think this function will return an empty diff.
(This may be an abuse of this function, but still as a unit it should return valid results)

Good point - but this is logic that should be in GetStartPos - as in this case the start pos should be "c" and not "a". I also added a test case for it.

johnnyaug · 2021-06-06T10:56:18Z

webui/src/pages/repositories/repository/changes.jsx

+                    <Card.Header>
+                        <span className="float-left">
+                            {(delimiter !== "") && (
+                                <URINavigator path={prefix} reference={reference} repo={repo} relativeTo={`${reference.id} workspace`} pathURLBuilder={(params, query) => {


"workspace" is a term we didn't publicly mention yet - I think best to avoid it for now. Also here it appears in the same font as the path - which is confusing.

Also the breadcrumb links do not work for me

We do use it publicly..

johnnyaug · 2021-06-06T11:19:13Z

webui/src/pages/repositories/repository/compare.jsx

+                                <URINavigator
+                                    path={prefix}
+                                    reference={reference}
+                                    relativeTo={`${reference.id}...${compareReference.id}`}


optional: trim if reference is a commit hash, to conform with a single commit view

johnnyaug · 2021-06-06T11:22:34Z

webui/src/pages/repositories/repository/commits/commit/index.jsx

+                                        return {
+                                            pathname: '/repositories/:repoId/commits/:commitId',
+                                            params: {repoId: repo.id, commitId: commit.id},
+                                            query: {delimiter: "/", prefix: query.path}


is this the right place to hardcode a /?

johnnyaug · 2021-06-06T11:23:23Z

webui/src/pages/repositories/repository/commits/commit/index.jsx

+                        </span>
+                        <span className="float-right">
+                            <Form>
+                                <Form.Switch


Optional: extract the toggle switch to a component

ozkatz · 2021-06-06T16:46:49Z

@nopcoder @johnnyaug @arielshaqed - please notice that I broke backwards compatibility for the object listing API. If you have a better idea I'd love to not do it.

lgtm-com · 2021-06-06T16:55:07Z

This pull request introduces 1 alert when merging 04fb060 into d9ae3b9 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

arielshaqed

About b/c: as long as existing deployed clients are OK (I think we have just the lakectl command) I see no issues with breaking b/c on some weird-ish edge cases.
Note new LGTM.com complaint.
Two minor comments.
???
THANKS (& PROFIT)!!!

arielshaqed · 2021-06-06T18:01:26Z

api/swagger.yml

+        size_bytes:
+          type: integer
+          format: int64


Unclear what is the "size" of a diff. Can you add a description please?

arielshaqed · 2021-06-06T18:03:34Z

pkg/api/controller_test.go

@@ -935,8 +935,9 @@ func TestController_ObjectsListObjectsHandler(t *testing.T) {
 	}

 	t.Run("get object list", func(t *testing.T) {
+		pfx := api.PaginationPrefix("foo/")


Suggested change

pfx := api.PaginationPrefix("foo/")

prefix := api.PaginationPrefix("foo/")

Why abbreviate?

johnnyaug

Nice.
In the UI, I still suggest dropping the word "workspace" from the path.

ozkatz added 6 commits June 1, 2021 15:57

support common prefixes for all diff types

3217b6b

allow listing changes as directories for uncommitted

b014b2b

revert uncommitted only under a prefix!

248f01f

added commit diff common prefix support

05781ec

support common delimiter listing in all diff operations in UI

b706adc

support common delimiter listing in all diff operations in UI

15869ed

ozkatz added area/API Improvements or additions to the API area/UI Improvements or additions to UI labels Jun 2, 2021

ozkatz requested review from nopcoder, johnnyaug and arielshaqed June 2, 2021 15:12

ozkatz self-assigned this Jun 2, 2021

added generated clients, fixed linter

a9752a4

remove unused import

8221a2f

nopcoder approved these changes Jun 3, 2021

View reviewed changes

improve styling per review

8ee4727

arielshaqed requested changes Jun 6, 2021

View reviewed changes

johnnyaug suggested changes Jun 6, 2021

View reviewed changes

ozkatz added 5 commits June 6, 2021 15:05

fix some review comments

85a3958

uniform behavior for delimiters that breaks current object listing

db84caf

UI fixes per review

875e92f

fixed nessie comparison checks

39cc839

attempting to fix nessie

04fb060

ozkatz requested review from johnnyaug and arielshaqed June 6, 2021 16:45

some LGTM fixes that I dont understand

c3caf9d

arielshaqed approved these changes Jun 6, 2021

View reviewed changes

added description to diff size field name

82c3907

johnnyaug approved these changes Jun 7, 2021

View reviewed changes

ozkatz merged commit b66a22e into master Jun 7, 2021

	if (pathText.indexOf(relativeTo) === 0) {
	if (pathText.startsWith(relativeTo)) {

	export const URINavigator = ({ repo, reference, path, relativeTo = "", pathURLBuilder = buildPathURL }) => {
	export const URINavigator = ({ repo, reference, path, relativeTo="", pathURLBuilder=buildPathURL }) => {

	pfx := api.PaginationPrefix("foo/")
	prefix := api.PaginationPrefix("foo/")

Support common prefix listing of diffs (uncommitted, commit view, compare view) #2051

Support common prefix listing of diffs (uncommitted, commit view, compare view) #2051

Conversation

ozkatz commented Jun 2, 2021

lgtm-com bot commented Jun 2, 2021

lgtm-com bot commented Jun 2, 2021

nopcoder 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

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyaug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ozkatz Jun 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ozkatz Jun 6, 2021 • edited Loading

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

ozkatz commented Jun 6, 2021

lgtm-com bot commented Jun 6, 2021

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyaug left a comment

Choose a reason for hiding this comment

ozkatz Jun 6, 2021 •

edited

Loading

ozkatz Jun 6, 2021 •

edited

Loading