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: Support for descending fields CLI index creation #3237

Merged
merged 15 commits into from
Nov 14, 2024

Conversation

ChrisBQu
Copy link
Collaborator

@ChrisBQu ChrisBQu commented Nov 12, 2024

Relevant issue(s)

Resolves #2460

Description

Using the HTTP endpoint, it was possible to create a collection index, with each field being either descending or ascending. This was possible because one of the parameters was a boolean field called "Descending." However, this functionality was not present in the CLI equivalent.

This feature adds support for creating descending fields.

For example, the following sorts name in ascending order, because ascension is the default

defradb client index create -c User --fields name

The following sorts it in descending order:

defradb client index create -c User --fields name:DESC

And the following sorts multiple fields, in different combinations of descending and ascending order:

defradb client index create -c User --fields name:ASC,score:DESC

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the pull request title adheres to the conventional commit style

How has this been tested?

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

  • Windows

@ChrisBQu ChrisBQu added the area/cli Related to the CLI binary label Nov 12, 2024
@ChrisBQu ChrisBQu added this to the DefraDB v0.15 milestone Nov 12, 2024
@ChrisBQu ChrisBQu self-assigned this Nov 12, 2024
@ChrisBQu ChrisBQu changed the title feat: Add support for ascending/descending fields on index creation in CLI feat: support for descending fields on index creation (CLI) Nov 12, 2024
@ChrisBQu ChrisBQu changed the title feat: support for descending fields on index creation (CLI) feat: Support for descending fields on index creation (CLI) Nov 12, 2024
Copy link

codecov bot commented Nov 12, 2024

Codecov Report

Attention: Patch coverage is 68.75000% with 10 lines in your changes missing coverage. Please review.

Project coverage is 77.49%. Comparing base (332f9d6) to head (1b181e1).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
cli/index_create.go 78.57% 5 Missing and 1 partial ⚠️
cli/errors.go 0.00% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3237      +/-   ##
===========================================
- Coverage    77.61%   77.49%   -0.12%     
===========================================
  Files          382      382              
  Lines        35236    35263      +27     
===========================================
- Hits         27345    27324      -21     
- Misses        6272     6308      +36     
- Partials      1619     1631      +12     
Flag Coverage Δ
all-tests 77.49% <68.75%> (-0.12%) ⬇️

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

Files with missing lines Coverage Δ
cli/errors.go 0.00% <0.00%> (ø)
cli/index_create.go 86.96% <78.57%> (-6.52%) ⬇️

... and 13 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 332f9d6...1b181e1. Read the comment docs.

@ChrisBQu ChrisBQu changed the title feat: Support for descending fields on index creation (CLI) feat: Support for descending fields CLI index creation Nov 12, 2024
@ChrisBQu
Copy link
Collaborator Author

todo: I will be adding integration tests for this.

Copy link
Contributor

@islamaliev islamaliev left a comment

Choose a reason for hiding this comment

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

Looks good. Tests would be great. Actually I don't know why there is no tests for this yet. I might have missed this case.

You can look into tests/integration/index/query_with_composite_index_field_order_test.go and tests/integration/index/query_with_index_only_field_order_test.go. Just copy&paste one of the tests and instead of including an index into testUtils.SchemaUpdate create another testUtils.CreateIndex with whatever params you deem appropriate.

Our integration tests will run for all 3 clients: go client, HTTP and CLI. So whatever action you write in integration tests will be translated to the "language" of these 3 clients. So you will have to change CreateIndex method in tests/clients/cli/wrapper_collection.go.

Copy link
Contributor

@islamaliev islamaliev left a comment

Choose a reason for hiding this comment

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

One thing that needs to be added is documentation. Please make sure you include the new change. Couldn't leave an inline comment as you didn't change that part. It's the description in cli/index_create.go.

After this you can run make docs to sync docs with other places.

Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

Looks really good Chris, solution is nice and simple, and easy to read. Am curious as to how nice/miserable you found writing that first integration test? Congrats on your first PR :)

I have a couple of minor suggestions for you, please respond to them before merging (your response can very much be 'no I think that is a bad idea because of xyz'), as well as making sure Islam and any other reviewers are of course happy :)

cli/index_create.go Outdated Show resolved Hide resolved
cli/index_create.go Outdated Show resolved Hide resolved
@ChrisBQu
Copy link
Collaborator Author

I discussed the way the CreateIndex function is written with Andy. Currently, it takes in an IndexDescription object. Because of this, the function has access to a boolean indicating whether or not the order should be descending. However, the function does not have access to any context indicating whether a false value means ":ASC" was explicitly passed in, or because nothing was passed in explicitly, and the default value (ascending order) was used.

cli/errors.go Outdated Show resolved Hide resolved
http/client_collection.go Outdated Show resolved Hide resolved
tests/clients/cli/wrapper_collection.go Show resolved Hide resolved
cli/index_create.go Outdated Show resolved Hide resolved
@ChrisBQu ChrisBQu requested a review from islamaliev November 14, 2024 15:33
Copy link
Contributor

@islamaliev islamaliev left a comment

Choose a reason for hiding this comment

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

LGTM! Good job, Chris.

@ChrisBQu ChrisBQu merged commit 909c4af into sourcenetwork:develop Nov 14, 2024
42 of 43 checks passed
@islamaliev
Copy link
Contributor

bug bash result: run different combinations of composite indexes with different directions. Everything work fine.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI and HTTP parameter mismatch for creating indexes.
4 participants