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

XLOOKUP function #1469

Open
wants to merge 47 commits into
base: develop
Choose a base branch
from
Open

XLOOKUP function #1469

wants to merge 47 commits into from
+1,616 −124

Conversation

sequba
Copy link
Contributor

@sequba sequba commented Dec 5, 2024 •
edited
Loading

Context

  • merge PR by @selimyoussry
  • add more unit tests for XLOOKUP including several edge cases (total: 109 unit tests for XLOOKUP)
  • adjust the implementation to meet the criteria specified by the tests
  • add changelog entry
  • add entry to the built-in functions page

How did you test your changes?

  • linter passed
  • 109 new unit tests for different scenarios of XLOOKUP
  • all unit tests passed

Types of changes

  • Breaking change (a fix or a feature because of which an existing functionality doesn't work as expected anymore)
  • New feature or improvement (a non-breaking change that adds functionality)
  • Bug fix (a non-breaking change that fixes an issue)
  • Additional language file, or a change to an existing language file (translations)
  • Change to the documentation

Related issues:

Fixes #1458

Checklist:

  • I have reviewed the guidelines about Contributing to HyperFormula and I confirm that my code follows the code style of this project.
  • I have signed the Contributor License Agreement.
  • My change is compliant with the OpenDocument standard.
  • My change is compatible with Microsoft Excel.
  • My change is compatible with Google Sheets.
  • I described my changes in the CHANGELOG.md file.
  • My changes require a documentation update.
  • My changes require a migration guide.

selimyoussry and others added 16 commits June 15, 2024 13:39
@selimyoussry
ISS-1 sapiologie/hyperformula XLOOKUP in built-in-functions.md
932e8c0
@selimyoussry
ISS-1 sapiologie/hyperformula add translations
f0f9545
@selimyoussry
ISS-1 sapiologie/hyperformula placeholder function and test
384bcb8
@selimyoussry
ISS-1 sapiologie/hyperformula progress on Xlookup logic
eecce3a
@selimyoussry
ISS-1 sapiologie/hyperformula Working XLOOKUP without support for opt…
e9cabd6 
…ional parameters
@selimyoussry
ISS-1 sapiologie/hyperformula override default if_not_found
83be86c
@selimyoussry
ISS-1 sapiologie/hyperformula add commented out range function return…
5101a90 
… test
@selimyoussry
ISS-1 sapiologie/hyperformula update the doc to reflect limitations
08445d1
@selimyoussry
ISS-1 sapiologie/hyperformula Set vectorizationForbidden: true on XLO…
9ece67c 
…OKUP
@sequba
Add test suite for the XLOOKUP function
9c448c2
@sequba
Plan basic tests for XLOOKUP
167cac8
@sequba
Merge branch 'iss-1-xlookup' of github.com:sapiologie/hyperformula in…
1f9d091 
…to sapiologie-iss-1-xlookup
@sequba
Merge branch 'sapiologie-iss-1-xlookup' into feature/issue-1458
0811866
@sequba
Make XLOOKUP work in basic mode
8699958
@sequba
Make XLOOKUP return a range
13b9cc0
@sequba
Add unit tests about the types of argument
d3c6a26
@sequba sequba linked an issue Dec 5, 2024 that may be closed by this pull request
Add XLOOKUP function #1458
Open
@sequba sequba self-assigned this Dec 5, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 5, 2024 •
edited
Loading

Performance comparison of head (e6d8013) vs base (aec974e)

                                     testName |   base |   head | change
------------------------------------------------------------------------
                                      Sheet A | 520.12 | 532.78 | +2.43%
                                      Sheet B | 171.12 | 174.42 | +1.93%
                                      Sheet T |  147.6 | 148.78 | +0.80%
                                Column ranges | 522.76 | 521.97 | -0.15%
Sheet A:  change value, add/remove row/column |  15.28 |  14.32 | -6.28%
 Sheet B: change value, add/remove row/column | 132.72 | 140.54 | +5.89%
                   Column ranges - add column | 157.01 | 161.13 | +2.62%
                Column ranges - without batch | 448.96 | 475.88 | +6.00%
                        Column ranges - batch | 112.96 | 114.32 | +1.20%

@sequba sequba marked this pull request as ready for review January 2, 2025 11:09
@sequba sequba requested a review from budnix January 2, 2025 11:26
budnix
budnix requested changes Jan 2, 2025
View reviewed changes
Copy link
Member

@budnix budnix left a comment

Choose a reason for hiding this comment

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

I'm not an expert in HF but it looks good to me. I only found a typo that worth to fix.

src/Lookup/AdvancedFind.ts Outdated Show resolved Hide resolved
src/Lookup/ColumnIndex.ts Outdated Show resolved Hide resolved
src/Lookup/ColumnIndex.ts Outdated Show resolved Hide resolved
src/Lookup/ColumnIndex.ts Outdated Show resolved Hide resolved
src/Lookup/ColumnIndex.ts Outdated Show resolved Hide resolved
src/Lookup/SearchStrategy.ts Outdated Show resolved Hide resolved
@sequba sequba requested a review from budnix January 2, 2025 16:11
@codecov Codecov
Copy link

codecov bot commented Jan 2, 2025

Codecov Report

Attention: Patch coverage is 97.27891% with 4 lines in your changes missing coverage. Please review.

Project coverage is 97.21%. Comparing base (aec974e) to head (e6d8013).

Files with missing lines Patch % Lines
src/interpreter/binarySearch.ts 91.30% 2 Missing ⚠️
src/Lookup/AdvancedFind.ts 97.29% 1 Missing ⚠️
src/Lookup/ColumnIndex.ts 92.30% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1469   +/-   ##
========================================
  Coverage    97.20%   97.21%           
========================================
  Files          169      169           
  Lines        14430    14527   +97     
  Branches      3027     3082   +55     
========================================
+ Hits         14027    14122   +95     
- Misses         403      405    +2
Files with missing lines Coverage Δ
src/DependencyGraph/TopSort.ts 100.00% <ø> (ø)
src/Lookup/SearchStrategy.ts 100.00% <ø> (ø)
src/i18n/languages/csCZ.ts 100.00% <ø> (ø)
src/i18n/languages/daDK.ts 100.00% <ø> (ø)
src/i18n/languages/deDE.ts 100.00% <ø> (ø)
src/i18n/languages/enGB.ts 100.00% <ø> (ø)
src/i18n/languages/esES.ts 100.00% <ø> (ø)
src/i18n/languages/fiFI.ts 100.00% <ø> (ø)
src/i18n/languages/frFR.ts 100.00% <ø> (ø)
src/i18n/languages/huHU.ts 100.00% <ø> (ø)
... and 12 more

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

@budnix budnix Awaiting requested review from budnix

Requested changes must be addressed to merge this pull request.

Assignees

@sequba sequba

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add XLOOKUP function
3 participants
@sequba @budnix @selimyoussry