stream_processor: fix operator precedence for logical operations#9388
stream_processor: fix operator precedence for logical operations#9388unitmatrix wants to merge 1 commit intofluent:masterfrom
Conversation
unitmatrix
force-pushed
the
fix-sql-conditions
branch
2 times, most recently
from
a01df75 to
b50a45a
Compare
unitmatrix
requested review from
edsiper,
fujimotos and
leonardo-albertovich
as code owners
unitmatrix
force-pushed
the
fix-sql-conditions
branch
from
50766fe to
e524174
Compare
|
Thanks @unitmatrix, it would be great if the syntax (https://github.com/fluent/fluent-bit/tree/master/src/stream_processor#sql-statement-syntax) can be updated based on your changes as well. |
|
Hi @koleini, Thank you for your feedback. To ensure that I address your request accurately, I revisited my changes and believe they do not impact the existing SQL statement syntax as outlined in the documentation. Could you please specify which part of the syntax might be affected, or if there are specific updates you have in mind? This will help me make the necessary adjustments to the documentation more effectively. Looking forward to your guidance. |
unitmatrix
force-pushed
the
fix-sql-conditions
branch
from
e524174 to
1acc74d
Compare
unitmatrix
changed the title
|
Hi team, I hope everyone is doing well. I wanted to follow up on this pull request submitted on Sep 14. I've made the necessary updates based on the pipeline checks and have responded to all the questions so far. Could someone please provide an update on the status of this PR? I'm looking forward to any further feedback or actions needed to move this forward. Thank you for your time! |
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
@koleini @unitmatrix is this ready to go ? |
|
This PR is ready to go, @edsiper. |
|
Hi @unitmatrix, I reviewed the BNF syntax and looks like it requires further cleaning up, which will take care of it. I am just wondering if your test functions would cover all the combinations and ensures the precedence is implemented correctly: |
unitmatrix
force-pushed
the
fix-sql-conditions
branch
from
1acc74d to
ca73d7b
Compare
WalkthroughAdds operator precedence rules for boolean expressions (NOT > AND > OR), updates Bison name-prefix syntax and parser parse-params, adds a destructor for IDENTIFIER tokens, and introduces three tests and callbacks validating boolean precedence in WHERE clauses. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Case
participant Parser as Lexer/Bison Parser
participant AST as AST
participant Eval as Evaluator
participant CB as Test Callback
Test->>Parser: Submit SQL query with WHERE using NOT/AND/OR
Parser->>AST: Parse and build AST using precedence (NOT > AND > OR)
AST->>Eval: Evaluate expression per row
Eval-->>Test: Return result set
Test->>CB: Invoke callback with results
CB-->>Test: Perform assertions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/internal/include/sp_select_keys.h (1)
130-148: Consider adding a couple more precedence/associativity cases.Optional but useful to harden coverage:
- true OR false AND false should be parsed as true OR (false AND false) -> true (11 rows).
- true AND true OR false should be (true AND true) OR false -> true (11 rows).
- NOT false OR false -> true OR false -> true (11 rows).
- true AND NOT false -> true AND true -> true (11 rows).
I can add these to the suite if you want.
src/stream_processor/parser/sql.y (2)
1-1: Use modern Bison directive for symbol prefix (cleaner and future-proof).The inline comment already hints at it. Prefer
%define api.prefix {flb_sp_}over%name-prefix "flb_sp_".Apply this diff:
-%name-prefix "flb_sp_" // replace with %define api.prefix {flb_sp_} +%define api.prefix {flb_sp_}
101-104: Optional: make NOT’s precedence explicit on the rule with %prec.Bison assigns a rule’s precedence from its rightmost terminal, so
NOT conditionalready picks upNOT’s precedence. Adding%prec NOTon the rule makes intent explicit and guards against future edits:Outside this hunk, update the rule as:
condition: NOT condition %prec NOT { $$ = flb_sp_cmd_operation(cmd, $2, NULL, FLB_EXP_NOT); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/stream_processor/parser/sql.y(2 hunks)tests/internal/include/sp_cb_functions.h(1 hunks)tests/internal/include/sp_select_keys.h(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/internal/include/sp_select_keys.h (1)
tests/internal/include/sp_cb_functions.h (1)
cb_select_and_or_precedence(209-217)
🔇 Additional comments (4)
tests/internal/include/sp_cb_functions.h (3)
209-218: New AND/OR precedence callback looks correct (expects 11 rows).Expression false AND true OR true parses as (false AND true) OR true -> true; expecting all 11 rows is correct.
219-227: New NOT/OR precedence callback looks correct (expects 11 rows).Expression NOT true OR true parses as (NOT true) OR true -> false OR true -> true; expecting all 11 rows is correct.
229-237: New NOT/AND precedence callback looks correct (expects 0 rows).Expression NOT true AND false parses as (NOT true) AND false -> false AND false -> false; expecting 0 rows is correct.
src/stream_processor/parser/sql.y (1)
101-104: Manual Bison Conflict Verification RequiredThe precedence and associativity declarations for
NOT > AND > ORare correct and align with standard logical rules. However, since Bison isn’t available in this environment, please run the following in your local setup to ensure there are no lingering shift/reduce conflicts:
- Generate a Bison report and check for conflicts:
bison --report=states,solved,lookahead --report-file=sql.output src/stream_processor/parser/sql.y rg -n "conflicts:" sql.output || echo "No conflicts found"
unitmatrix
force-pushed
the
fix-sql-conditions
branch
from
ca73d7b to
53795fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/internal/include/sp_select_keys.h (1)
130-148: Add a couple more edge-case tests to harden coverageTwo quick additions would increase confidence in associativity and parenthesized overrides:
- Chained OR/AND associativity:
- true OR true AND false → expect all rows (AND binds before OR).
- true AND true AND false → expect 0 rows (left associativity).
- Parentheses override:
- NOT (true OR true) → expect 0 rows (ensures NOT applies after grouped OR).
I can draft the test entries if helpful.
src/stream_processor/parser/sql.y (2)
101-104: Precedence directives LGTM; consider switching to C-style comments for consistencyThe precedence levels are correct (OR lowest, AND middle, NOT highest). For consistency with the rest of this grammar file, prefer C-style comments.
-/* Define operator precedence and associativity for logical operations in conditions */ -%left OR // Lowest precedence for OR -%left AND // Middle precedence for AND -%right NOT // Highest precedence for NOT +/* Define operator precedence and associativity for logical operations in conditions */ +%left OR /* Lowest precedence for OR */ +%left AND /* Middle precedence for AND */ +%right NOT /* Highest precedence for NOT */
1-1: Prefer modern Bison directive: %define api.prefix {flb_sp_}%name-prefix is deprecated. Switching to the modern form avoids future warnings and matches current Bison conventions.
-%name-prefix "flb_sp_" // replace with %define api.prefix {flb_sp_} +%define api.prefix {flb_sp_}If your CI uses an older Bison, confirm it supports api.prefix (it’s available in Bison 2.6+).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/stream_processor/parser/sql.y(2 hunks)tests/internal/include/sp_cb_functions.h(1 hunks)tests/internal/include/sp_select_keys.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/internal/include/sp_cb_functions.h
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/internal/include/sp_select_keys.h (1)
tests/internal/include/sp_cb_functions.h (3)
cb_select_and_or_precedence(209-217)cb_select_not_or_precedence(219-227)cb_select_not_and_precedence(229-237)
🔇 Additional comments (2)
tests/internal/include/sp_select_keys.h (1)
130-148: New precedence tests are correct and consistent with callbacks
- The three queries exercise the intended precedence (NOT > AND > OR).
- The callbacks now match the actual implementations in sp_cb_functions.h; this resolves the earlier “cb_select_not_precedence” mismatch.
Looks good.
src/stream_processor/parser/sql.y (1)
1-1: flb_sp_parse call sites correctly updatedNo missing invocations found after adding the
queryparameter:
- src/stream_processor/parser/flb_sp_parser.c:396 –
ret = flb_sp_parse(cmd, sql, scanner);- No occurrences of
flb_sp_yyparse(elsewhereLexer prototype remains unchanged (
extern int yylex();), consistent with reentrant scanner usage.
Hi @koleini. Added 'NOT' > 'AND' test for completeness. Thank you. |
Signed-off-by: Aleksandr Cupacenko <apaxuc@gmail.com> revert definition removal Signed-off-by: Aleksandr Cupacenko <apaxuc@gmail.com> add operator precedence unit tests correct precedencer order add not_and precedent check correct function references ai suggestion
unitmatrix
force-pushed
the
fix-sql-conditions
branch
from
53795fc to
a722215
Compare
3 participants
Fixes #3763
Summary
This pull request addresses the issue where the precedence of logical operators (
AND,OR,NOT) was not defined in the grammar for the stream processor.Changes Made
NOT,AND, andORoperators to the Bison grammar file.NOTnow has the highest precedence.ANDhas middle precedence.ORhas the lowest precedence.Issue Addressed
Fixes GitHub issue: stream_processor: precedence of AND, OR, and NOT not defined in grammar.
Impact
This change ensures that logical operations in conditions are parsed with the correct precedence, aligning with expected logical operation rules.
Testing
Please verify that the logical operations in conditions are handled correctly according to the new precedence rules.
Feel free to reach out if there are any questions or further adjustments needed.
Summary by CodeRabbit
Bug Fixes
Tests
Chores