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

feat: CLI commands for secondary indexes #1595

Merged
merged 35 commits into from
Jul 4, 2023

Conversation

islamaliev
Copy link
Contributor

@islamaliev islamaliev commented Jun 27, 2023

Relevant issue(s)

Resolves #1553

Description

Adds following CLI commands:

  • client index create that creates a new index on an existing collection
  • client index drop that drop a named indexes on a collection
  • client index list that lists all existing indexes in db or indexes for a specific collection

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Unit test, integration tests, manual tests

Specify the platform(s) on which this was tested:

  • MacOS

@islamaliev islamaliev self-assigned this Jun 27, 2023
@islamaliev islamaliev added the area/cli Related to the CLI binary label Jun 27, 2023
@islamaliev islamaliev added this to the DefraDB v0.6 milestone Jun 27, 2023
@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Patch coverage: 82.64% and project coverage change: +0.03 🎉

Comparison is base (5f4beb9) 73.64% compared to head (8b1cea3) 73.67%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1595      +/-   ##
===========================================
+ Coverage    73.64%   73.67%   +0.03%     
===========================================
  Files          188      193       +5     
  Lines        19608    19953     +345     
===========================================
+ Hits         14439    14699     +260     
- Misses        4104     4160      +56     
- Partials      1065     1094      +29     
Flag Coverage Δ
all-tests 73.67% <82.64%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cli/errors.go 14.29% <0.00%> (+6.59%) ⬆️
db/txn_db.go 50.85% <50.00%> (-0.05%) ⬇️
cli/index_drop.go 70.83% <70.83%> (ø)
logging/config.go 95.83% <75.00%> (ø)
cli/index_list.go 75.41% <75.41%> (ø)
cli/index_create.go 76.00% <76.00%> (ø)
api/http/handlerfuncs_index.go 100.00% <100.00%> (ø)
api/http/router.go 100.00% <100.00%> (ø)
cli/cli.go 72.55% <100.00%> (+2.02%) ⬆️
cli/index.go 100.00% <100.00%> (ø)
... and 2 more

... and 7 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f4beb9...8b1cea3. Read the comment docs.

@islamaliev islamaliev force-pushed the feat/islam/cli-secondary-index branch from aa68928 to c620989 Compare June 27, 2023 16:00
Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

Good work Islam! I do have a few things that I'd like to see changed and discussed as you'll see bellow.

api/http/handlerfuncs.go Outdated Show resolved Hide resolved
api/http/router.go Outdated Show resolved Hide resolved
cli/index.go Outdated Show resolved Hide resolved
api/http/handlerfuncs.go Outdated Show resolved Hide resolved
cli/index_create.go Outdated Show resolved Hide resolved
cli/index_create.go Outdated Show resolved Hide resolved
api/http/handlerfuncs.go Outdated Show resolved Hide resolved
cli/index_drop.go Outdated Show resolved Hide resolved
cli/index_list.go Outdated Show resolved Hide resolved
@islamaliev islamaliev force-pushed the feat/islam/cli-secondary-index branch from c97a417 to d781503 Compare June 28, 2023 13:20
@islamaliev islamaliev requested a review from fredcarle June 28, 2023 13:20
Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

Good improvements Islam. I have a few more things before I approve and then we should be good to go. I'm sorry if some of the comments are repetitive. I used some copy pasting to save me time :)

api/http/handlerfuncs_index.go Outdated Show resolved Hide resolved
api/http/handlerfuncs_index.go Outdated Show resolved Hide resolved
api/http/handlerfuncs_index.go Outdated Show resolved Hide resolved
cli/index_create.go Outdated Show resolved Hide resolved
cli/index_drop.go Outdated Show resolved Hide resolved
tests/integration/cli/client_index_list_test.go Outdated Show resolved Hide resolved
tests/integration/cli/client_index_list_test.go Outdated Show resolved Hide resolved
tests/integration/cli/client_index_list_test.go Outdated Show resolved Hide resolved
tests/integration/cli/client_index_list_test.go Outdated Show resolved Hide resolved
tests/integration/cli/client_index_list_test.go Outdated Show resolved Hide resolved
cli/index_create.go Outdated Show resolved Hide resolved
cli/index_create.go Show resolved Hide resolved
cli/index_create_test.go Outdated Show resolved Hide resolved
cli/index_drop.go Show resolved Hide resolved
cli/index_list.go Show resolved Hide resolved
@islamaliev islamaliev force-pushed the feat/islam/cli-secondary-index branch from fe23768 to 12cc0a8 Compare July 4, 2023 15:25
Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

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

Some nitpicks about formatting, :P might be considered personal pref though. One minor todo.

Approving as all else LGTM =)

cli/index_create_test.go Outdated Show resolved Hide resolved
cli/index_create_test.go Outdated Show resolved Hide resolved
cli/index_create_test.go Outdated Show resolved Hide resolved
cli/index_create_test.go Outdated Show resolved Hide resolved
cli/index_create_test.go Outdated Show resolved Hide resolved
tests/integration/cli/client_index_drop_test.go Outdated Show resolved Hide resolved
tests/integration/cli/client_index_drop_test.go Outdated Show resolved Hide resolved
tests/integration/cli/client_index_drop_test.go Outdated Show resolved Hide resolved
tests/integration/cli/client_index_drop_test.go Outdated Show resolved Hide resolved
tests/integration/cli/client_index_list_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

LGTM. Note that I do agree with Shahzad that changing the formatting of the command flags would help readability.

@islamaliev islamaliev merged commit cd014c3 into develop Jul 4, 2023
@islamaliev islamaliev deleted the feat/islam/cli-secondary-index branch July 4, 2023 16:36
@fredcarle fredcarle added the feature New feature or request label Jul 17, 2023
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
## Relevant issue(s)

Resolves sourcenetwork#1553 

## Description

Adds following CLI commands:
- `client index create` that creates a new index on an existing
collection
- `client index drop` that drop a named indexes on a collection
- `client index list` that lists all existing indexes in db or indexes
for a specific collection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli Related to the CLI binary feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sec. Indexes: CLI commands for creating and dropping indexes
3 participants