apply filterStringFunction when customIndexFn #3992
Conversation
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ A review job has been created and sent to the PullRequest network.
@sjhewitt you can click here to see the review status or cancel the code review job.
|
|
There was a problem hiding this comment.
I've left an inline comment regarding bounds checks in the customIndexedFn.
Reviewed with ❤️ by PullRequest
| case customIndexFn: | ||
| filter.tokens = arg.srcFn.tokens | ||
| filter.match = defaultMatch | ||
| filter.tokName = arg.q.SrcFunc.Args[0] |
There was a problem hiding this comment.
Would len(arg.q.SrcFunc.Args) be valuable here to guard against indexing this when nothing is present, or simply overkill?
There was a problem hiding this comment.
I figured this has already been checked by the parseSrcFn and must be valid in order to reach this point
There was a problem hiding this comment.
Thanks for your PR. It looks good but I'll let @pawanrawal take another look since he's more familiar with Dgraph's query code.
Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manishrjain and @pullrequest[bot])
worker/task.go, line 1405 at r1 (raw file):
Previously, pullrequest[bot] wrote…
Would
len(arg.q.SrcFunc.Args)be valuable here to guard against indexing this when nothing is present, or simply overkill?
Let's add a check just to be sure. Just return an error if Args is nil or of length 0.
There was a problem hiding this comment.
Nice work @sjhewitt. Thanks for your contribution.
The PR looks good but the test case result doesn't look right. Also noticed that these tests are not being skipped in CI https://teamcity.dgraph.io/viewLog.html?tab=buildLog&logTab=tree&filter=debug&expand=all&buildId=23939&_focus=15583. @danielmai could you please have a look why that is the case?
Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @manishrjain, @pullrequest[bot], and @sjhewitt)
systest/plugin_test.go, line 150 at r1 (raw file):
}}`, ` { "q": [ { "word": "marine" }
Shouldn't this return airmen?
There was a problem hiding this comment.
⚠️ Warning
PullRequest detected a force-push on this branch. This may have caused some information to be lost, and additional time may be required to complete review of the code. Read More
There was a problem hiding this comment.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @manishrjain, @martinmr, @pawanrawal, @pullrequest[bot], and @sjhewitt)
Line 58 in c6ecca1 Lines 198 to 201 in c6ecca1 We've always been setting I'll open a PR to fix test script for CI. |
As mentioned in #3992, the plugin tests were being skipped when running tests via ./test.sh. This is because some tests were being skipped when the -short=true flag is set for go test. * The -short=true setting is now an option for test.sh, settable via ./test.sh --short. It is not set by default, so short tests are not skipped. * Update usage text with all the current flags. * Rename the test tag :full: to :systest: since systest tests are what gets run.
|
@pawanrawal looks like the CI tests pass now |
fix for #3991
This change is