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

Support for Gql variable in arrays #2981

Merged
merged 6 commits into from
Feb 14, 2019
Merged

Conversation

srfrog
Copy link
Contributor

@srfrog srfrog commented Feb 6, 2019

This change adds support for Gql variables in array arguments of inequality functions.
Argument values are still handled.

Example with variables:

query q($name1:string = "Ridley Scott", $name2:string = "Steven Spielberg") {
  me(func: eq(name@en, [$name1, $name2])) {
    name@en
    director.film @filter(lt(initial_release_date, "1980-01-01"))  {
      initial_release_date
      name@en
    }
  }
}

Mixed use:

query q($name1:string = "Ridley Scott") {
  me(func: eq(name@en, [$name1, "Steven Spielberg"])) {
    name@en
    director.film @filter(lt(initial_release_date, "1980-01-01"))  {
      initial_release_date
      name@en
    }
  }
}

Typical use without variables, should yield same result:

{
  me(func: eq(name@en, ["Ridley Scott", "Steven Spielberg"])) {
    name@en
    director.film @filter(lt(initial_release_date, "1980-01-01"))  {
      initial_release_date
      name@en
    }
  }
}

Closes #2272


This change is Reviewable

This change adds support for Gql variables in array arguments in inequality functions.
Scalar arguments are still handled.
@srfrog srfrog requested a review from a team February 6, 2019 23:06
srfrog added 2 commits February 6, 2019 17:16
New tests TestParseGraphQLVarArray, TestParseGraphQLValueArray, and
TestParseGraphQLMixedVarArray check usage of variable arrays in
eq function.
Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r1.
Reviewable status: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @srfrog)


gql/parser.go, line 1346 at r1 (raw file):

// parseIneqArgs will try to parse the arguments inside an array ([]). If the values
// are prefixed with $ they are treated as Gql variables, otherwise used as scalar values.

otherwise they are used ...


gql/parser.go, line 1374 at r1 (raw file):

				}
				g.Args = append(g.Args, Arg{Value: val})
				break

It's unclear why we break here. Should this be continue?

If it's supposed to be a break statement, maybe add a short comment above explaining why.


gql/parser.go, line 1390 at r1 (raw file):

		isDollar = false
	}
	return it.Errorf("Expecting ] to end list but none was found")

return it.Errorf("Expecting ] to end list but got %v instead", it.Item().Val)


types/geofilter.go, line 25 at r1 (raw file):

	"github.com/golang/geo/s2"
	geom "github.com/twpayne/go-geom"

Package is already called geom so this is not doing anything.

https://github.com/twpayne/go-geom/blob/master/geom.go#L3

I doubt the package name will change on their end.

Copy link
Contributor Author

@srfrog srfrog 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: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @martinmr)


gql/parser.go, line 1346 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

otherwise they are used ...

Done.


gql/parser.go, line 1374 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

It's unclear why we break here. Should this be continue?

If it's supposed to be a break statement, maybe add a short comment above explaining why.

if we don't break we append the value twice.


gql/parser.go, line 1390 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

return it.Errorf("Expecting ] to end list but got %v instead", it.Item().Val)

Done.


types/geofilter.go, line 25 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Package is already called geom so this is not doing anything.

https://github.com/twpayne/go-geom/blob/master/geom.go#L3

I doubt the package name will change on their end.

The import location has a non-standard name, this is added by the formatter. It's not based on the package name.

@srfrog srfrog requested a review from a team February 11, 2019 18:10
Copy link
Contributor

@codexnull codexnull left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @martinmr)

Copy link
Contributor

@martinmr martinmr 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: 3 of 4 files reviewed, all discussions resolved

@srfrog srfrog merged commit a6b3d18 into master Feb 14, 2019
@srfrog srfrog deleted the srfrog/issue-2272_variable_arrays branch February 14, 2019 01:43
@F21
Copy link
Contributor

F21 commented Feb 14, 2019

Will this work in the case where we want to pass the array in as a variable?

query q($names:string = ["Ridley Scott",  "Steven Spielberg"]) {
  me(func: eq(name@en, $names)) {
    name@en
    director.film @filter(lt(initial_release_date, "1980-01-01"))  {
      initial_release_date
      name@en
    }
  }
}

I have a lot of queries that look like this, and having to dynamically generate the place holders: $name1, $name2, etc in mode makes the query hard to read and is not very nice to implement.

@MichelDiz
Copy link
Contributor

This won't work @F21 . And in the last version also don't work as far I can tell.
If you wanna test by yourself. Just install Go Lang and run:
PS. Only macOS or Linux.

  • go get github.com/dgraph-io/dgraph/...
  • cd ~/go/src/github.com/dgraph-io/dgraph/dgraph
  • make

After make you gonna have a binary with latest commits ready to use.

@F21
Copy link
Contributor

F21 commented Feb 14, 2019

@MichelDiz Should #2272 or #2726 be reopened? Those issues include support for passing a list as a variable to a query.

@MichelDiz
Copy link
Contributor

@F21 not sure. But if would necessary open sure would be only the #2726

@F21
Copy link
Contributor

F21 commented Jul 8, 2019

Is this supported on v1.0.15? I am using the following query, but it returns this error: Expected comma or language but got: id1:

query Roles($organizationID: string, $id1: string, $id2: string){
   role(func: has(role)) @filter(eq(organizationID, $organizationID) AND eq(id, [$id1, $id2])){
       uid
   }
}

@MichelDiz
Copy link
Contributor

I guess this is only in master. I couldn't find any RC with this commit. So it will be in 1.1.

dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
* gql/parser.go: add support for gql variable arrays

This change adds support for Gql variables in array arguments in inequality functions.
Scalar arguments are still handled.

* added comments, removed debug log statements, fixed typos

* gql/parser_test.go: added tests for ineq var arrays

New tests TestParseGraphQLVarArray, TestParseGraphQLValueArray, and
TestParseGraphQLMixedVarArray check usage of variable arrays in
eq function.

* added and fixed comments, and other formatting changes

* wiki/content/query-language/index.md: eq syntax with vars in array
@hamada969
Copy link

hamada969 commented Feb 17, 2020

Hey, any updates on the above ?

I am using Dgraph 1.1, the python version and I still can't seem to pass in a str joined list of uids in the variable section against the uid lookup:

query = f"""query nodes($uids: string) {{
                          nodes(func: uid($uids)){{
                            uid,
                            nodeType
                        }}
                    }}
                        """
        variables = {f"$uids": f"{','.join([str(uid) for uid in uid_list])}"}
        try:
            result = self._ro_txn(query, variables)

error:

Failed running query (StatusCode.UNKNOWN: strconv.ParseUint: parsing "0xdd6,0xdd6": invalid syntax). See logs for details.

@MichelDiz
Copy link
Contributor

@hamada969 this feels like a bug but you can do like:

query test($uids: string = "[0xf468, 0xf468, 0xf468, 0xf468, 0x4f408, 0x8e129]") {
  films(func: uid($uids)) {
    uid
    expand(_all_) {
      expand(_all_)
    }
  }
}

In our public dataset should return:

{
  "data": {
    "films": [
      {
        "uid": "0xf468",
        "performance.film": [
          {
            "initial_release_date": "2005-07-01T00:00:00Z",
            "netflix_id": "70040924"
          }
        ]
      },
      {
        "uid": "0xf468",
        "performance.film": [
          {
            "initial_release_date": "2005-07-01T00:00:00Z",
            "netflix_id": "70040924"
          }
        ]
      },
      {
        "uid": "0xf468",
        "performance.film": [
          {
            "initial_release_date": "2005-07-01T00:00:00Z",
            "netflix_id": "70040924"
          }
        ]
      },
      {
        "uid": "0xf468",
        "performance.film": [
          {
            "initial_release_date": "2005-07-01T00:00:00Z",
            "netflix_id": "70040924"
          }
        ]
      },
      {
        "uid": "0x4f408",
        "performance.film": [
          {
            "initial_release_date": "2002-01-01T00:00:00Z"
          }
        ]
      },
      {
        "uid": "0x8e129",
        "performance.film": [
          {
            "initial_release_date": "2013-06-01T00:00:00Z"
          }
        ]
      }
    ]
  }
}

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.

DQL variables don't seem to be supported in arrays
6 participants