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 passing GraphQL schema to bulk loader. #5509

Merged
merged 5 commits into from
May 26, 2020
Merged

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented May 23, 2020

Support passing a GraphQL schema file to the bulk loader. The bulk loader will generate
the triples relevant triples expected by Dgraph.

Related to DGRAPH-1283


This change is Reviewable

Docs Preview: Dgraph Preview

@github-actions github-actions bot added the area/bulk-loader Issues related to bulk loading. label May 23, 2020
@martinmr
Copy link
Contributor Author

I tested this manually and it works. I will work on automated tests first thing next week.

Copy link
Contributor Author

@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: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)


dgraph/cmd/bulk/reduce.go, line 61 at r1 (raw file):

	x.AssertTrue(len(r.opt.shardOutputDirs) == r.opt.ReduceShards)

	r.dbs = make([]*badger.DB, r.opt.ReduceShards)

Ignore the changes to this file. I thought I needed to change the reduce phase at first. I will revert these changes.

Copy link

@vvbalaji-dgraph vvbalaji-dgraph 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 3 files reviewed, 2 unresolved discussions (waiting on @manishrjain, @martinmr, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

a discussion (no related file):

I will work on automated tests first thing next week.
:lgtm


Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @manishrjain, @martinmr, and @MichaelJCompton)


dgraph/cmd/bulk/loader.go, line 275 at r1 (raw file):

	x.Check(err)
	if filepath.Ext(ld.opt.GqlSchemaFile) == ".gz" {
		r, err = gzip.NewReader(r)

Do you need to defer close this reader too?

Copy link
Contributor Author

@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: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

a discussion (no related file):

Previously, vvbalaji-dgraph (V V Balaji) wrote…

I will work on automated tests first thing next week.
:lgtm

Done. I have added the tests.



dgraph/cmd/bulk/loader.go, line 275 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Do you need to defer close this reader too?

io.Reader does not have a close method. I think it's safe. The file reader is being closed.

@martinmr martinmr merged commit cc97957 into master May 26, 2020
@martinmr martinmr deleted the martinmr/bulk-gql branch May 26, 2020 23:07
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
Support passing a GraphQL schema file to the bulk loader. The bulk loader will generate
the triples relevant triples expected by Dgraph.

Related to DGRAPH-1283
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bulk-loader Issues related to bulk loading.
Development

Successfully merging this pull request may close these issues.

3 participants