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

Corrected function handling when no language is specified. #1309

Closed
wants to merge 8 commits into from

Conversation

tzdybal
Copy link
Contributor

@tzdybal tzdybal commented Aug 4, 2017

This is a fix for #1295.


This change is Reviewable

@manishrjain
Copy link
Contributor

:lgtm: Just make sure that the @. is the tag required for picking any lang.


Reviewed 2 of 4 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


wiki/content/query-language/index.md, line 178 at r1 (raw file):

{{% notice "note" %}}In functions, language lists and `.` notation are not allowed.  In functions, a string edge without a language tag means apply function to all languages, while a single language tag means apply to only the given language.{{% /notice %}}

{{% notice "note" %}}In case of full text search functions (`alloftext`, `anyoftext`), when no language is specified default (English) Full Text Search tokenizer is used.{{% /notice %}}

is specified, (comma) default ...


Comments from Reviewable

@tzdybal
Copy link
Contributor Author

tzdybal commented Aug 7, 2017

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


wiki/content/query-language/index.md, line 178 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

is specified, (comma) default ...

Done.


Comments from Reviewable

@pawanrawal
Copy link
Contributor

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks broke.


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

	// this case is not trivial, because farmhash of "en" is less than farmhash of "fr"
	// so we need to iterate over values in all languages to find a match
	// for alloftext, this won't work - we use default/English tokenizer for function parameters,

we use default/English tokenizer when no language is specified.


worker/stringfilter.go, line 91 at r2 (raw file):

		}
		return false
	} else {

Can get rid of this else, since you are returning in the if above.
Actually you can handle the smaller len(filter.eqVals) == 0 block first and then the bigger block doesn't need to be indented.


worker/task.go, line 502 at r2 (raw file):

	// For string matching functions, check the language.
	if (srcFn.fnType == StandardFn || srcFn.fnType == HasFn ||

This if condition probably needs its own function?


worker/task.go, line 664 at r2 (raw file):

					return false
				default:
					langs := []string{arg.srcFn.lang}

What happens when user gives something like name@en:hi:. in functions. Do we give an error or just parse the first language?


Comments from Reviewable

@tzdybal
Copy link
Contributor Author

tzdybal commented Aug 7, 2017

Review status: 2 of 6 files reviewed at latest revision, 5 unresolved discussions.


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

Previously, pawanrawal (Pawan Rawal) wrote…

we use default/English tokenizer when no language is specified.

Done


worker/stringfilter.go, line 91 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Can get rid of this else, since you are returning in the if above.
Actually you can handle the smaller len(filter.eqVals) == 0 block first and then the bigger block doesn't need to be indented.

Cyclomatic complexity reduced.


worker/task.go, line 502 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This if condition probably needs its own function?

Good idea. Function introduced.


worker/task.go, line 664 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

What happens when user gives something like name@en:hi:. in functions. Do we give an error or just parse the first language?

We return error, it's tested in gql/parser_test.go in TestLangsFunctionMultipleLangs.


Comments from Reviewable

@pawanrawal
Copy link
Contributor

:lgtm:


Reviewed 1 of 4 files at r1, 1 of 4 files at r2, 4 of 4 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@tzdybal
Copy link
Contributor Author

tzdybal commented Aug 7, 2017

Merged to master.

@tzdybal tzdybal closed this Aug 7, 2017
@pawanrawal pawanrawal deleted the fix/lang_eq_hash branch December 19, 2017 08:40
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.

3 participants