Skip to content

fix: Fix boolean handling in CEL-to-SQL and in facets #4894

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

Merged
merged 39 commits into from
May 28, 2025

Conversation

skynetigor
Copy link
Contributor

@skynetigor skynetigor commented May 24, 2025

Closes #4876

📑 Description

  • Refactor facets: now SQL query building will be done in dialect specific facet query builders
  • Fix "disabled" property handling in Workflow CEL query
  • Add data type assignments for all workflow fields for better handling in CEL
  • Add "dismissed" field configuration with proper data-type for alerts
  • Fix boolean handling in facets
  • Fix boolean handling in filtering by CEL
  • Add Unit-Test cases
  • Improve facet options query performance for alerts/incidents by joining only tables participated in filter or facet is created for. The improvement is 2x faster for the same number of alerts/incidents and facets.

✅ Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

ℹ Additional Information

Copy link

vercel bot commented May 24, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
keep ⬜️ Ignored (Inspect) Visit Preview May 26, 2025 0:42am

@skynetigor skynetigor marked this pull request as ready for review May 25, 2025 12:56
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. Bug Something isn't working labels May 25, 2025
Copy link

codecov bot commented May 25, 2025

Codecov Report

Attention: Patch coverage is 34.38735% with 166 lines in your changes missing coverage. Please review.

Project coverage is 46.31%. Comparing base (1ae49bf) to head (1bafa25).
Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
.../facets_query_builder/base_facets_query_builder.py 25.33% 56 Missing ⚠️
keep/api/core/facets_query_builder/postgresql.py 43.75% 18 Missing ⚠️
keep/api/core/facets.py 29.16% 17 Missing ⚠️
keep/api/core/facets_query_builder/mysql.py 48.00% 13 Missing ⚠️
keep/api/core/facets_query_builder/sqlite.py 45.45% 12 Missing ⚠️
keep/api/core/workflows.py 20.00% 12 Missing ⚠️
...e/facets_query_builder/get_facets_query_builder.py 52.94% 8 Missing ⚠️
keep/api/core/cel_to_sql/sql_providers/sqlite.py 12.50% 7 Missing ⚠️
keep/api/core/incidents.py 12.50% 7 Missing ⚠️
...ep/api/core/cel_to_sql/sql_providers/postgresql.py 0.00% 5 Missing ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4894      +/-   ##
==========================================
+ Coverage   46.19%   46.31%   +0.12%     
==========================================
  Files         165      172       +7     
  Lines       17495    17695     +200     
==========================================
+ Hits         8081     8196     +115     
- Misses       9414     9499      +85     

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@shahargl shahargl left a comment

Choose a reason for hiding this comment

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

lgtm'ed

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label May 28, 2025
@skynetigor skynetigor merged commit 175177a into main May 28, 2025
19 checks passed
@skynetigor skynetigor deleted the 4876-Booleans-work-bad-with-CEL-to-SQL branch May 28, 2025 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working lgtm This PR has been approved by a maintainer size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: Booleans work bad with CEL-to-SQL
2 participants