Skip to content

Provide FFI stubs and ignore flaky test#6860

Merged
chesedo merged 3 commits intomasterfrom
no_geo_filter_for_benchmarks
Sep 21, 2025
Merged

Provide FFI stubs and ignore flaky test#6860
chesedo merged 3 commits intomasterfrom
no_geo_filter_for_benchmarks

Conversation

@chesedo
Copy link
Collaborator

@chesedo chesedo commented Sep 19, 2025 •
edited
Loading

Describe the changes in the pull request

This provides the isWithinRadius and DocTable_Exists symbols for the inverted index benchmarks to allow it to build correctly on some systems for testing. For eg here alpine fails to link to it in CI. It also disables the flaky numeric FFI test

Mark if applicable

  • This PR introduces API changes
  • This PR introduces serialization changes

@chesedo chesedo marked this pull request as ready for review September 19, 2025 09:19
@chesedo chesedo requested a review from JoanFM September 19, 2025 09:19
JoanFM
JoanFM previously approved these changes Sep 19, 2025
View reviewed changes
@chesedo chesedo enabled auto-merge September 19, 2025 09:24
@JoanFM
Copy link
Collaborator

JoanFM commented Sep 19, 2025

I would like to understand better the cause though?

@JoanFM
Copy link
Collaborator

JoanFM commented Sep 19, 2025

Wouldn't it make more sense to link also against geo_index?

    link_static_libraries(&[
        ("src/inverted_index", "inverted_index"),
        ("src/buffer", "buffer"),
        ("src", "geo_index"), // Add this line
    ]);

@codecov
Copy link

codecov bot commented Sep 19, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.34%. Comparing base (ef5c927) to head (6288a2d).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #6860      +/-   ##
==========================================
- Coverage   86.36%   86.34%   -0.02%     
==========================================
  Files         288      288              
  Lines       46513    46513              
  Branches     9404     9404              
==========================================
- Hits        40172    40163       -9     
- Misses       6190     6199       +9     
  Partials      151      151
Flag Coverage Δ
flow 84.67% <ø> (-0.15%) ⬇️
unit 49.34% <ø> (ø)

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JonasKruckenberg
Copy link
Collaborator

JonasKruckenberg commented Sep 19, 2025

eagerly anticipating this so I can requeue my PR ;)

@chesedo
Copy link
Collaborator Author

chesedo commented Sep 19, 2025

Wouldn't it make more sense to link also against geo_index?

    link_static_libraries(&[
        ("src/inverted_index", "inverted_index"),
        ("src/buffer", "buffer"),
        ("src", "geo_index"), // Add this line
    ]);

Geo filter is not a static library so this is not possible

JoanFM
JoanFM previously approved these changes Sep 19, 2025
View reviewed changes
@chesedo chesedo added this pull request to the merge queue Sep 19, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 19, 2025
@JoanFM JoanFM added this pull request to the merge queue Sep 19, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 19, 2025
@chesedo chesedo added this pull request to the merge queue Sep 19, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 19, 2025
@chesedo chesedo added this pull request to the merge queue Sep 19, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 19, 2025
cursor[bot]
cursor bot reviewed Sep 21, 2025
View reviewed changes
@chesedo chesedo enabled auto-merge September 21, 2025 05:02
@chesedo chesedo changed the title Provide a fake isWithinRadius for the benchmarks Provide FFI stubs and ignore flaky test Sep 21, 2025
@chesedo chesedo added this pull request to the merge queue Sep 21, 2025
Merged via the queue into master with commit c786acf Sep 21, 2025
18 checks passed
@chesedo chesedo deleted the no_geo_filter_for_benchmarks branch September 21, 2025 07:16
lerman25 pushed a commit that referenced this pull request Sep 24, 2025
@chesedo @lerman25
Provide FFI stubs and ignore flaky test (#6860)
20fdaca 
* Provide a fake `isWithinRadius` for the benchmarks

* Provide a fake for `DocTable_Exists`

* Turn numeric FFI test off
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cursor cursor[bot] cursor[bot] left review comments

@oshadmi oshadmi oshadmi approved these changes

@JoanFM JoanFM JoanFM left review comments

Assignees

No one assigned

Labels

size:S size:XS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@chesedo @JoanFM @JonasKruckenberg @oshadmi