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.

Sign up for GitHub

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

Jump to bottom

Partition field #230

Merged
merged 1 commit into from
Nov 22, 2019
Merged

Partition field #230

merged 1 commit into from
Nov 22, 2019

Conversation

jannyHou
Copy link
Contributor

@jannyHou jannyHou commented Nov 20, 2019 •
edited
Loading

Description

connect to #217

  • You can specify a partition key field in model definition like
customers = db.define('customers', {
  userId: {type: Number, id: true},
  // partition key field
  countryCode: {type: String, isPartitionKey: true},
  name: String,
  zipCode: Number,
});

PR loopbackio/loopback-connector-couchdb2#72 adds the name of partition key field to couchdb/cloudant's model object.

  • When the partition key field is included in query, Cloudant.prototype.all will trigger parititonFind from driver instead of a global search.

  • The partition key provided in options will override the one in query.

Related issues

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

@jannyHou jannyHou mentioned this pull request Nov 20, 2019
Merged
4 tasks
agnes512
agnes512 reviewed Nov 21, 2019
View reviewed changes
Copy link
Contributor

@agnes512 agnes512 left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of the partitionKey feature! I left a few comments.

test/partition.test.js
});
resultTaggedFood.length.should.equal(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't it equal to 3?

test/partition.test.js
(err, results) => {
if (err) return done(err);
should.exist(results);
results.length.should.equal(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't it equal to 3? and what's the difference between find all records by partition key from option and find all records by partition key from property? Does it mean we can invoke the partition key from property or option, and they they give the same result?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems the tests are confusing, let me add more docs for it.

agnes512
agnes512 reviewed Nov 21, 2019
View reviewed changes
test/partition.test.js
{partitionKey: 'london'}, (err, results) => {
if (err) return done(err);
should.exist(results);
results.length.should.equal(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean the options overrides the query clause?

And if it does, why isn't it result to 2? ( food and london)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@agnes512

Do you mean the options overrides the query clause?

Yes, it's 0 because when search within partition london, there is no record with city as toronto. See the sample records created in https://github.com/strongloop/loopback-connector-cloudant/pull/230/files#diff-121c1d00e6380aa6109385e1df1ae803R237

jannyHou
jannyHou commented Nov 21, 2019
View reviewed changes
lib/cloudant.js
Cloudant.prototype._getPartitionKey = function(mo, query, options) {
const pkFromOption = options && options.partitionKey;
debug('_getPartitionKey pkFromOption %s', pkFromOption);
if (pkFromOption) return pkFromOption;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if option has partitionKey, return immediately.

agnes512
agnes512 approved these changes Nov 21, 2019
View reviewed changes
test/partition.test.js Outdated
@@ -223,6 +229,10 @@ describe('cloudant - partitioned db', () => {
});

it('partition key in options overrides property for find all', (done) => {
// Find records with partition `toronto`, tag `food`, city `toronto`,
Copy link
Contributor

Choose a reason for hiding this comment

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

with partition toronto -> with partition london?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch !

test/partition.test.js
@@ -235,11 +245,18 @@ describe('cloudant - partitioned db', () => {
});

const SEED_DATA = [
// partition `toronto`, tag `drink`
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 the comments are helpful

@jannyHou jannyHou merged commit 7ebabc9 into master Nov 22, 2019
@delete-merged-branch delete-merged-branch bot deleted the partition-field branch November 22, 2019 19:33
dhmlau added a commit that referenced this pull request Jan 27, 2020
@dhmlau
2.4.0
d62a37d 
 * chore: update copyright year (Agnes Lin)
 * replace couchdb3 docker img with a stable version (Agnes Lin)
 * docs: add partition document (#232) (Janny)
 * fixup!: fix the dependency version (#233) (Janny)
 * feat: query with partition field (#230) (Janny)
 * chore: improve issue and PR templates (Nora)
 * feat: add partitioned find (#229) (Janny)
 * add docker setup script (#227) (Janny)
 * feat: partitioned index (#225) (Janny)
 * fix CODEOWNERS file (Diana Lau)
 * update docker image (#224) (Janny)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agnes512 agnes512 agnes512 approved these changes

@dhmlau dhmlau Awaiting requested review from dhmlau dhmlau is a code owner

@emonddr emonddr Awaiting requested review from emonddr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jannyHou @agnes512