feat: integrate ParserSQL as vendored static library#5736
Conversation
C1: NODE_SET_CHARSET map key corrected to character_set_results C2: query_processor_parser==1 now also routes SET through ParserSQL C3: qp->digest initialized to 0 on parse failure I1: header docs corrected (tokenizer-based, not libinjection) I3: defensive NODE_SET_TRANSACTION case added to SET walker
- New test group test/tap/groups/set_parser_algorithm=3/ with env.sh, infras.lst, and pre-proxysql.sql enabling both mysql and pgsql set_parser_algorithm=3 - 14 SET-related tests added to set_parser_algorithm=3-g1 in groups.json (12 MySQL + 2 PgSQL) - 2 ParserSQL unit tests added to unit-tests-g1 in groups.json - CI reusable workflow and caller workflow for set_parser_algorithm=3-g1 using Docker-safe INFRA_ID (ci-set-parser-algorithm-3-g1) - Locally verified: infrastructure starts, tests discovered correctly, unit tests pass (51/51 digest, 202/224 SET)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a vendored ParserSQL dependency and an adapter that provides digest generation, command-type classification, and SET-statement parsing. Introduces runtime toggle ChangesParserSQL integration + wiring
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant QP as QueryProcessor
participant Parser as ParserSQL
participant DB as Database
rect rgba(100,149,237,0.5)
Client->>QP: Sends query (any SQL)
alt query_processor_parser == 1
QP->>Parser: parsersql_digest_init_*(query)
Parser-->>QP: normalized digest_text + digest
QP->>Parser: parsersql_command_type_*(query)
Parser-->>QP: command enum
alt command == SET
QP->>Parser: parsersql_parse_set_*(query)
Parser-->>QP: map<var, values>
end
else legacy path
QP->>QP: legacy tokenizer/digest logic
end
QP->>DB: execute (possibly using parsed metadata)
DB-->>QP: result
QP-->>Client: response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates the ParserSQL library into ProxySQL as a vendored static library, providing an alternative query processing path for digests, command classification, and SET statement parsing. The integration is runtime-selectable via new thread variables. Feedback highlights critical issues including a regression in comment extraction, failure to respect the maximum digest length during hashing, and inefficiencies from redundant parsing and heap allocations. Improvements were also suggested for command mapping granularity and whitespace handling.
| void parsersql_digest_init_mysql(SQP_par_t* qp, const char* query, int query_length) { | ||
| qp->digest_text = NULL; | ||
| qp->first_comment = NULL; | ||
| qp->query_prefix = NULL; | ||
| qp->digest = 0; |
There was a problem hiding this comment.
The parsersql_digest_init_mysql function (and its PostgreSQL counterpart) initializes qp->first_comment to NULL but never populates it. This is a regression from the legacy implementation, which extracts the first comment from the query. Features relying on query comments for routing or logging will be broken when query_processor_parser is enabled.
| } | ||
| qp->digest_text = strdup(normalized.c_str()); | ||
| // SpookyHash is preserved for backward compatibility with existing digest stats | ||
| qp->digest = SpookyHash::Hash64(normalized.c_str(), normalized.size(), 0); |
There was a problem hiding this comment.
The digest hash is computed over the entire normalized string, ignoring the query_digests_max_digest_length setting. This will result in different digest hashes for long queries compared to the legacy implementation, potentially breaking existing query rules and statistics that rely on specific hash values.
| case StmtType::CREATE: return MYSQL_COM_QUERY_CREATE_TABLE; | ||
| case StmtType::ALTER: return MYSQL_COM_QUERY_ALTER_TABLE; | ||
| case StmtType::DROP: return MYSQL_COM_QUERY_DROP_TABLE; | ||
| case StmtType::TRUNCATE: return MYSQL_COM_QUERY_TRUNCATE_TABLE; |
There was a problem hiding this comment.
The mapping for CREATE, ALTER, and DROP is too coarse, as it always returns the _TABLE variant of the command. This will lead to incorrect command statistics for other object types (e.g., CREATE DATABASE, DROP VIEW). The mapping should be expanded to cover more specific StmtType values if provided by the parser.
| if (mysql_thread___query_processor_parser == 1) { | ||
| if (text) { | ||
| return parsersql_command_type_mysql(text, strlen(text)); | ||
| } | ||
| return MYSQL_COM_QUERY_UNKNOWN; | ||
| } |
There was a problem hiding this comment.
When query_processor_parser is enabled, the query has already been parsed once in query_parser_init. Re-parsing the normalized digest text here to determine the command type is inefficient. Ideally, the command type should be captured during the initial parse and stored in SQP_par_t to avoid redundant work on every query.
| if (!after_var || after_var >= query + query_len) return ""; | ||
| const char* p = after_var; | ||
| const char* end = query + query_len; | ||
| while (p < end && (*p == ' ' || *p == '\t')) p++; |
There was a problem hiding this comment.
This loop only skips spaces and tabs. It should handle all whitespace characters (e.g., newlines) to correctly support multi-line SET statements. Using isspace() from <cctype> is recommended.
| while (p < end && (*p == ' ' || *p == '\t')) p++; | |
| while (p < end && isspace(*p)) p++; |
| DigestResult dr = digest.compute(query, query_length); | ||
| normalized.assign(dr.normalized.ptr, dr.normalized.len); | ||
| } | ||
| qp->digest_text = strdup(normalized.c_str()); |
There was a problem hiding this comment.
To improve efficiency on the hot path, consider using the pre-allocated qp->buf for short digests instead of always calling strdup. This matches the behavior of the legacy tokenizer implementation.
if (normalized.size() < QUERY_DIGEST_BUF) {
qp->digest_text = qp->buf;
memcpy(qp->buf, normalized.c_str(), normalized.size() + 1);
} else {
qp->digest_text = strdup(normalized.c_str());
}| if (val == "''" || val == "\"\"") { | ||
| val = ""; | ||
| } else { | ||
| val = strip_quotes(val); | ||
| } |
There was a problem hiding this comment.
The explicit check for empty string literals ('' or "") is redundant because strip_quotes already handles these cases by returning an empty string.
val = strip_quotes(val);![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Makefile (1)
145-182:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winmacOS build will fail to link
libsqlparser.On Darwin,
MYLIBSis completely replaced with:=(Line 146), discarding the entireSTATICMYLIBSblock (which contains-lsqlparser).LIBPROXYSQLARalso never adds$(PARSERSQL_LDIR)/libsqlparser.afor Darwin, solibsqlparsersymbols contributed byQuery_Processor_ParserSQL.oowill be unresolved at link time.🛠️ Proposed fix
Add
libsqlparser.ato the Darwin-specificLIBPROXYSQLARblock (parallel to how other deps are handled there):ifeq ($(UNAME_S),Darwin) LIBPROXYSQLAR += $(JEMALLOC_LDIR)/libjemalloc.a ... LIBPROXYSQLAR += $(POSTGRESQL_PATH)/port/libpgport.a + LIBPROXYSQLAR += $(PARSERSQL_LDIR)/libsqlparser.a endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Makefile` around lines 145 - 182, The Darwin block currently overwrites MYLIBS (using :=) which drops STATICMYLIBS and omits libsqlparser from LIBPROXYSQLAR causing unresolved Query_Processor_ParserSQL.oo symbols; change the Darwin MYLIBS assignment to append (use += instead of :=) so STATICMYLIBS is kept, and add $(PARSERSQL_LDIR)/libsqlparser.a to the Darwin-specific LIBPROXYSQLAR list so libsqlparser symbols are linked.
🧹 Nitpick comments (4)
include/Query_Processor_ParserSQL.h (1)
28-29: ⚡ Quick winAlign the include guard with the repo convention.
This new header uses a project-local guard style instead of the
__CLASS_*_Hpattern required for files underinclude/. As per coding guidelines,include/**/*.{h,hpp}: Use#ifndef __CLASS_*_Hinclude guards in header files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/Query_Processor_ParserSQL.h` around lines 28 - 29, Replace the project-local include guard macro PROXYSQL_QUERY_PROCESSOR_PARSERSQL_H with the repo convention `__CLASS_*_H`: change the two lines that define the guard to use `#ifndef __CLASS_QUERY_PROCESSOR_PARSERSQL_H` and `#define __CLASS_QUERY_PROCESSOR_PARSERSQL_H`, and ensure the trailing `#endif` (in Query_Processor_ParserSQL.h) remains present and optionally comments the macro name for clarity.lib/Query_Processor.cpp (1)
2128-2137: Consider defensively initializingqp->digestat the start ofquery_parser_init().The ParserSQL adapter functions (
parsersql_digest_init_mysqlandparsersql_digest_init_pgsql) already defensively initializeqp->digest = 0andqp->first_comment = NULLat function entry (before any parsing logic). However, the legacy digest path only setsqp->digestconditionally within its branch. SinceSQP_par_tis reused across multiple queries (viaquery_parser_free()followed byquery_parser_init()at lines 1969–1971), initializingqp->digest = 0upfront inquery_parser_init()ensures all code paths start from a clean state and improves clarity.Suggested initialization
void Query_Processor<QP_DERIVED>::query_parser_init(SQP_par_t *qp, const char *query, int query_length, int flags) { // trying to get rid of libinjection // instead of initializing qp->sf , we copy query info later in this function + qp->digest = 0; qp->digest_text = NULL; qp->first_comment = NULL; qp->query_prefix = NULL;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/Query_Processor.cpp` around lines 2128 - 2137, Initialize qp->digest to 0 at the start of query_parser_init() so the SQP_par_t state is always clean when reused; specifically, add a defensive assignment qp->digest = 0 near the top of query_parser_init() (alongside existing qp->digest_text=NULL, qp->first_comment=NULL, qp->query_prefix=NULL) to guarantee a default value for both legacy and ParserSQL paths (parsersql_digest_init_mysql and parsersql_digest_init_pgsql) and avoid relying on conditional branches after query_parser_free()/query_parser_init() reuse.test/tap/tests/Makefile (1)
390-412: ⚡ Quick winAdd the new ParserSQL test binaries to
clean.
cleanremoves older setparser artifacts but currently leavessetparser_parsersql_test,setparser_parsersql_test-t, andparsersql_digest_testbehind.Suggested diff
clean: rm -f *-t || true rm -f galera_1_timeout_count galera_2_timeout_no_count aurora || true rm -f generate_set_session_csv set_testing-240.csv || true rm -f setparser_test setparser_test2 setparser_test3 || true + rm -f setparser_parsersql_test setparser_parsersql_test-t parsersql_digest_test || true rm -f reg_test_3504-change_user_libmariadb_helper reg_test_3504-change_user_libmysql_helper || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/Makefile` around lines 390 - 412, The clean target currently omits the new ParserSQL test binaries; update the Makefile's clean rule to also remove setparser_parsersql_test, setparser_parsersql_test-t, and parsersql_digest_test (and any related artifacts) so those targets are deleted by clean; locate the clean target and add rm -f entries for setparser_parsersql_test setparser_parsersql_test-t parsersql_digest_test (and any matching test artifacts) to ensure they are removed.test/tap/tests/setparser_parsersql_test.cpp (1)
50-77: ⚡ Quick winKnown-failing cases need
todo()markers to prevent non-zero exit statusWith 22 of the 224 SET cases failing (per PR objectives), each call to
TestParsefor those cases emits one or two failingok()assertions.exit_status()therefore returns non-zero, causing CI to mark the binary as failed. Since these failures are upstream ParserSQL bugs (not ProxySQL logic errors), they should be expressed as expected failures viatodo().Without seeing the exact failing indices, a practical approach is to identify the failing entries in
setparser_test_common.hand wrap only those twook()calls:- bool size_ok = (result.size() == data.size()); - ok(size_ok, "[%s %d] Sizes match: %lu, %lu", title.c_str(), i, result.size(), data.size()); - ... - bool elem_ok = maps_match(result, data); - ok(elem_ok, "[%s %d] Elements match", title.c_str(), i); + // For known-failing ParserSQL bugs, use todo(): + bool size_ok = (result.size() == data.size()); + if (tests[i].known_parsersql_bug) { + todo("known ParserSQL bug"); + ok(size_ok, "[%s %d] Sizes match: %lu, %lu", title.c_str(), i, result.size(), data.size()); + end_todo; + } else { + ok(size_ok, "[%s %d] Sizes match: %lu, %lu", title.c_str(), i, result.size(), data.size()); + }(The exact TAP helper API depends on what
setparser_test_common.hexposes; adjust accordingly.)This aligns with the fix suggested in
groups.json— either resolve the exit-status issue here withtodo(), or movesetparser_parsersql_test-tout ofunit-tests-g1until upstream bugs are fixed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/setparser_parsersql_test.cpp` around lines 50 - 77, TestParse is producing hard failures for known-broken ParserSQL cases causing non-zero exit; modify TestParse to mark the specific failing assertions as expected failures using the TAP todo() wrapper (or the project’s equivalent) around the two ok() checks that compare sizes and elements for the failing test indices from setparser_test_common.h. Concretely: detect the failing test indices (from the 22 known cases) when iterating tests in TestParse, and call todo() before the size_ok and elem_ok assertions (and end the todo block after them) so those ok() calls are treated as expected failures; keep all other logic (maps_match, diag output, normalize_value) unchanged and only wrap the two ok(...) calls for the affected tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-05-02-parsersql-integration.md`:
- Around line 250-252: The plan uses the Postgres TLS symbol
`pgsql_thread___query_processor_parser` in the MySQL step; change the TLS
refresh and assignment to the MySQL symbol instead. Add
REFRESH_VARIABLE_INT(query_processor_parser) alongside
REFRESH_VARIABLE_INT(query_processor_regex), and replace the incorrect
`pgsql_thread___query_processor_parser = ...` assignment with
`mysql_thread___query_processor_parser = ...` near the existing
`mysql_thread___query_processor_regex` refresh so the MySQL path wires the
parser variable into the correct thread-local symbol.
In `@lib/MySQL_Query_Processor.cpp`:
- Around line 165-171: When mysql_thread___query_processor_parser == 1 the code
returns MYSQL_COM_QUERY_UNKNOWN if text (qp->digest_text or qp->query_prefix) is
NULL; fix query_parser_command_type (the block calling
parsersql_command_type_mysql) to fall back to the legacy/tokenizer logic when
query_digests == 1 and parsing failed: if text is NULL, attempt to derive the
command prefix using the legacy tokenizer or the existing query string (the same
logic that would populate qp->query_prefix in the non-parsersql path) and then
call parsersql_command_type_mysql or return the command type from that fallback;
reference mysql_thread___query_processor_parser, query_digests,
query_parser_init, parsersql_digest_init_mysql, qp->query_prefix,
qp->digest_text, and parsersql_command_type_mysql to locate and implement the
fallback so command type is determined even when parsersql parsing fails.
In `@lib/PgSQL_Query_Processor.cpp`:
- Around line 671-673: The early return on pgsql_thread___query_processor_parser
should not unconditionally use parsersql_command_type_pgsql(text, strlen(text))
because ParserSQL currently collapses many PostgreSQL statement kinds; change
the branch in PgSQL_Query_Processor (the block guarded by
pgsql_thread___query_processor_parser) to call parsersql_command_type_pgsql as a
first attempt but, if it returns a coarse/generic result (e.g. the generic
CREATE/ALTER/DROP values that map to PGSQL_QUERY_CREATE_TABLE / ALTER_TABLE /
DROP_TABLE) then fall back to the legacy tokenizer/classifier used previously
(the original query-processing classification code in this file) so that
specific commands like CREATE VIEW, ALTER ROLE, DROP INDEX, COPY, DISCARD and
cursor statements are preserved; implement this by comparing the parsersql
result against the generic enums in enum PGSQL_QUERY_command and invoking the
legacy classification path when a generic value is returned.
- Around line 671-675: The early-return branch guarded by
pgsql_thread___query_processor_parser == 1 leaks qp->query_prefix because it
returns before the cleanup; modify that branch in the function containing
parsersql_command_type_pgsql calls to perform the same cleanup of
qp->query_prefix (free/null or call the existing cleanup helper used elsewhere)
before returning the result from parsersql_command_type_pgsql(text,
strlen(text)) or PGSQL_QUERY_UNKNOWN so the buffer is not leaked.
In `@lib/Query_Processor_ParserSQL.cpp`:
- Around line 206-208: The adapter currently sets qp->digest_text =
strdup(normalized.c_str()) then computes qp->digest over the full normalized
string; change the hashing to only use up to query_digests_max_digest_length
bytes (same behavior as the legacy path) by truncating or passing the minimum of
normalized.size() and query_digests_max_digest_length to SpookyHash::Hash64 when
computing qp->digest (and ensure qp->digest_text still contains the full or
legacy-truncated text as required); apply the same fix to the second occurrence
that computes qp->digest later in this file.
- Around line 185-189: parsersql_digest_init_mysql() and
parsersql_digest_init_pgsql() currently leave qp->first_comment NULL which
breaks downstream code expecting the header contract; update these functions to
populate qp->first_comment the same way the legacy paths do (i.e., reuse or call
the logic in mysql_query_digest_and_first_comment() /
pgsql_query_digest_and_first_comment() or replicate their extraction steps using
the ParserSQL parse result), set qp->first_comment to a heap-allocated
NUL-terminated string with the same ownership/cleanup semantics as the legacy
path, and ensure Query_Processor.cpp's existing if (qp->first_comment) checks
work correctly; alternatively, if you choose not to implement extraction, update
the header/docs and remove/guard any code paths that rely on qp->first_comment
when ParserSQL is enabled.
In `@test/tap/groups/groups.json`:
- Around line 196-197: The entry "setparser_parsersql_test-t" currently
registered in "unit-tests-g1" will fail CI due to 22 known ParserSQL failures;
fix by either removing that key from the "unit-tests-g1" array and adding it
exclusively to the "set_parser_algorithm=3-g1" group in groups.json (so only
that alternate parser group runs it), or modify the test binary
setparser_parsersql_test-t to wrap the 22 known-failing ok() assertions with
todo() markers (identify and mark the specific test IDs/assertions inside
setparser_parsersql_test-t so TAP treats them as expected failures).
---
Outside diff comments:
In `@src/Makefile`:
- Around line 145-182: The Darwin block currently overwrites MYLIBS (using :=)
which drops STATICMYLIBS and omits libsqlparser from LIBPROXYSQLAR causing
unresolved Query_Processor_ParserSQL.oo symbols; change the Darwin MYLIBS
assignment to append (use += instead of :=) so STATICMYLIBS is kept, and add
$(PARSERSQL_LDIR)/libsqlparser.a to the Darwin-specific LIBPROXYSQLAR list so
libsqlparser symbols are linked.
---
Nitpick comments:
In `@include/Query_Processor_ParserSQL.h`:
- Around line 28-29: Replace the project-local include guard macro
PROXYSQL_QUERY_PROCESSOR_PARSERSQL_H with the repo convention `__CLASS_*_H`:
change the two lines that define the guard to use `#ifndef
__CLASS_QUERY_PROCESSOR_PARSERSQL_H` and `#define
__CLASS_QUERY_PROCESSOR_PARSERSQL_H`, and ensure the trailing `#endif` (in
Query_Processor_ParserSQL.h) remains present and optionally comments the macro
name for clarity.
In `@lib/Query_Processor.cpp`:
- Around line 2128-2137: Initialize qp->digest to 0 at the start of
query_parser_init() so the SQP_par_t state is always clean when reused;
specifically, add a defensive assignment qp->digest = 0 near the top of
query_parser_init() (alongside existing qp->digest_text=NULL,
qp->first_comment=NULL, qp->query_prefix=NULL) to guarantee a default value for
both legacy and ParserSQL paths (parsersql_digest_init_mysql and
parsersql_digest_init_pgsql) and avoid relying on conditional branches after
query_parser_free()/query_parser_init() reuse.
In `@test/tap/tests/Makefile`:
- Around line 390-412: The clean target currently omits the new ParserSQL test
binaries; update the Makefile's clean rule to also remove
setparser_parsersql_test, setparser_parsersql_test-t, and parsersql_digest_test
(and any related artifacts) so those targets are deleted by clean; locate the
clean target and add rm -f entries for setparser_parsersql_test
setparser_parsersql_test-t parsersql_digest_test (and any matching test
artifacts) to ensure they are removed.
In `@test/tap/tests/setparser_parsersql_test.cpp`:
- Around line 50-77: TestParse is producing hard failures for known-broken
ParserSQL cases causing non-zero exit; modify TestParse to mark the specific
failing assertions as expected failures using the TAP todo() wrapper (or the
project’s equivalent) around the two ok() checks that compare sizes and elements
for the failing test indices from setparser_test_common.h. Concretely: detect
the failing test indices (from the 22 known cases) when iterating tests in
TestParse, and call todo() before the size_ok and elem_ok assertions (and end
the todo block after them) so those ok() calls are treated as expected failures;
keep all other logic (maps_match, diag output, normalize_value) unchanged and
only wrap the two ok(...) calls for the affected tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ffb644b8-0d69-4fb4-8a86-88e339b6199a
⛔ Files ignored due to path filters (1)
deps/parsersql/parsersql-1.0.0.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (27)
.github/workflows/CI-set_parser_algorithm=3-g1.yml.github/workflows/gh-actions-reusable/ci-set_parser_algorithm=3-g1.ymldeps/Makefiledocs/superpowers/plans/2026-05-02-parsersql-integration.mddocs/superpowers/specs/2026-05-02-parsersql-integration-design.mdinclude/MySQL_Thread.hinclude/PgSQL_Thread.hinclude/Query_Processor_ParserSQL.hinclude/makefiles_paths.mkinclude/proxysql_structs.hlib/Makefilelib/MySQL_Query_Processor.cpplib/MySQL_Session.cpplib/MySQL_Thread.cpplib/PgSQL_Query_Processor.cpplib/PgSQL_Session.cpplib/PgSQL_Thread.cpplib/Query_Processor.cpplib/Query_Processor_ParserSQL.cppsrc/Makefiletest/tap/groups/groups.jsontest/tap/groups/set_parser_algorithm=3/env.shtest/tap/groups/set_parser_algorithm=3/infras.lsttest/tap/groups/set_parser_algorithm=3/pre-proxysql.sqltest/tap/tests/Makefiletest/tap/tests/parsersql_digest_test.cpptest/tap/tests/setparser_parsersql_test.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: run / trigger
- GitHub Check: claude-review
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names should use PascalCase with protocol prefixes (MySQL_,PgSQL_,ProxySQL_)
Member variables should use snake_case naming
Constants and macros should use UPPER_SNAKE_CASE naming
C++17 is required; use conditional compilation via#ifdef PROXYSQLGENAI,#ifdef PROXYSQL31, etc. for feature-gated code
Use jemalloc for memory allocation
Use pthread mutexes for synchronization andstd::atomic<>for counters
Files:
test/tap/tests/parsersql_digest_test.cpptest/tap/tests/setparser_parsersql_test.cpplib/PgSQL_Thread.cppinclude/MySQL_Thread.hinclude/Query_Processor_ParserSQL.hlib/MySQL_Session.cpplib/MySQL_Thread.cppinclude/proxysql_structs.hlib/PgSQL_Session.cpplib/Query_Processor_ParserSQL.cppinclude/PgSQL_Thread.hlib/MySQL_Query_Processor.cpplib/PgSQL_Query_Processor.cpplib/Query_Processor.cpp
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file typically in
lib/directory
Files:
lib/PgSQL_Thread.cpplib/MySQL_Session.cpplib/MySQL_Thread.cpplib/PgSQL_Session.cpplib/Query_Processor_ParserSQL.cpplib/MySQL_Query_Processor.cpplib/PgSQL_Query_Processor.cpplib/Query_Processor.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
#ifndef __CLASS_*_Hinclude guards in header files
Files:
include/MySQL_Thread.hinclude/Query_Processor_ParserSQL.hinclude/proxysql_structs.hinclude/PgSQL_Thread.h
test/tap/groups/groups.json
📄 CodeRabbit inference engine (CLAUDE.md)
Register new test files in
test/tap/groups/groups.jsonfor test discovery via pattern rules intest/tap/tests/Makefile
Files:
test/tap/groups/groups.json
🧠 Learnings (3)
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/parsersql_digest_test.cpptest/tap/tests/setparser_parsersql_test.cpp
📚 Learning: 2026-04-11T13:17:55.508Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.508Z
Learning: When using GitHub-flavored Markdown headings, be aware that an em-dash surrounded by spaces (written as ` — `) affects the generated anchor/slug: GitHub replaces spaces with hyphens and removes non-alphanumeric punctuation, which can produce double hyphens (e.g., `## Foo — bar` → anchor `#foo--bar`, not `#foo-bar`). If you reference these anchors (e.g., internal links), ensure the expected slug matches this behavior.
Applied to files:
docs/superpowers/specs/2026-05-02-parsersql-integration-design.mddocs/superpowers/plans/2026-05-02-parsersql-integration.md
📚 Learning: 2026-04-11T13:17:55.509Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.509Z
Learning: When reviewing GitHub-flavored Markdown links/anchors, remember that heading-to-anchor slug generation treats spaces as hyphens and removes punctuation. If a heading contains an em-dash surrounded by spaces (e.g. ` — `), the slugs can legitimately include a double hyphen where the two surrounding space-runs become `-` on either side of the removed em-dash (e.g. `...vocabulary--read...`). Do not flag double-hyphens in anchor links for em-dash-containing headings as errors; they reflect GitHub’s correct slug behavior.
Applied to files:
docs/superpowers/specs/2026-05-02-parsersql-integration-design.mddocs/superpowers/plans/2026-05-02-parsersql-integration.md
🪛 checkmake (0.3.2)
deps/Makefile
[warning] 388-388: Target "parsersql" should be declared PHONY.
(phonydeclared)
test/tap/tests/Makefile
[warning] 400-400: Target "setparser_parsersql_test" should be declared PHONY.
(phonydeclared)
[warning] 407-407: Target "parsersql_digest_test" should be declared PHONY.
(phonydeclared)
🪛 Cppcheck (2.20.0)
test/tap/tests/parsersql_digest_test.cpp
[warning] 46-46: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
test/tap/tests/setparser_parsersql_test.cpp
[warning] 46-46: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
lib/Query_Processor_ParserSQL.cpp
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 46-46: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🪛 LanguageTool
docs/superpowers/plans/2026-05-02-parsersql-integration.md
[style] ~251-~251: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...processor_regex);(around line 4607). Addpgsql_thread___query_processor_parser ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.22.1)
docs/superpowers/specs/2026-05-02-parsersql-integration-design.md
[warning] 17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 145-145: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 183-183: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 190-190: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🔇 Additional comments (14)
lib/Makefile (1)
30-30: LGTM – build wiring is correct.
PARSERSQL_IDIRinIDIRSis required to compileQuery_Processor_ParserSQL.cpp;Query_Processor_ParserSQL.oocorrectly extends_OBJ_CXXso the adapter is archived intolibproxysql.a. (Note:LDIRSinlib/Makefileis unused because the only output is anar rcsarchive, not a linked binary — the entry is harmless but dead weight.)Also applies to: 43-44, 126-126
test/tap/groups/set_parser_algorithm=3/infras.lst (1)
1-3: LGTM – infra entries match the test group environment.test/tap/groups/set_parser_algorithm=3/pre-proxysql.sql (1)
1-7: LGTM – standard pre-test fixture.lib/MySQL_Session.cpp (2)
18-18: ParserSQL header wiring looks correct.Line 18 adds the required adapter include used by the SET parsing path later in this file.
7480-7482: SET parsing mode switch is implemented as intended.Lines 7480-7482 correctly route SET parsing to ParserSQL when either algorithm 3 is selected or full parser mode (
query_processor_parser == 1) is enabled.lib/PgSQL_Session.cpp (2)
1-1: Include updates look good for ParserSQL wiring.Lines 1 and 18 are consistent with the new adapter integration and keep the translation unit aligned with the new parsing path.
Also applies to: 18-18
4454-4460: Parser routing condition is correctly aligned with runtime flags.Lines 4454-4460 correctly enable ParserSQL for SET statements when either
set_parser_algorithm=3orquery_processor_parser=1, while preserving the legacy parser fallback otherwise.lib/MySQL_Thread.cpp (1)
468-468: Good end-to-end thread-variable wiring for ParserSQL toggles.The new
query_processor_parservariable is consistently registered, defaulted, range-checked, and refreshed; theset_parser_algorithmrange extension to3is also coherently applied.Also applies to: 1350-1350, 2639-2642, 4611-4611
test/tap/tests/parsersql_digest_test.cpp (1)
33-58: TAP loop structure and per-iteration cleanup look solid.
plan(count * 3)matches the three assertions per query, and parser-owned fields are released each iteration..github/workflows/CI-set_parser_algorithm=3-g1.yml (1)
1-20: LGTMThe trigger conditions, concurrency group, conditional job guard, and delegation to the reusable workflow are all consistent with the existing CI workflow patterns.
test/tap/groups/set_parser_algorithm=3/env.sh (1)
1-8: LGTMEnvironment variables are correctly exported and align with the infrastructure entries in
infras.lst(infra-dbdeployer-mysql57,docker-pgsql16-single) and howensure-infras.bashresolves them.include/proxysql_structs.h (1)
1200-1200: LGTMBoth
pgsql_thread___query_processor_parserandmysql_thread___query_processor_parserare correctly added in the definition (#ifdef PROXYSQL_EXTERN) and declaration (#else) blocks, adjacent to the existingquery_processor_regexvariables, consistent with the rest of the file's pattern.Also applies to: 1293-1293, 1538-1538, 1631-1631
.github/workflows/gh-actions-reusable/ci-set_parser_algorithm=3-g1.yml (1)
1-121: LGTMThe workflow structure (cache restore → binary verify → build image → start infra → run tests → cleanup → upload artifacts → update check) is consistent with other reusable CI workflows in the project. The
fail-fast: false,if: always()cleanup guard, and artifact-upload condition are all correctly applied.test/tap/tests/setparser_parsersql_test.cpp (1)
17-48: LGTM
normalize_value/values_match/maps_matchare correct. The simultaneous ordered iteration inmaps_matchis valid forstd::map. The Cppcheck warning at line 46 is a false positive (std::bad_alloc, not a null dereference path).
| Add `REFRESH_VARIABLE_INT(query_processor_parser);` near `REFRESH_VARIABLE_INT(query_processor_regex);` (around line 4607). | ||
|
|
||
| Add `pgsql_thread___query_processor_parser = ...` refresh assignment near the existing `pgsql_thread___query_processor_regex` line (around line 4607). |
There was a problem hiding this comment.
Fix the MySQL TLS symbol in this step.
This MySQL_Thread.cpp step points to pgsql_thread___query_processor_parser, which is the wrong thread-local variable for the MySQL path. Anyone following the plan verbatim will wire the refresh into the wrong symbol.
🧰 Tools
🪛 LanguageTool
[style] ~251-~251: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...processor_regex);(around line 4607). Addpgsql_thread___query_processor_parser ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-05-02-parsersql-integration.md` around lines 250
- 252, The plan uses the Postgres TLS symbol
`pgsql_thread___query_processor_parser` in the MySQL step; change the TLS
refresh and assignment to the MySQL symbol instead. Add
REFRESH_VARIABLE_INT(query_processor_parser) alongside
REFRESH_VARIABLE_INT(query_processor_regex), and replace the incorrect
`pgsql_thread___query_processor_parser = ...` assignment with
`mysql_thread___query_processor_parser = ...` near the existing
`mysql_thread___query_processor_regex` refresh so the MySQL path wires the
parser variable into the correct thread-local symbol.
|
|
||
| if (mysql_thread___query_processor_parser == 1) { | ||
| if (text) { | ||
| return parsersql_command_type_mysql(text, strlen(text)); | ||
| } | ||
| return MYSQL_COM_QUERY_UNKNOWN; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all write sites for qp->query_prefix to confirm whether it is ever
# populated outside the query_digests guard (which would mean the legacy
# tokenizer survives digests==0 but the new parsersql path silently drops it).
rg -n 'query_prefix\s*=' --type cpp -A2 -B4Repository: sysown/proxysql
Length of output: 3775
🏁 Script executed:
# Check the actual code at lib/MySQL_Query_Processor.cpp lines 165-171
sed -n '160,175p' lib/MySQL_Query_Processor.cppRepository: sysown/proxysql
Length of output: 358
🏁 Script executed:
# Check the context around Query_Processor.cpp:2170 to see the guard scope
sed -n '2125,2175p' lib/Query_Processor.cppRepository: sysown/proxysql
Length of output: 2429
🏁 Script executed:
# Verify if parsersql_digest_init_mysql actually sets query_prefix
sed -n '183,210p' lib/Query_Processor_ParserSQL.cppRepository: sysown/proxysql
Length of output: 1304
🏁 Script executed:
# Check what parsersql_digest_init_mysql does with query_prefix
sed -n '183,212p' lib/Query_Processor_ParserSQL.cppRepository: sysown/proxysql
Length of output: 1335
🏁 Script executed:
# Check if there's any post-processing that sets query_prefix after parsersql_digest_init_mysql
sed -n '2126,2145p' lib/Query_Processor.cppRepository: sysown/proxysql
Length of output: 1092
🏁 Script executed:
# Verify the exact condition in MySQL_Query_Processor.cpp around lines 155-175
sed -n '155,180p' lib/MySQL_Query_Processor.cppRepository: sysown/proxysql
Length of output: 741
Query command-type degrades to MYSQL_COM_QUERY_UNKNOWN when query_processor_parser=1 with query_digests=1 and parsing fails.
When query_processor_parser == 1 and query_digests == 1, query_parser_init() calls parsersql_digest_init_mysql(), which explicitly sets qp->query_prefix = NULL and only populates qp->digest_text if parsing succeeds. If parsing fails (e.g., unsupported SQL), both pointers remain NULL. In query_parser_command_type(), this causes text to be NULL, returning MYSQL_COM_QUERY_UNKNOWN even though the command type could be determined. This disables command counters and routing logic that depend on command type.
The legacy tokenizer path (query_processor_parser != 1) populates qp->query_prefix independently when query_digests == 0 and commands_stats == 1, but the new parsersql path has no such fallback when query_digests == 1.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/MySQL_Query_Processor.cpp` around lines 165 - 171, When
mysql_thread___query_processor_parser == 1 the code returns
MYSQL_COM_QUERY_UNKNOWN if text (qp->digest_text or qp->query_prefix) is NULL;
fix query_parser_command_type (the block calling parsersql_command_type_mysql)
to fall back to the legacy/tokenizer logic when query_digests == 1 and parsing
failed: if text is NULL, attempt to derive the command prefix using the legacy
tokenizer or the existing query string (the same logic that would populate
qp->query_prefix in the non-parsersql path) and then call
parsersql_command_type_mysql or return the command type from that fallback;
reference mysql_thread___query_processor_parser, query_digests,
query_parser_init, parsersql_digest_init_mysql, qp->query_prefix,
qp->digest_text, and parsersql_command_type_mysql to locate and implement the
fallback so command type is determined even when parsersql parsing fails.
| if (pgsql_thread___query_processor_parser == 1) { | ||
| if (text) { | ||
| return parsersql_command_type_pgsql(text, strlen(text)); |
There was a problem hiding this comment.
ParserSQL full mode currently regresses PostgreSQL command classification.
parsersql_command_type_pgsql() in lib/Query_Processor_ParserSQL.cpp:296-330 only exposes coarse statement kinds, e.g. generic CREATE/ALTER/DROP map to PGSQL_QUERY_CREATE_TABLE/ALTER_TABLE/DROP_TABLE, while this classifier and enum PGSQL_QUERY_command still distinguish commands like CREATE VIEW, ALTER ROLE, DROP INDEX, COPY, DISCARD, and cursor statements. With this unconditional early return, enabling pgsql-query_processor_parser=1 will misclassify those statements and can change query-rule behavior and command stats. Keep the legacy tokenizer as a fallback until ParserSQL can emit the full PostgreSQL command surface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/PgSQL_Query_Processor.cpp` around lines 671 - 673, The early return on
pgsql_thread___query_processor_parser should not unconditionally use
parsersql_command_type_pgsql(text, strlen(text)) because ParserSQL currently
collapses many PostgreSQL statement kinds; change the branch in
PgSQL_Query_Processor (the block guarded by
pgsql_thread___query_processor_parser) to call parsersql_command_type_pgsql as a
first attempt but, if it returns a coarse/generic result (e.g. the generic
CREATE/ALTER/DROP values that map to PGSQL_QUERY_CREATE_TABLE / ALTER_TABLE /
DROP_TABLE) then fall back to the legacy tokenizer/classifier used previously
(the original query-processing classification code in this file) so that
specific commands like CREATE VIEW, ALTER ROLE, DROP INDEX, COPY, DISCARD and
cursor statements are preserved; implement this by comparing the parsersql
result against the generic enums in enum PGSQL_QUERY_command and invoking the
legacy classification path when a generic value is returned.
| if (pgsql_thread___query_processor_parser == 1) { | ||
| if (text) { | ||
| return parsersql_command_type_pgsql(text, strlen(text)); | ||
| } | ||
| return PGSQL_QUERY_UNKNOWN; |
There was a problem hiding this comment.
Preserve query_prefix cleanup on the ParserSQL path.
These early returns bypass the existing cleanup at Lines 1166-1169, so any call that populated qp->query_prefix now leaks that buffer when pgsql_thread___query_processor_parser == 1.
Suggested fix
if (pgsql_thread___query_processor_parser == 1) {
- if (text) {
- return parsersql_command_type_pgsql(text, strlen(text));
- }
- return PGSQL_QUERY_UNKNOWN;
+ ret = text ? parsersql_command_type_pgsql(text, strlen(text)) : PGSQL_QUERY_UNKNOWN;
+ if (qp->query_prefix) {
+ free(qp->query_prefix);
+ qp->query_prefix = NULL;
+ }
+ return ret;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (pgsql_thread___query_processor_parser == 1) { | |
| if (text) { | |
| return parsersql_command_type_pgsql(text, strlen(text)); | |
| } | |
| return PGSQL_QUERY_UNKNOWN; | |
| if (pgsql_thread___query_processor_parser == 1) { | |
| ret = text ? parsersql_command_type_pgsql(text, strlen(text)) : PGSQL_QUERY_UNKNOWN; | |
| if (qp->query_prefix) { | |
| free(qp->query_prefix); | |
| qp->query_prefix = NULL; | |
| } | |
| return ret; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/PgSQL_Query_Processor.cpp` around lines 671 - 675, The early-return
branch guarded by pgsql_thread___query_processor_parser == 1 leaks
qp->query_prefix because it returns before the cleanup; modify that branch in
the function containing parsersql_command_type_pgsql calls to perform the same
cleanup of qp->query_prefix (free/null or call the existing cleanup helper used
elsewhere) before returning the result from parsersql_command_type_pgsql(text,
strlen(text)) or PGSQL_QUERY_UNKNOWN so the buffer is not leaked.
| qp->digest_text = strdup(normalized.c_str()); | ||
| // SpookyHash is preserved for backward compatibility with existing digest stats | ||
| qp->digest = SpookyHash::Hash64(normalized.c_str(), normalized.size(), 0); |
There was a problem hiding this comment.
Honor query_digests_max_digest_length when hashing.
The legacy path hashes only the first query_digests_max_digest_length bytes of digest_text, but this adapter hashes the entire normalized string. That changes digest IDs for long queries and breaks the compatibility goal for any rule/stat keyed on the legacy digest.
Suggested fix
- qp->digest = SpookyHash::Hash64(normalized.c_str(), normalized.size(), 0);
+ const size_t digest_len = std::min(
+ normalized.size(),
+ static_cast<size_t>(GET_THREAD_VARIABLE(query_digests_max_digest_length)));
+ qp->digest = SpookyHash::Hash64(normalized.c_str(), digest_len, 0);Also applies to: 235-236
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/Query_Processor_ParserSQL.cpp` around lines 206 - 208, The adapter
currently sets qp->digest_text = strdup(normalized.c_str()) then computes
qp->digest over the full normalized string; change the hashing to only use up to
query_digests_max_digest_length bytes (same behavior as the legacy path) by
truncating or passing the minimum of normalized.size() and
query_digests_max_digest_length to SpookyHash::Hash64 when computing qp->digest
(and ensure qp->digest_text still contains the full or legacy-truncated text as
required); apply the same fix to the second occurrence that computes qp->digest
later in this file.
| "parsersql_digest_test-t" : [ "unit-tests-g1" ], | ||
| "setparser_parsersql_test-t" : [ "unit-tests-g1" ], |
There was a problem hiding this comment.
setparser_parsersql_test-t in unit-tests-g1 will break CI given 22 known failures
Per the PR description ("202/224 SET pass, 22 known ParserSQL bugs filed as issues"), setparser_parsersql_test-t has 22 test cases that will emit failing ok() assertions and cause the binary to exit non-zero. Registering it in unit-tests-g1 will therefore fail that existing CI group on every run.
Either:
- Move it exclusively to
set_parser_algorithm=3-g1until the 22 upstream ParserSQL bugs are fixed, or - Wrap the known-failing assertions with
todo()markers in the test binary so TAP treats them as expected failures without breaking the exit status.
🔧 Option 1 — Move to `set_parser_algorithm=3-g1` only
- "setparser_parsersql_test-t" : [ "unit-tests-g1" ],
+ "setparser_parsersql_test-t" : [ "set_parser_algorithm=3-g1" ],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "parsersql_digest_test-t" : [ "unit-tests-g1" ], | |
| "setparser_parsersql_test-t" : [ "unit-tests-g1" ], | |
| "parsersql_digest_test-t" : [ "unit-tests-g1" ], | |
| "setparser_parsersql_test-t" : [ "set_parser_algorithm=3-g1" ], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/groups/groups.json` around lines 196 - 197, The entry
"setparser_parsersql_test-t" currently registered in "unit-tests-g1" will fail
CI due to 22 known ParserSQL failures; fix by either removing that key from the
"unit-tests-g1" array and adding it exclusively to the
"set_parser_algorithm=3-g1" group in groups.json (so only that alternate parser
group runs it), or modify the test binary setparser_parsersql_test-t to wrap the
22 known-failing ok() assertions with todo() markers (identify and mark the
specific test IDs/assertions inside setparser_parsersql_test-t so TAP treats
them as expected failures).
…pectations - Vendored ParserSQL 1.0.1 (was 1.0.0) with fixes for issues #34, #35, #36: - #34: Comma-separated assignments after SET NAMES/CHARSET - #35: ON/OFF keyword values accepted as SET RHS - #36: Empty AST detection for incomplete SET statements - New: NAMES/CHARSET/CHARACTER SET recognized after commas - Updated deps/Makefile and makefiles_paths.mk for parsersql-1.0.1 - Added -lsqlparser to TAP test link flags (MYLIBS_STATIC_PART, LDIRS) - Updated setparser_test_common.h expectations: - Invalid subquery tests now produce extracted paren values (sql_mode 23-25) - SET with empty RHS skips invalid assignment, returns valid ones (Set1_v2 0) - Complex nested function calls now fully parsed (Set1_v2 1) - Syntax errors partially parsed (syntax_errors 0-1) - All 224 SET tests pass, all 51 digest tests pass
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟡 Minor comments (2)
deps/Makefile-388-388 (1)
388-388:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDeclare
parsersqlas phony.This alias target has the same name as an existing directory, so
makecan treat it as up to date based on filesystem timestamps instead of always evaluating the dependency chain forlibsqlparser.a.Suggested fix
parsersql: parsersql/parsersql-1.0.1/libsqlparser.a + +.PHONY: parsersql🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/Makefile` at line 388, The Makefile target named "parsersql" shadows an existing directory, so mark it phony to force evaluating its dependency chain; add a .PHONY declaration for the parsersql target (referencing the target name "parsersql" and its dependency "parsersql/parsersql-1.0.1/libsqlparser.a") so make will always run the rule rather than treating the directory as up-to-date.deps/parsersql/parsersql-1.0.1/include/sql_engine/shard_map.h-170-173 (1)
170-173:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard null
StringRefs before building the lookup key.
std::string(table_name.ptr, table_name.len)is undefined whenptr == nullptr. A caller probing with an empty/missing table name can crash this path instead of just gettingnullptr.Suggested fix
const TableShardConfig* lookup(sql_parser::StringRef table_name) const { + if (!table_name.ptr || table_name.len == 0) { + return nullptr; + } std::string key = to_lower(std::string(table_name.ptr, table_name.len)); auto it = tables_.find(key); return it == tables_.end() ? nullptr : &it->second; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/parsersql/parsersql-1.0.1/include/sql_engine/shard_map.h` around lines 170 - 173, The lookup method in shard_map.h builds a std::string from sql_parser::StringRef without guarding against null ptrs; modify lookup(sql_parser::StringRef table_name) in class using tables_ so it first checks table_name.ptr != nullptr and table_name.len > 0 (or treat len==0 as empty) and immediately return nullptr for invalid inputs, and only call std::string(table_name.ptr, table_name.len) and to_lower when the StringRef is valid; keep the return logic (return nullptr or &it->second) unchanged otherwise.
🧹 Nitpick comments (4)
deps/parsersql/parsersql-1.0.1/include/sql_engine/transaction_manager.h (1)
37-41: ⚡ Quick winSet the fallback
route_dml()result to failure explicitly.The comment says this path returns
success=false, but the implementation only fillserror_message. Writing the flag here avoids coupling the behavior toDmlResult's default constructor.Small contract hardening
virtual DmlResult route_dml(const char* /*backend_name*/, sql_parser::StringRef /*sql*/) { DmlResult r; + r.success = false; r.error_message = "route_dml not supported by this transaction manager"; return r; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/parsersql/parsersql-1.0.1/include/sql_engine/transaction_manager.h` around lines 37 - 41, The fallback implementation of transaction_manager::route_dml currently only sets DmlResult::error_message but relies on DmlResult's default to mark failure; explicitly set r.success = false in the route_dml fallback to harden the contract so the method unambiguously returns failure (modify the virtual DmlResult route_dml(...) implementation where DmlResult r is constructed to assign r.success = false along with the existing error_message).deps/parsersql/parsersql-1.0.1/include/sql_parser/keyword_hash.h (1)
61-67: ⚡ Quick winGuard table construction against probe-depth drift.
build_table()keeps probing until it finds a slot, whilelookup_in_table()gives up afterMAX_PROBE. If a future keyword/table change creates a longer cluster, those entries will insert successfully and then never be found. Please fail fast during table build instead of silently degrading tokenization.Suggested guard
+#include <cassert> + template <typename KeywordEntry, size_t N> inline void build_table(const KeywordEntry (&keywords)[N], HashEntry (&table)[TABLE_SIZE]) { // Zero-init for (uint32_t i = 0; i < TABLE_SIZE; ++i) { table[i] = {nullptr, 0, TokenType::TK_IDENTIFIER}; } for (size_t k = 0; k < N; ++k) { uint32_t idx = hash_keyword(keywords[k].text, keywords[k].len) & TABLE_MASK; + uint32_t probe = 0; while (table[idx].text != nullptr) { + assert(probe < MAX_PROBE && "keyword hash cluster exceeds MAX_PROBE"); + ++probe; idx = (idx + 1) & TABLE_MASK; } table[idx] = {keywords[k].text, keywords[k].len, keywords[k].token}; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/parsersql/parsersql-1.0.1/include/sql_parser/keyword_hash.h` around lines 61 - 67, build_table() currently probes until it finds an empty slot but lookup_in_table() bails after MAX_PROBE, so change the insertion loop in build_table (the for loop that uses hash_keyword, TABLE_MASK and writes into table[]) to count probes and fail fast if the probe count exceeds MAX_PROBE; i.e., when computing idx = (idx + 1) & TABLE_MASK, increment a probe counter and if probe_count > MAX_PROBE then return an error/false or assert (or otherwise abort initialization) so insertion cannot silently succeed when lookup_in_table() would never find the entry; keep the same use of hash_keyword, TABLE_MASK, table, keywords[] and MAX_PROBE symbols.deps/parsersql/parsersql-1.0.1/include/sql_engine/operators/remote_scan_op.h (1)
18-33: ⚡ Quick winReset
results_during lifecycle transitions to avoid stale reads and retained buffers.Line 21 can leave prior
results_intact whenexecutor_is null, and Line 32 only resets the cursor. Reused operators can keep old rows/memory longer than needed.Proposed patch
void open() override { cursor_ = 0; + results_ = ResultSet{}; if (executor_) { results_ = executor_->execute(backend_name_, remote_sql_); } } @@ void close() override { cursor_ = 0; + results_ = ResultSet{}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/parsersql/parsersql-1.0.1/include/sql_engine/operators/remote_scan_op.h` around lines 18 - 33, open() may leave previous results_ intact when executor_ is null and close() only resets cursor_, causing stale rows to be retained; update lifecycle handling so open() clears results_ before or when executor_ is absent and close() also resets/clears results_ to release buffers. In practice, in the remote_scan_op methods (open(), next(), close()) ensure you explicitly reset the results_ member (e.g., assign an empty result object) whenever you reset cursor_ or fail to re-populate results_ so subsequent calls to next() don't see stale rows and memory is freed; use the existing members/results_ and keep checks against results_.rows in next() as-is.deps/parsersql/parsersql-1.0.1/include/sql_engine/thread_safe_executor.h (1)
291-292: ⚡ Quick winUse the repo's pthread mutex primitive here.
This adds
std::mutex/std::lock_guardin a path the codebase standardizes on pthread mutexes. Please switchmu_to the project mutex type so this executor follows the same synchronization model as the rest of the runtime.As per coding guidelines, "Use pthread mutexes for synchronization and
std::atomic<>for counters".Also applies to: 375-376
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4939d0f3-da38-483e-9b97-fde6fe1a6fbd
⛔ Files ignored due to path filters (1)
deps/parsersql/parsersql-1.0.1.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (106)
.gitignoredeps/Makefiledeps/parsersql/parsersql-1.0.1/Makefiledeps/parsersql/parsersql-1.0.1/include/sql_engine/backend_config.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/catalog.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/catalog_resolver.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/coercion.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/connection_pool.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/data_source.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/datetime_parse.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/distributed_planner.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/distributed_txn.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/dml_plan_builder.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/dml_result.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/durable_txn_log.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/engine_limits.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/expression_eval.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/function_registry.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/functions/arithmetic.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/functions/cast.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/functions/comparison.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/functions/datetime.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/functions/string.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/in_memory_catalog.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/like.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/local_txn.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/multi_remote_executor.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/mutable_data_source.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/mysql_remote_executor.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/null_semantics.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operator.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/aggregate_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/derived_scan_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/distinct_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/filter_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/hash_join_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/join_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/limit_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/merge_aggregate_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/merge_sort_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/project_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/remote_scan_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/scan_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/set_op_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/sort_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/window_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/optimizer.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/pgsql_remote_executor.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/plan_builder.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/plan_executor.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/plan_node.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/remote_executor.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/remote_query_builder.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/remote_session.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/result_set.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/row.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/rules/constant_folding.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/rules/limit_pushdown.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/rules/predicate_pushdown.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/session.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/shard_map.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/single_backend_txn.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/subquery_executor.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/tag_kind_map.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/thread_pool.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/thread_safe_executor.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/tool_config_parser.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/transaction_manager.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/transaction_recovery.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/types.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/value.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/arena.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/ast.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/common.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/compound_query_parser.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/delete_parser.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/digest.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/emitter.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/expression_parser.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/insert_parser.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/keyword_hash.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/keywords_mysql.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/keywords_pgsql.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/parse_result.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/parser.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/select_parser.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/set_parser.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/stmt_cache.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/string_builder.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/subquery_parse_callback.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/table_ref_parser.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/token.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/tokenizer.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/update_parser.hdeps/parsersql/parsersql-1.0.1/src/sql_engine/datetime_parse.cppdeps/parsersql/parsersql-1.0.1/src/sql_engine/function_registry.cppdeps/parsersql/parsersql-1.0.1/src/sql_engine/in_memory_catalog.cppdeps/parsersql/parsersql-1.0.1/src/sql_engine/multi_remote_executor.cppdeps/parsersql/parsersql-1.0.1/src/sql_engine/mysql_remote_executor.cppdeps/parsersql/parsersql-1.0.1/src/sql_engine/pgsql_remote_executor.cppdeps/parsersql/parsersql-1.0.1/src/sql_engine/tool_config_parser.cppdeps/parsersql/parsersql-1.0.1/src/sql_parser/arena.cppdeps/parsersql/parsersql-1.0.1/src/sql_parser/parser.cppinclude/makefiles_paths.mktest/tap/tests/Makefiletest/tap/tests/setparser_test_common.h
✅ Files skipped from review due to trivial changes (8)
- deps/parsersql/parsersql-1.0.1/include/sql_engine/connection_pool.h
- deps/parsersql/parsersql-1.0.1/include/sql_engine/operator.h
- deps/parsersql/parsersql-1.0.1/include/sql_engine/backend_config.h
- deps/parsersql/parsersql-1.0.1/include/sql_parser/parse_result.h
- .gitignore
- deps/parsersql/parsersql-1.0.1/include/sql_parser/string_builder.h
- deps/parsersql/parsersql-1.0.1/include/sql_engine/operators/scan_op.h
- deps/parsersql/parsersql-1.0.1/include/sql_parser/keywords_mysql.h
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names should use PascalCase with protocol prefixes (MySQL_,PgSQL_,ProxySQL_)
Member variables should use snake_case naming
Constants and macros should use UPPER_SNAKE_CASE naming
C++17 is required; use conditional compilation via#ifdef PROXYSQLGENAI,#ifdef PROXYSQL31, etc. for feature-gated code
Use jemalloc for memory allocation
Use pthread mutexes for synchronization andstd::atomic<>for counters
Files:
deps/parsersql/parsersql-1.0.1/include/sql_engine/operators/derived_scan_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/engine_limits.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/thread_pool.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/dml_result.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/distinct_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/catalog_resolver.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/remote_scan_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/single_backend_txn.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/pgsql_remote_executor.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/project_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/tag_kind_map.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/data_source.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/optimizer.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/mysql_remote_executor.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/remote_session.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/row.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/subquery_executor.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/merge_sort_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/sort_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/tool_config_parser.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/multi_remote_executor.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/subquery_parse_callback.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/transaction_manager.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/limit_op.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/keyword_hash.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/remote_executor.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/like.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/filter_op.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/arena.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/functions/comparison.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/set_parser.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/ast.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/function_registry.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/catalog.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/keywords_pgsql.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/rules/limit_pushdown.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/table_ref_parser.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/functions/cast.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/in_memory_catalog.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/merge_aggregate_op.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/compound_query_parser.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/common.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/remote_query_builder.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/rules/constant_folding.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/hash_join_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/result_set.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/datetime_parse.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/coercion.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/null_semantics.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/delete_parser.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/parser.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/dml_plan_builder.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/transaction_recovery.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/stmt_cache.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/types.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/mutable_data_source.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/rules/predicate_pushdown.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/expression_eval.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/select_parser.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/window_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/local_txn.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/durable_txn_log.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/value.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/plan_builder.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/join_op.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/digest.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/expression_parser.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/insert_parser.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/session.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/thread_safe_executor.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/functions/datetime.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/shard_map.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/set_op_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/plan_executor.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/plan_node.hdeps/parsersql/parsersql-1.0.1/include/sql_parser/emitter.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/distributed_planner.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/functions/string.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/operators/aggregate_op.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/distributed_txn.hdeps/parsersql/parsersql-1.0.1/include/sql_engine/functions/arithmetic.h
🪛 checkmake (0.3.2)
deps/parsersql/parsersql-1.0.1/Makefile
[warning] 232-232: Target body for "clean" exceeds allowed length of 5 lines (9).
(maxbodylength)
deps/Makefile
[warning] 388-388: Target "parsersql" should be declared PHONY.
(phonydeclared)
🔇 Additional comments (3)
deps/parsersql/parsersql-1.0.1/include/sql_parser/delete_parser.h (1)
66-123: This review comment is factually incorrect about MySQL DELETE syntax.The web search confirms that
DELETE FROM t1, t2 WHERE ...is valid MySQL syntax—not invalid as claimed. MySQL supports multiple-table DELETE in this form without requiring USING. The suggested fix would reject syntactically valid input.If the parser intends to support MySQL-compatible syntax, it should not reject this form. The code's handling of collecting multiple targets and then parsing WHERE/ORDER BY/LIMIT appears reasonable, though note that MySQL does not support ORDER BY and LIMIT in multi-table DELETE statements, which the current parser may not enforce.
> Likely an incorrect or invalid review comment.deps/parsersql/parsersql-1.0.1/include/sql_parser/keywords_pgsql.h (1)
155-163: The code is safe as written. The hash table is zero-initialized bybuild_table()and safely handles uninitialized state vialookup_in_table()(which returnsTK_IDENTIFIERfor empty slots). More importantly, lookup() is never called from global or static initializers—it is only invoked during runtime parsing viaParser::parse(), which is itself accessed only through runtime functions. The cross-translation-unit initialization order concern described does not apply to this codebase.deps/parsersql/parsersql-1.0.1/include/sql_engine/result_set.h (1)
52-74: The code is correct as written. C++17 explicitly guarantees thatstd::vectoris empty after move operations, so the destructor of the moved-from object will not free anything. No explicitclear()is needed.> Likely an incorrect or invalid review comment.
| if (!log_decision_or_fail(DurableTransactionLog::Decision::ROLLBACK)) { | ||
| active_ = false; | ||
| sessions_.clear(); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Always attempt phase-2 rollback before dropping the transaction.
If phase 1 fails and require_durable_log_ makes log_decision_or_fail(ROLLBACK) return false, this path clears sessions_ and exits without calling phase2_rollback(). Any participant that already ran XA START/XA PREPARE is left hanging with no pinned session and no WAL record to recover from.
Suggested direction
if (!phase1_prepare()) {
if (!log_decision_or_fail(DurableTransactionLog::Decision::ROLLBACK)) {
+ phase2_rollback(); // best effort before dropping pinned sessions
active_ = false;
sessions_.clear();
return false;
}
phase2_rollback();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deps/parsersql/parsersql-1.0.1/include/sql_engine/distributed_txn.h` around
lines 191 - 195, The current early-return after
log_decision_or_fail(Decision::ROLLBACK) skips phase2_rollback(), leaving
prepared participants orphaned; change the control flow in the function that
contains log_decision_or_fail and sessions_ so that even if
log_decision_or_fail(ROLLBACK) returns false (e.g., when require_durable_log_
forces failure), you still call phase2_rollback() before clearing sessions_ and
setting active_ = false and returning false; ensure you preserve the failure
return value from log_decision_or_fail while still invoking phase2_rollback()
and then clear sessions_ and deactivate the transaction.
| static void append_value_to_key(std::string& key, const Value& v) { | ||
| if (v.is_null()) { key += "N"; return; } | ||
| switch (v.tag) { | ||
| case Value::TAG_BOOL: key += v.bool_val ? "T" : "F"; break; | ||
| case Value::TAG_INT64: key += std::to_string(v.int_val); break; | ||
| case Value::TAG_UINT64: key += std::to_string(v.uint_val); break; | ||
| case Value::TAG_DOUBLE: key += std::to_string(v.double_val); break; | ||
| case Value::TAG_STRING: | ||
| key.append(v.str_val.ptr, v.str_val.len); | ||
| break; | ||
| default: key += "?"; break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Make the composite group key collision-free.
append_value_to_key() conflates distinct values (NULL vs "N", true vs "T", numeric 123 vs string "123"), and raw string payloads can also contain the '\x01' separator. That can collapse different groups into the same hash bucket and silently corrupt merged aggregates.
Possible fix
+ static void append_u32(std::string& key, uint32_t n) {
+ key.append(reinterpret_cast<const char*>(&n), sizeof(n));
+ }
+
static void append_value_to_key(std::string& key, const Value& v) {
- if (v.is_null()) { key += "N"; return; }
+ key.push_back(static_cast<char>(v.tag));
+ if (v.is_null()) return;
switch (v.tag) {
- case Value::TAG_BOOL: key += v.bool_val ? "T" : "F"; break;
- case Value::TAG_INT64: key += std::to_string(v.int_val); break;
- case Value::TAG_UINT64: key += std::to_string(v.uint_val); break;
- case Value::TAG_DOUBLE: key += std::to_string(v.double_val); break;
+ case Value::TAG_BOOL:
+ key.push_back(v.bool_val ? '\1' : '\0');
+ break;
+ case Value::TAG_INT64:
+ key.append(reinterpret_cast<const char*>(&v.int_val), sizeof(v.int_val));
+ break;
+ case Value::TAG_UINT64:
+ key.append(reinterpret_cast<const char*>(&v.uint_val), sizeof(v.uint_val));
+ break;
+ case Value::TAG_DOUBLE:
+ key.append(reinterpret_cast<const char*>(&v.double_val), sizeof(v.double_val));
+ break;
case Value::TAG_STRING:
+ append_u32(key, v.str_val.len);
key.append(v.str_val.ptr, v.str_val.len);
break;
default: key += "?"; break;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static void append_value_to_key(std::string& key, const Value& v) { | |
| if (v.is_null()) { key += "N"; return; } | |
| switch (v.tag) { | |
| case Value::TAG_BOOL: key += v.bool_val ? "T" : "F"; break; | |
| case Value::TAG_INT64: key += std::to_string(v.int_val); break; | |
| case Value::TAG_UINT64: key += std::to_string(v.uint_val); break; | |
| case Value::TAG_DOUBLE: key += std::to_string(v.double_val); break; | |
| case Value::TAG_STRING: | |
| key.append(v.str_val.ptr, v.str_val.len); | |
| break; | |
| default: key += "?"; break; | |
| } | |
| } | |
| static void append_u32(std::string& key, uint32_t n) { | |
| key.append(reinterpret_cast<const char*>(&n), sizeof(n)); | |
| } | |
| static void append_value_to_key(std::string& key, const Value& v) { | |
| key.push_back(static_cast<char>(v.tag)); | |
| if (v.is_null()) return; | |
| switch (v.tag) { | |
| case Value::TAG_BOOL: | |
| key.push_back(v.bool_val ? '\1' : '\0'); | |
| break; | |
| case Value::TAG_INT64: | |
| key.append(reinterpret_cast<const char*>(&v.int_val), sizeof(v.int_val)); | |
| break; | |
| case Value::TAG_UINT64: | |
| key.append(reinterpret_cast<const char*>(&v.uint_val), sizeof(v.uint_val)); | |
| break; | |
| case Value::TAG_DOUBLE: | |
| key.append(reinterpret_cast<const char*>(&v.double_val), sizeof(v.double_val)); | |
| break; | |
| case Value::TAG_STRING: | |
| append_u32(key, v.str_val.len); | |
| key.append(v.str_val.ptr, v.str_val.len); | |
| break; | |
| default: key += "?"; break; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@deps/parsersql/parsersql-1.0.1/include/sql_engine/operators/merge_aggregate_op.h`
around lines 231 - 243, The current append_value_to_key (in
merge_aggregate_op.h) produces ambiguous composite keys (e.g., NULL vs "N", bool
vs char, numbers vs numeric-strings and raw strings containing separators);
change it to emit an unambiguous, type-prefixed encoding for each Value: write a
single type tag byte (or short token) for Value::TAG_* first, then a
length-prefixed payload for variable-length data (strings) or a fixed-byte
canonical representation for numbers/bools, and use an escaping or explicit
length for any separator so raw string bytes cannot collide (e.g., "S" + length
+ data, "I"+binary-int, "N" for null, "B"+('0'|'1') for bool). Update
append_value_to_key to use these unique type tokens and length/escaping so keys
are collision-free while retaining existing Value tag names for location.
| // Pattern: Filter above Join | ||
| if (node->type != PlanNodeType::FILTER) return node; | ||
| if (!node->left || node->left->type != PlanNodeType::JOIN) return node; | ||
|
|
||
| PlanNode* join = node->left; | ||
| const sql_parser::AstNode* filter_expr = node->filter.expr; | ||
| if (!filter_expr) return node; | ||
|
|
||
| // Collect tables from left and right sides of join | ||
| constexpr uint16_t MAX_TABLES = 16; | ||
| const char* left_tables[MAX_TABLES]; uint32_t left_lens[MAX_TABLES]; uint16_t left_count = 0; | ||
| const char* right_tables[MAX_TABLES]; uint32_t right_lens[MAX_TABLES]; uint16_t right_count = 0; | ||
|
|
||
| detail_ppd::collect_plan_tables(join->left, left_tables, left_lens, left_count, MAX_TABLES); | ||
| detail_ppd::collect_plan_tables(join->right, right_tables, right_lens, right_count, MAX_TABLES); | ||
|
|
||
| // Split filter into AND-separated predicates | ||
| constexpr uint16_t MAX_PREDS = 32; | ||
| const sql_parser::AstNode* predicates[MAX_PREDS]; | ||
| uint16_t pred_count = 0; | ||
| detail_ppd::collect_and_predicates(filter_expr, predicates, pred_count, MAX_PREDS); | ||
|
|
||
| // Classify each predicate | ||
| const sql_parser::AstNode* left_preds[MAX_PREDS]; uint16_t lp_count = 0; | ||
| const sql_parser::AstNode* right_preds[MAX_PREDS]; uint16_t rp_count = 0; | ||
| const sql_parser::AstNode* stay_preds[MAX_PREDS]; uint16_t sp_count = 0; | ||
|
|
||
| for (uint16_t i = 0; i < pred_count; ++i) { | ||
| const char* expr_tables[MAX_TABLES]; uint32_t expr_lens[MAX_TABLES]; uint16_t expr_count = 0; | ||
| detail_ppd::collect_expr_table_refs(predicates[i], catalog, | ||
| expr_tables, expr_lens, expr_count, MAX_TABLES); | ||
|
|
||
| if (expr_count == 0) { | ||
| // No qualified refs — can't determine side, leave in place | ||
| stay_preds[sp_count++] = predicates[i]; | ||
| } else { | ||
| bool fits_left = detail_ppd::all_refs_in(expr_tables, expr_lens, expr_count, | ||
| left_tables, left_lens, left_count); | ||
| bool fits_right = detail_ppd::all_refs_in(expr_tables, expr_lens, expr_count, | ||
| right_tables, right_lens, right_count); | ||
| if (fits_left && !fits_right) { | ||
| left_preds[lp_count++] = predicates[i]; | ||
| } else if (fits_right && !fits_left) { | ||
| right_preds[rp_count++] = predicates[i]; | ||
| } else { | ||
| stay_preds[sp_count++] = predicates[i]; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // If nothing to push, return unchanged | ||
| if (lp_count == 0 && rp_count == 0) return node; | ||
|
|
||
| // Push predicates to left side | ||
| if (lp_count > 0) { | ||
| PlanNode* lf = make_plan_node(arena, PlanNodeType::FILTER); | ||
| lf->filter.expr = detail_ppd::build_and_chain(left_preds, lp_count, arena); | ||
| lf->left = join->left; | ||
| join->left = lf; | ||
| } | ||
|
|
||
| // Push predicates to right side | ||
| if (rp_count > 0) { | ||
| PlanNode* rf = make_plan_node(arena, PlanNodeType::FILTER); | ||
| rf->filter.expr = detail_ppd::build_and_chain(right_preds, rp_count, arena); | ||
| rf->left = join->right; | ||
| join->right = rf; | ||
| } | ||
|
|
||
| // If all predicates were pushed, remove the filter node entirely | ||
| if (sp_count == 0) { | ||
| return join; | ||
| } |
There was a problem hiding this comment.
Make pushdown join-type aware to avoid wrong results on outer joins.
Current classification/push can remove the original filter (Line 258) even for outer joins. Example: pushing a right-only predicate below a LEFT JOIN changes semantics by preserving null-extended left rows that the original WHERE would remove.
Suggested guard strategy
PlanNode* join = node->left;
+ const uint8_t join_type = join->join.join_type;
@@
- // Push predicates to left side
+ // Outer-join safety: only push to preserved side.
+ // LEFT JOIN: allow left push only.
+ // RIGHT JOIN: allow right push only.
+ // FULL JOIN: do not push either side.
+ if (join_type == JOIN_FULL) {
+ lp_count = 0;
+ rp_count = 0;
+ } else if (join_type == JOIN_LEFT) {
+ for (uint16_t i = 0; i < rp_count; ++i) stay_preds[sp_count++] = right_preds[i];
+ rp_count = 0;
+ } else if (join_type == JOIN_RIGHT) {
+ for (uint16_t i = 0; i < lp_count; ++i) stay_preds[sp_count++] = left_preds[i];
+ lp_count = 0;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deps/parsersql/parsersql-1.0.1/include/sql_engine/rules/predicate_pushdown.h`
around lines 188 - 260, The filter pushdown currently ignores join type and can
change semantics for outer joins; update the logic around join (PlanNode* join)
so push is only allowed when safe: consult the join node's join type field
(e.g., join->join_type or join->type_name) and forbid pushing predicates from
the nullable side—for LEFT JOIN do not push right_preds, for RIGHT JOIN do not
push left_preds, and for FULL OUTER prevent pushing to either side; only create
lf/rf and attach left_preds/right_preds when allowed by join type, and only
return join (i.e., remove the original FILTER node) when sp_count==0 AND the
join is an inner join (or otherwise proven safe).
| // If sharding is configured, distribute DML to remote backends. | ||
| if (shard_map_ && remote_executor_) { | ||
| DistributedPlanner<D> dp(*shard_map_, catalog_, parser_.arena(), | ||
| remote_executor_, &functions_); | ||
| PlanNode* dist_plan = dp.distribute_dml(plan); | ||
|
|
||
| if (dist_plan && dist_plan->type == PlanNodeType::REMOTE_SCAN) { | ||
| // Single-shard DML | ||
| sql_parser::StringRef sql_ref{dist_plan->remote_scan.remote_sql, | ||
| dist_plan->remote_scan.remote_sql_len}; | ||
| if (txn_mgr_.in_transaction() && txn_mgr_.is_distributed()) { | ||
| result = txn_mgr_.route_dml(dist_plan->remote_scan.backend_name, sql_ref); | ||
| } else { | ||
| result = remote_executor_->execute_dml( | ||
| dist_plan->remote_scan.backend_name, sql_ref); | ||
| } | ||
| } else if (dist_plan && dist_plan->type == PlanNodeType::SET_OP) { | ||
| // Scatter DML to multiple shards | ||
| result.success = true; | ||
| result.affected_rows = 0; | ||
| for_each_remote_scan(dist_plan, [&](const PlanNode* rs) { | ||
| sql_parser::StringRef s{rs->remote_scan.remote_sql, | ||
| rs->remote_scan.remote_sql_len}; | ||
| DmlResult shard_result; | ||
| if (txn_mgr_.in_transaction() && txn_mgr_.is_distributed()) { | ||
| shard_result = txn_mgr_.route_dml(rs->remote_scan.backend_name, s); | ||
| } else { | ||
| shard_result = remote_executor_->execute_dml( | ||
| rs->remote_scan.backend_name, s); | ||
| } | ||
| if (!shard_result.success) { | ||
| result.success = false; | ||
| result.error_message = shard_result.error_message; | ||
| } | ||
| result.affected_rows += shard_result.affected_rows; | ||
| }); |
There was a problem hiding this comment.
Scatter DML can partially commit across shards.
The SET_OP path executes each shard write directly through RemoteExecutor unless a distributed transaction is already active, and it keeps iterating after failures. A mid-fanout error leaves earlier shards committed and later shards skipped, but the caller only gets success = false. This needs a coordinated distributed transaction or an explicit refusal to run multi-shard DML without one.
Safer minimum guard
} else if (dist_plan && dist_plan->type == PlanNodeType::SET_OP) {
+ if (!txn_mgr_.in_transaction() || !txn_mgr_.is_distributed()) {
+ result.error_message = "scatter DML requires a distributed transaction";
+ return result;
+ }
// Scatter DML to multiple shards
result.success = true;
result.affected_rows = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deps/parsersql/parsersql-1.0.1/include/sql_engine/session.h` around lines 221
- 256, The SET_OP branch currently executes per-shard writes directly and can
partially commit; change it to refuse running multi-shard DML unless the session
is in a distributed transaction: inside the PlanNodeType::SET_OP branch (where
for_each_remote_scan is used), first check txn_mgr_.in_transaction() &&
txn_mgr_.is_distributed(); if that check fails, set result.success = false, set
a clear error_message indicating "multi-shard DML requires a distributed
transaction" and return/skip executing any remote_executor_->execute_dml calls;
otherwise (when distributed) proceed with routing via txn_mgr_.route_dml for
each shard as currently implemented to ensure coordinated commit.
…tracted source The extracted parsersql-1.0.1/ source directory was committed to git, which violates the dependency convention. All other deps (re2, curl, libev, etc.) only track the tarball and a versioned symlink, with the extracted dir generated at build time. Changes: - git rm --cached parsersql-1.0.1/ (90 files removed from tracking) - Added symlink parsersql -> parsersql-1.0.1 (tracked) - Added deps/parsersql/parsersql-*/ to .gitignore - Updated deps/Makefile to use symlink path - Updated makefiles_paths.mk to use symlink path
Signed-off-by: René Cannaò <rene@proxysql.com>
parsersql_digest_test-t and setparser_parsersql_test-t were inserted out of order, breaking CI-lint-groups-json.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/tap/tests/Makefile`:
- Around line 419-434: The Makefile’s clean target needs to remove the two new
ParserSQL test binaries so they don’t persist; update the clean (and/or
distclean) recipe to delete setparser_parsersql_test and parsersql_digest_test
(and the symlink setparser_parsersql_test-t if present) so running make clean
removes these artifacts; locate the clean target in the same Makefile and add rm
-f entries for setparser_parsersql_test setparser_parsersql_test-t and
parsersql_digest_test (and mirror the change in any distclean target if used).
- Around line 419-434: Add the new ParserSQL tests to the default test flow by
wiring their targets into the tests: dependency list and aligning source-name
patterns; specifically, add the test binaries setparser_parsersql_test-t and
parsersql_digest_test-t (or add setparser_parsersql_test and
parsersql_digest_test if your Makefile uses those names) to the tests: target,
and ensure the source filenames follow the *-t.cpp wildcard (rename
setparser_parsersql_test.cpp -> setparser_parsersql_test-t.cpp and
parsersql_digest_test.cpp -> parsersql_digest_test-t.cpp or update the pattern
that builds tests). Update any TESTS or test list variables used by make tests
to include setparser_parsersql_test(-t) and parsersql_digest_test(-t) so make
tests will build and run them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ae650df-81e3-4450-a753-82883d363141
📒 Files selected for processing (13)
.gitignoredeps/Makefiledeps/parsersql/parsersqlinclude/MySQL_Thread.hinclude/makefiles_paths.mklib/Makefilelib/MySQL_Session.cpplib/MySQL_Thread.cpplib/PgSQL_Session.cpplib/PgSQL_Thread.cppsrc/Makefiletest/tap/groups/groups.jsontest/tap/tests/Makefile
✅ Files skipped from review due to trivial changes (5)
- include/MySQL_Thread.h
- deps/parsersql/parsersql
- .gitignore
- src/Makefile
- lib/PgSQL_Session.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- include/makefiles_paths.mk
- lib/Makefile
- lib/MySQL_Thread.cpp
- lib/PgSQL_Thread.cpp
- test/tap/groups/groups.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: run / trigger
- GitHub Check: claude-review
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_) for class names
Use snake_case for member variables
Use UPPER_SNAKE_CASE for constants and macros
Require C++17 standard for all code
Use conditional compilation via#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB,#ifdef PROXYSQLCLICKHOUSEfor feature-gated code
Use RAII for resource management throughout the codebase
Use jemalloc for memory allocation
Use pthread mutexes for synchronization andstd::atomic<>for counters
Files:
lib/MySQL_Session.cpp
🪛 checkmake (0.3.2)
deps/Makefile
[warning] 388-388: Target "parsersql" should be declared PHONY.
(phonydeclared)
test/tap/tests/Makefile
[warning] 422-422: Target "setparser_parsersql_test" should be declared PHONY.
(phonydeclared)
[warning] 429-429: Target "parsersql_digest_test" should be declared PHONY.
(phonydeclared)
🔇 Additional comments (1)
lib/MySQL_Session.cpp (1)
6531-6533: ParserSQL SET-routing condition looks correct.This condition cleanly enables ParserSQL SET parsing for both explicit algorithm
3and full parser mode (query_processor_parser == 1), which aligns with the integration intent.
| setparser_parsersql_test-t: setparser_parsersql_test | ||
| ln -fs setparser_parsersql_test setparser_parsersql_test-t | ||
|
|
||
| setparser_parsersql_test: setparser_parsersql_test.cpp $(TAP_LDIR)/libtap.so setparser_test_common.h $(LIBPROXYSQLAR) $(LIBCOREDUMPERAR) $(PARSERSQL_LDIR)/libsqlparser.a | ||
| ifeq ($(PROXYSQLGENAI),1) | ||
| $(CXX) $< -DEXCLUDE_TRACKING_VARIABLES $(IDIRS) $(LDIRS) $(OPT) $(MYLIBS) $(LIBCOREDUMPERAR) $(SQLITE3_LDIR)/vec.o $(PARSERSQL_LDIR)/libsqlparser.a -o $@ | ||
| else | ||
| $(CXX) $< -DEXCLUDE_TRACKING_VARIABLES $(IDIRS) $(LDIRS) $(OPT) $(MYLIBS) $(LIBCOREDUMPERAR) $(PARSERSQL_LDIR)/libsqlparser.a -o $@ | ||
| endif | ||
|
|
||
| parsersql_digest_test: parsersql_digest_test.cpp $(TAP_LDIR)/libtap.so $(LIBPROXYSQLAR) $(LIBCOREDUMPERAR) $(PARSERSQL_LDIR)/libsqlparser.a | ||
| ifeq ($(PROXYSQLGENAI),1) | ||
| $(CXX) $< -DEXCLUDE_TRACKING_VARIABLES $(IDIRS) $(LDIRS) $(OPT) $(MYLIBS) $(LIBCOREDUMPERAR) $(SQLITE3_LDIR)/vec.o $(PARSERSQL_LDIR)/libsqlparser.a -o $@ | ||
| else | ||
| $(CXX) $< -DEXCLUDE_TRACKING_VARIABLES $(IDIRS) $(LDIRS) $(OPT) $(MYLIBS) $(LIBCOREDUMPERAR) $(PARSERSQL_LDIR)/libsqlparser.a -o $@ | ||
| endif |
There was a problem hiding this comment.
Extend clean for the new ParserSQL binaries.
make clean still leaves setparser_parsersql_test and parsersql_digest_test behind, so stale binaries can linger across branch switches or incremental rebuilds.
🧹 Suggested fix
clean:
rm -f *-t || true
rm -f galera_1_timeout_count galera_2_timeout_no_count aurora || true
rm -f generate_set_session_csv set_testing-240.csv || true
rm -f setparser_test setparser_test2 setparser_test3 || true
+ rm -f setparser_parsersql_test parsersql_digest_test || true
rm -f reg_test_3504-change_user_libmariadb_helper reg_test_3504-change_user_libmysql_helper || true🧰 Tools
🪛 checkmake (0.3.2)
[warning] 422-422: Target "setparser_parsersql_test" should be declared PHONY.
(phonydeclared)
[warning] 429-429: Target "parsersql_digest_test" should be declared PHONY.
(phonydeclared)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/Makefile` around lines 419 - 434, The Makefile’s clean target
needs to remove the two new ParserSQL test binaries so they don’t persist;
update the clean (and/or distclean) recipe to delete setparser_parsersql_test
and parsersql_digest_test (and the symlink setparser_parsersql_test-t if
present) so running make clean removes these artifacts; locate the clean target
in the same Makefile and add rm -f entries for setparser_parsersql_test
setparser_parsersql_test-t and parsersql_digest_test (and mirror the change in
any distclean target if used).
Wire the new ParserSQL tests into the default test flow.
setparser_parsersql_test and parsersql_digest_test are defined here, but nothing adds them to tests: and their source names do not match the *-t.cpp wildcard. As a result, make tests will skip them entirely.
🛠️ Suggested fix
tests: CUSTOMARGS += $(OPT)
tests: tests-cpp \
tests-php \
tests-py \
tests-sh \
setparser_test \
+ setparser_parsersql_test \
+ parsersql_digest_test \
reg_test_3504-change_user_libmariadb_helper \
reg_test_3504-change_user_libmysql_helper \🧰 Tools
🪛 checkmake (0.3.2)
[warning] 422-422: Target "setparser_parsersql_test" should be declared PHONY.
(phonydeclared)
[warning] 429-429: Target "parsersql_digest_test" should be declared PHONY.
(phonydeclared)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/Makefile` around lines 419 - 434, Add the new ParserSQL tests
to the default test flow by wiring their targets into the tests: dependency list
and aligning source-name patterns; specifically, add the test binaries
setparser_parsersql_test-t and parsersql_digest_test-t (or add
setparser_parsersql_test and parsersql_digest_test if your Makefile uses those
names) to the tests: target, and ensure the source filenames follow the *-t.cpp
wildcard (rename setparser_parsersql_test.cpp -> setparser_parsersql_test-t.cpp
and parsersql_digest_test.cpp -> parsersql_digest_test-t.cpp or update the
pattern that builds tests). Update any TESTS or test list variables used by make
tests to include setparser_parsersql_test(-t) and parsersql_digest_test(-t) so
make tests will build and run them.
…on.cpp MySQL_Session.cpp calls parsersql_parse_set_mysql() declared in Query_Processor_ParserSQL.h but never included it, causing a compilation failure in all CI jobs that rebuild from scratch.
The unit test Makefile was missing: 1. -lsqlparser - resolves sql_parser::Parser symbols from Query_Processor_ParserSQL.cpp linked into libproxysql.a 2. -lprometheus-cpp-pull -lprometheus-cpp-core - resolves ~2700 prometheus symbols (pre-existing omission, always broken but masked by the parsersql link failure) 3. -I and -L for the parsersql include/library paths
- Replace parsersql-1.0.1.tar.gz with parsersql-1.0.2.tar.gz (GitHub release) - Update symlink parsersql -> parsersql-1.0.2 - Handle ParserSQL-* prefix from GitHub archives in deps/Makefile
Resolve conflict in test/tap/tests/unit/Makefile: keep v3.0's refactored linking (WHOLE_LIBPROXYSQL to avoid double-link under ASAN) and add -lsqlparser for parsersql integration.
- Extract skip_quoted_char() from extract_paren_expr() to reduce cognitive complexity (S3776, S134) in Query_Processor_ParserSQL.cpp - Extract resolve_var_value() and finalize_var_value() from walk_set_stmt() to reduce complexity from 39 to under 25 - Extract qp_digest_parsersql() and qp_digest_legacy() from query_parser_init() to reduce complexity from 30 to under 25 - Add NOSONAR annotations for C-style casts on string literals that follow existing codebase patterns (M23_090) - Add NOSONAR annotations for free() calls on C-allocated memory in test code (S1231)
…tions - Extract extract_names_values() and extract_charset_values() from walk_set_stmt() to reduce nesting depth (S134 at line 416) - Fix setparser_test2-t and setparser_test3-t expectations: the regex parser only recognizes CONCAT/REPLACE/IFNULL, so CONCA (typo), mismatched brackets, and SELCT all return empty results. Update the test data to expect empty maps instead of parsed values.
- Add parsersql_syntax_errors[] array with correct ParserSQL expectations: CONCA, mismatched brackets, and SELCT are all extracted structurally by ParserSQL (unlike the regex parser) - setparser_parsersql_test-t now uses parsersql_syntax_errors instead of the generic syntax_errors array - setparser_test2-t and setparser_test3-t keep using syntax_errors with empty expectations for invalid SQL (regex parser behavior)
- S5813 (strlen safety): all 3 instances use string literal prefixes or null-terminated array entries where strlen is compile-time evaluated or trivially safe - S7635/S7637 (GitHub Actions): branch ref and secrets:inherit match the pattern used by all other caller workflows in the repo
…ql_syntax_errors[] The shared header setparser_test_common.h must not contain ParserSQL-specific test expectations. syntax_errors[] and Set1_v2[] entries are reverted to their original regex-parser expectations (empty results for invalid SQL). The parsersql_syntax_errors[] array is moved into setparser_parsersql_test.cpp where ParserSQL-specific data belongs.
|
1 participant
Summary
deps/parsersql/) with SpookyHash for backward digest compatibilityQuery_Processor_ParserSQL.h/.cpp) exposing three capabilities: query digest, command-type classification, and SET statement parsing via ParserSQL's AST walkerset_parser_algorithm=3(SET-only mode, conservative) andquery_processor_parser=1(full mode: digest + command type + SET)set_parser_algorithm=3-g1with 14 SET-related integration tests and CI workflowQuery_Processor,MySQL_Query_Processor,PgSQL_Query_Processor,MySQL_Session, andPgSQL_SessionKey design decisions
thread_local Parser<D>— zero shared state, O(1) arena reset, matches ProxySQL threading modelset_parser_algorithm=3for SET-only,query_processor_parser=1for full mode; both default to legacy (off)Files changed (28 files, +2185/-45)
deps/Makefile,include/makefiles_paths.mk,lib/Makefile,src/Makefileinclude/Query_Processor_ParserSQL.h,lib/Query_Processor_ParserSQL.cppQuery_Processor.cpp,MySQL_Query_Processor.cpp,PgSQL_Query_Processor.cpp,MySQL_Session.cpp,PgSQL_Session.cppMySQL_Thread.h/cpp,PgSQL_Thread.h/cpp,proxysql_structs.hdeps/parsersql/parsersql-1.0.0.tar.gz(extracted)setparser_parsersql_test.cpp,parsersql_digest_test.cppCI-set_parser_algorithm=3-g1.yml,gh-actions-reusable/ci-set_parser_algorithm=3-g1.ymltest/tap/groups/set_parser_algorithm=3/{env.sh,infras.lst,pre-proxysql.sql},groups.jsonSummary by CodeRabbit
New Features
Tests
Documentation
CI / Build