Skip to content

[MOD-11410] Make QueryError Fields Private#6928

Closed
enricozb wants to merge 10 commits intomasterfrom
enricozb/query-error
Closed

[MOD-11410] Make QueryError Fields Private#6928
enricozb wants to merge 10 commits intomasterfrom
enricozb/query-error

Conversation

@enricozb
Copy link
Contributor

@enricozb enricozb commented Sep 29, 2025

  1. Make the QueryError fields private, accessing them only via API.
  2. Replace all brace initializations with QueryError_Default().
    • QueryError_Init was removed in favor of initializing with QUERY_ERROR_DEFAULT.
  3. Additionally, comparisons of == QUERY_OK and != QUERY_OK were mostly replaced with QueryError_IsOk and QueryError_HasError.

For review purposes, each field was made private in a separate commit.

Note that there are two places where .detail was accessed directly, so reviewers please look carefully at the commit where QueryError.detail was made private.

Main objects this PR modified

  1. QueryError.

Mark if applicable

  • This PR introduces API changes
    • Removed QueryError_Init
    • Added QueryError_Default
    • Added QueryError_HasReachedMaxPrefixExpansionsWarning
    • Added QueryError_SetReachedMaxPrefixExpansionsWarning
    • Added QueryError_IsOk
    • Modified QueryError_HasError to explicitly return bool
  • This PR introduces serialization changes

Note

Encapsulates QueryError fields, adds new helper APIs, removes direct struct access/brace init, and updates all code/tests to use QueryError_Default/IsOk/HasError and new prefix-expansion warning helpers.

  • Core/Error Handling:
    • Make QueryError fields private (rename to internal members) and add APIs: QueryError_Default, QueryError_IsOk, QueryError_HasReachedMaxPrefixExpansionsWarning, QueryError_SetReachedMaxPrefixExpansionsWarning (and QueryError_HasError now returns bool).
    • Remove QueryError_Init; replace brace initializers and .code == QUERY_OK checks with new helpers; eliminate direct access to .detail, .code, and reachedMaxPrefixExpansions.
  • Executors/Processors:
    • Refactor aggregate, hybrid, coord, module, pipeline, query, vector index, and alias paths to the new API (e.g., QueryError_IsOk, warning helpers, QueryError_Default).
    • Adjust profiling/warning emission and timeout checks to use the new getters and helpers.
  • Misc:
    • Update tests to new initialization and getters; include query_error.h where needed.
    • Minor logic cleanups (e.g., clone/clear errors instead of direct struct copies).

Written by Cursor Bugbot for commit 33db84c. This will update automatically on new commits. Configure here.

@hdoordt
Copy link
Collaborator

hdoordt commented Sep 29, 2025

Changes look good to me. Could you rebase on master instead of merging master in here?

@enricozb enricozb force-pushed the enricozb/query-error branch 2 times, most recently from 33572f2 to 8d84752 Compare September 29, 2025 11:47
hdoordt
hdoordt previously approved these changes Sep 29, 2025
@enricozb enricozb changed the title MOD-11410: Make QueryError Fields Private [MOD-11410] Make QueryError Fields Private Sep 29, 2025
@enricozb enricozb marked this pull request as ready for review September 29, 2025 12:24
@enricozb enricozb force-pushed the enricozb/query-error branch from 2435688 to 25a8a5f Compare September 29, 2025 12:45
There are two places in this commit where QueryError.detail were
previously being accessed by consumers of the QueryError API. This
probably shouldn't happen since I believe detail (and message) should
only be read from and written to via the API. I'm not 100% sure though,
and this should be carefully looked at when reviewed.
cursor[bot]

This comment was marked as outdated.

@enricozb enricozb force-pushed the enricozb/query-error branch from eec851a to bf61081 Compare September 29, 2025 13:45
cursor[bot]

This comment was marked as outdated.

@codecov
Copy link

codecov bot commented Sep 29, 2025

Codecov Report

❌ Patch coverage is 88.34951% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.06%. Comparing base (db22e58) to head (33db84c).
⚠️ Report is 13 commits behind head on master.

Files with missing lines Patch % Lines
src/query.c 28.57% 5 Missing ⚠️
src/module.c 81.81% 2 Missing ⚠️
src/aggregate/aggregate_exec.c 66.66% 1 Missing ⚠️
src/aggregate/aggregate_exec_common.c 66.66% 1 Missing ⚠️
src/document.c 66.66% 1 Missing ⚠️
src/document_add.c 75.00% 1 Missing ⚠️
src/redisearch_api.c 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6928      +/-   ##
==========================================
- Coverage   86.11%   86.06%   -0.06%     
==========================================
  Files         310      311       +1     
  Lines       49797    50097     +300     
  Branches     9726     9929     +203     
==========================================
+ Hits        42882    43115     +233     
- Misses       6763     6831      +68     
+ Partials      152      151       -1     
Flag Coverage Δ
flow 84.12% <82.52%> (-0.07%) ⬇️
unit 51.77% <46.60%> (+0.09%) ⬆️

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.

@enricozb
Copy link
Contributor Author

enricozb commented Oct 1, 2025

Closed for #6962 since branches cannot have / in them.

@enricozb enricozb closed this Oct 1, 2025
@GuyAv46 GuyAv46 deleted the enricozb/query-error branch October 5, 2025 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants