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

Sort alphabets of languages for non indexed fields. #4260

Merged
merged 6 commits into from
Nov 21, 2019

Conversation

arijitAD
Copy link

@arijitAD arijitAD commented Nov 11, 2019

This will fix the sorting of a non-indexed field having lang directive.
Fixes #4005


This change is Reviewable

types/collate/collate.go Outdated Show resolved Hide resolved
posting/list.go Outdated Show resolved Hide resolved
posting/list.go Outdated
@@ -1014,21 +1014,22 @@ func (l *List) Value(readTs uint64) (rval types.Val, rerr error) {
func (l *List) ValueFor(readTs uint64, langs []string) (rval types.Val, rerr error) {
l.RLock() // All public methods should acquire locks, while private ones should assert them.
defer l.RUnlock()
p, err := l.postingFor(readTs, langs)
p, lang, err := l.postingFor(readTs, langs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't make a lot of changes.

you already passing lang as a parameter. just use it from the input. Anyways you're going to sort based on
one language. Just put an assert in the sort fetch. So you don't break anything unknowingly

query/query.go Outdated Show resolved Hide resolved
Copy link
Author

@arijitAD arijitAD left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 7 files reviewed, 4 unresolved discussions (waiting on @balajijinnah, @golangcibot, @manishrjain, @martinmr, and @pawanrawal)


posting/list.go, line 1017 at r1 (raw file):

Previously, balajijinnah (balaji) wrote…

Don't make a lot of changes.

you already passing lang as a parameter. just use it from the input. Anyways you're going to sort based on
one language. Just put an assert in the sort fetch. So you don't break anything unknowingly

Done.


posting/list.go, line 1057 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 106 characters (from lll)

Done.


query/query.go, line 2375 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 103 characters (from lll)

Done.


types/collate/collate.go, line 35 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 115 characters (from lll)

Done.

Copy link
Contributor

@poonai poonai left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just fix my comment and get one approval from @manishrjain before merge

Reviewable status: 0 of 9 files reviewed, 6 unresolved discussions (waiting on @arijitAD, @balajijinnah, @golangcibot, @manishrjain, @martinmr, and @pawanrawal)


gql/parser_test.go, line 4496 at r3 (raw file):

	require.Equal(t, "lastname", orders[1].Attr)
	require.Equal(t, []string{"ci"}, orders[1].Langs)
	require.Equal(t, "salary", orders[2].Attr)

Add a test for failing case


types/sort_test.go, line 175 at r3 (raw file):

	// Don't care about relative ordering between types. However, like types
	// should be sorted with each other.

Add case for language sorting

Copy link
Author

@arijitAD arijitAD left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 9 files reviewed, 6 unresolved discussions (waiting on @balajijinnah, @golangcibot, @manishrjain, @martinmr, and @pawanrawal)


gql/parser_test.go, line 4496 at r3 (raw file):

Previously, balajijinnah (balaji) wrote…

Add a test for failing case

Done.


types/sort_test.go, line 175 at r3 (raw file):

Previously, balajijinnah (balaji) wrote…

Add case for language sorting

Done.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm: Few comments.

Reviewed 1 of 11 files at r1, 3 of 13 files at r2, 3 of 5 files at r3, 3 of 3 files at r4.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @arijitAD, @balajijinnah, @golangcibot, @martinmr, and @pawanrawal)


query/query2_test.go, line 870 at r4 (raw file):

	js := processQueryNoErr(t, query)
	require.JSONEq(t,
		`{ "data": { "q": [ { "name_lang@de": "öffnen", "name_lang@sv": "zon" }, { "name_lang@de": "zumachen", "name_lang@sv": "öppna0" } ] } }`,

100 chars.


types/sort.go, line 102 at r4 (raw file):

	var cl *collate.Collator
	if lang != "" {
		langTag, _ := language.Parse(lang)

Always handle the error.

If the func is returning error, then return the error. Otherwise, if you're sure that the error shouldn't trigger, then call x.Check. Otherwise, make the func return the error.


worker/sort.go, line 692 at r4 (raw file):

	var lang string
	if langCount := len(order.Langs); langCount > 0 {
		x.AssertTruef(langCount == 1, "Sorting on multiple language is not supported.")

Just return the error.

Copy link
Author

@arijitAD arijitAD left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 of 9 files reviewed, 9 unresolved discussions (waiting on @balajijinnah, @golangcibot, @manishrjain, @martinmr, and @pawanrawal)


query/query2_test.go, line 870 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

100 chars.

Done.


types/sort.go, line 102 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Always handle the error.

If the func is returning error, then return the error. Otherwise, if you're sure that the error shouldn't trigger, then call x.Check. Otherwise, make the func return the error.

Done.


worker/sort.go, line 692 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Just return the error.

Done.

@arijitAD arijitAD merged commit b58c077 into master Nov 21, 2019
@arijitAD arijitAD deleted the arijitAD/nonindexed_sorting branch November 21, 2019 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Values are not sorted properly
4 participants