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

feat: partitioned index #225

Merged
merged 1 commit into from
Nov 15, 2019
Merged

feat: partitioned index #225

merged 1 commit into from
Nov 15, 2019

Conversation

jannyHou
Copy link
Contributor

@jannyHou jannyHou commented Nov 9, 2019 •
edited
Loading

Description

connect to #216
Support create partitioned index in partitioned database.

User can define the index in model config as

Product = db.define('Product', {
      prodName: {type: String},
      desc: {type: String},
    }, {
      indexes: {
        'prodName_index': {
         // Partitioned index
          partitioned: true,
          keys: {
            prodName: 1,
          },
        },
      },
    });

to create partitioned index.
If no partitioned property defined, the default index will be global.

TODO:

Related issues

  • connect to <link_to_referenced_issue>

Checklist

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

@jannyHou jannyHou requested a review from virkt25 as a code owner November 9, 2019 21:36
@jannyHou jannyHou mentioned this pull request Nov 11, 2019
Merged
2 tasks
@jannyHou jannyHou changed the title [WIP]feat: partitioned index feat: partitioned index Nov 11, 2019
dhmlau
dhmlau reviewed Nov 13, 2019
View reviewed changes
lib/migrate.js Outdated
* e.g. {fields: [{"foo": "asc"}, {"bar": "asc"}], partitioned: true}
* @param {Function} cb
*/
Cloudant.prototype.__createIndex = function(mo, model, name, indexObj, cb) {
Copy link
Member

Choose a reason for hiding this comment

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

similar to comment from @bajtos, it should probably be single _ in the function name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 renamed them to single _

@jannyHou jannyHou requested a review from emonddr as a code owner November 13, 2019 16:24
agnes512
agnes512 reviewed Nov 14, 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.

I am not familiar with detailed implementation of Cloudant, but it makes sense to me high -level-ly 😄

lib/migrate.js
};

/**
* Perform the indexes comparison for `autoupdate`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this function for comparing if indexes exist?

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 yep, it returns the new indexes to add and old indexes to drop

test/partition.test.js Outdated
});
});

it('configured true - create partitioned index for index entry', (done) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like a test title like create partitioned index for index entry with partition enabled would make the intention more clear. Same for configured false test. ( Unless the option we enable/disable partitioned called configured)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed all the tests title :)

test/partition.test.js
Comment on lines +84 to +85
index.def.fields[0]['prodName'].should.equal('desc');
index.def.fields[1][DEFAULT_MODEL_VIEW].should.equal('desc');
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not familiar with Cloudant index field. Why is this desc and others are asc?

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 Good question, see https://github.com/strongloop/loopback-connector-cloudant/pull/225/files#diff-121c1d00e6380aa6109385e1df1ae803R66

The order code specified for prodName is -1, which stands for desc

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like they need a better name 😆

dhmlau
dhmlau reviewed Nov 15, 2019
View reviewed changes
Copy link
Member

@dhmlau dhmlau left a comment

Choose a reason for hiding this comment

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

I mainly looked at the tests and had 2 comments.

test.js
@@ -37,7 +38,8 @@ async.waterfall([
setCloudantEnv,
waitFor('/_all_dbs'),
createAdmin(),
createDB('test-db'),
createDB(process.env.CLOUDANT_DATABASE),
createPartitionedDB(process.env.CLOUDANT_PARTITIONED_DATABASE),
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't seem to be called in other places?

Copy link
Member

Choose a reason for hiding this comment

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

@jannyHou showed me where it's being used.

test/partition.test.js
prodName: {type: String},
desc: {type: String},
}, {
indexes: {
Copy link
Member

Choose a reason for hiding this comment

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

not really about this test case.. Do we have any tests to show after we've created a database using the partitioned:true option, the indexes look like this?

Copy link
Member

Choose a reason for hiding this comment

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

Discussed it with @jannyHou. This test case is about if someone specify an indexes for the product, it should be preserved after putting it into the database.
For the partitioned:true "property", it is at the global level, so if we don't specify any indexes, there will be no difference for the indexes between a partitioned database and a non-partitioned database.

dhmlau
dhmlau approved these changes Nov 15, 2019
View reviewed changes
Copy link
Member

@dhmlau dhmlau left a comment

Choose a reason for hiding this comment

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

Discussed with @jannyHou about my comment and summarized in #225 (comment). It's looking good from the test perspective. so I trust the code is functional.

@jannyHou jannyHou merged commit 22b6137 into master Nov 15, 2019
@delete-merged-branch delete-merged-branch bot deleted the partition/index branch November 15, 2019 18:47
@jannyHou jannyHou mentioned this pull request Nov 15, 2019
Closed
1 task
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 dhmlau approved these changes

@virkt25 virkt25 Awaiting requested review from virkt25

@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.
3 participants
@jannyHou @dhmlau @agnes512