Skip to content

Comments

Hotfix/optional leaf object#1865

Open
oytuntez wants to merge 11 commits intodocarray:mainfrom
MotaWord:hotfix/optional-leaf-object
Open

Hotfix/optional leaf object#1865
oytuntez wants to merge 11 commits intodocarray:mainfrom
MotaWord:hotfix/optional-leaf-object

Conversation

@oytuntez
Copy link

@oytuntez oytuntez commented Feb 14, 2024

This PR aims to improve DocList fields that are Optional or that are given None default value. Giving empty [] as default value would create entries in the subindex (with just the ID field, no other data), so I opted for None.

The biggest issue that this PR aims to solve is DocList type fields would be required no matter if they have a default value or are marked as Optional.

JoanFM
JoanFM requested changes Feb 14, 2024
View reviewed changes
Copy link
Member

@JoanFM JoanFM 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 the PR, is there a chance for you to add tests for the fixes? Also, I see code related to ES, does it belong to the same problem?

@codecov
Copy link

codecov bot commented Feb 14, 2024 •
edited
Loading

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (7c1e18e) 84.68% compared to head (0a7e030) 84.67%.
Report is 2 commits behind head on main.

❗ Current head 0a7e030 differs from pull request most recent head 7555eb3. Consider uploading reports for the commit 7555eb3 to get more accurate results

Files Patch % Lines
docarray/index/backends/elastic.py 66.66% 1 Missing ⚠️
docarray/index/backends/hnswlib.py 75.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1865      +/-   ##
==========================================
- Coverage   84.68%   84.67%   -0.02%     
==========================================
  Files         136      136              
  Lines        9261     9268       +7     
==========================================
+ Hits         7843     7848       +5     
- Misses       1418     1420       +2
Flag Coverage Δ
docarray 84.67% <87.50%> (-0.02%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JohannesMessner JohannesMessner self-requested a review February 14, 2024 20:57
@JohannesMessner
Copy link
Member

JohannesMessner commented Feb 14, 2024

+1 for adding a test, once there is a test I could take over for the review if you want @JoanFM

@JoanFM
Copy link
Member

JoanFM commented Feb 14, 2024

+1 for adding a test, once there is a test I could take over for the review if you want @JoanFM

Perfect!

@JoanFM
Copy link
Member

JoanFM commented Feb 16, 2024

Also @oytuntez, please be aware that we will need to have all the commits signed-off for us to merge the contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JoanFM JoanFM JoanFM requested changes

@JohannesMessner JohannesMessner Awaiting requested review from JohannesMessner

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

@oytuntez @JohannesMessner @JoanFM