Skip to content
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

lakectl log: add option to filter merge commits #8142

Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/lakectl/cmd/common_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ const (
const (
internalPageSize = 1000 // when retrieving all records, use this page size under the hood
defaultAmountArgumentValue = 100 // when no amount is specified, use this value for the argument
maxAmountNoMerges = 333 // when using --no-merges & amount, this const is the upper limit use huristic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rephrase, I don't understand what this means and why 333(?!)


defaultPollInterval = 3 * time.Second // default interval while pulling tasks status
minimumPollInterval = time.Second // minimum interval while pulling tasks status
Expand Down
27 changes: 26 additions & 1 deletion cmd/lakectl/cmd/log.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,19 @@
}
}

// filter merge commits, used for --no-merges flag
func filterMergeCommits(commits []apigen.Commit) []apigen.Commit {
filteredCommits := make([]apigen.Commit, 0, len(commits))
itaiad200 marked this conversation as resolved.
Show resolved Hide resolved

// iterating through data.Commit, appending every instance with 1 or less parents.
for _, commit := range commits {
if len(commit.Parents) <= 1 {
filteredCommits = append(filteredCommits, commit)
}
}
return filteredCommits
}

// logCmd represents the log command
var logCmd = &cobra.Command{
Use: "log <branch URI>",
Expand All @@ -80,6 +93,7 @@
limit := Must(cmd.Flags().GetBool("limit"))
since := Must(cmd.Flags().GetString("since"))
dot := Must(cmd.Flags().GetBool("dot"))
noMerges := Must(cmd.Flags().GetBool("no-merges"))
firstParent := Must(cmd.Flags().GetBool("first-parent"))
objects := Must(cmd.Flags().GetStringSlice("objects"))
prefixes := Must(cmd.Flags().GetStringSlice("prefixes"))
Expand All @@ -100,6 +114,10 @@
if amountForPagination <= 0 {
amountForPagination = internalPageSize
}
// case --no-merges & --amount, ask for more results since some will filter out
if noMerges && amountForPagination < maxAmountNoMerges {
amountForPagination *= 3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I get the 333 - it's directly linked to 3. I wonder how our linter doesn't complain about this magic number.

}
logCommitsParams := &apigen.LogCommitsParams{
After: apiutil.Ptr(apigen.PaginationAfter(after)),
Amount: apiutil.Ptr(apigen.PaginationAmount(amountForPagination)),
Expand Down Expand Up @@ -151,13 +169,19 @@
},
}

// case --no-merges, filter commits and subtract that amount from amount desired
if noMerges {
data.Commits = filterMergeCommits(data.Commits)
amount = amount - (3 * len(data.Commits))

Check failure on line 175 in cmd/lakectl/cmd/log.go

View workflow job for this annotation

GitHub Actions / Run Linters and Checkers

assignOp: replace `amount = amount - (3 * len(data.Commits))` with `amount -= (3 * len(data.Commits))` (gocritic)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it tripled here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought was that if we use that heuristic, in this way we stay true to it. We asked for three times the amount we needed to get ~1/3 results of that. Updating the amount this way will keep that heuristic in every request.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better if we have a single location to handle amount heuristics. Can you try to combine this block and the above one (lines 114-120) into one? Preferably where the loop starts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wrong here, works better with subtracting only the original size. subtracting X3 amount caused receiving less results than asked.

}

if dot {
graph.Write(data.Commits)
} else {
Write(commitsTemplate, data)
}

if amount != 0 {
if amount <= 0 {
// user request only one page
break
}
Expand All @@ -177,6 +201,7 @@
logCmd.Flags().String("after", "", "show results after this value (used for pagination)")
logCmd.Flags().Bool("dot", false, "return results in a dotgraph format")
logCmd.Flags().Bool("first-parent", false, "follow only the first parent commit upon seeing a merge commit")
logCmd.Flags().Bool("no-merges", false, "skip merge commits")
logCmd.Flags().Bool("show-meta-range-id", false, "also show meta range ID")
logCmd.Flags().StringSlice("objects", nil, "show results that contains changes to at least one path in that list of objects. Use comma separator to pass all objects together")
logCmd.Flags().StringSlice("prefixes", nil, "show results that contains changes to at least one path in that list of prefixes. Use comma separator to pass all prefixes together")
Expand Down
Loading