fix(set): comma-separation after NAMES/CHARSET, ON keyword values, empty-result detection#37
Conversation
…word values, empty-result detection Issue #34: SET NAMES/CHARSET branches returned immediately without checking for comma-separated variable assignments. Added comma loops to all three branches (NAMES, CHARACTER SET, CHARSET). Issue #35: ON is a keyword (TK_ON) not recognized as an identifier by the expression parser's is_keyword_as_identifier() allowlist. Added TK_ON to the allowlist so SET sql_auto_is_null = ON works. Issue #36: SetParser::parse() always returned a NODE_SET_STMT root even when nothing was parsed, causing parse_set() to report OK for invalid input. Added empty-AST guards (return nullptr if no children) to all branches, plus defense-in-depth check in parse_set() for ast->first_child. parse_variable_assignment() now returns nullptr when no RHS expression was parsed. All 1213 tests pass.
📝 WalkthroughWalkthroughThe pull request enhances the SET statement parser to handle comma-separated items after ChangesSET Statement Parser Enhancement
Possibly Related Issues
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes involve new parsing logic across multiple files, with mixed complexity in the set parser (loops and conditional routing), straightforward validation in the main parser, and standard test assertions. CI changes are isolated and straightforward. 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 docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@include/sql_parser/set_parser.h`:
- Around line 32-36: The loops that consume TK_COMMA call
parse_variable_assignment(nullptr) and ignore a null result, which lets
malformed trailing assignments pass; update each loop (the one shown and the
similar loops in the CHARACTER SET and CHARSET branches) to treat a null
next_assign as a parse failure by returning nullptr (or otherwise failing the
parse) instead of continuing — locate the loops that use
tok_.peek()/tok_.skip(), call parse_variable_assignment(nullptr) and
root->add_child(next_assign) and add an if (!next_assign) return nullptr;
immediately after the parse_variable_assignment call in each of those loops.
In `@tests/test_set.cpp`:
- Around line 651-658: The test TEST(MySQLSetBulk, InvalidSyntaxReturnsPartial)
includes a case {"SET @@@ BROKEN", (int)ParseResult::OK} which contradicts the
test intent; update this case to expect ParseResult::PARTIAL or move it into a
new test (e.g., TEST(MySQLSetBulk, PermissiveAcceptsWeirdTokens)) if you intend
to assert permissiveness; specifically edit the array in tests/test_set.cpp to
replace (int)ParseResult::OK for the "SET @@@ BROKEN" entry with
(int)ParseResult::PARTIAL or extract that case into a separate test function and
assert OK there.
🪄 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: 40bf17d4-cb1f-4531-8159-e5922620b494
📒 Files selected for processing (4)
include/sql_parser/expression_parser.hinclude/sql_parser/set_parser.hsrc/sql_parser/parser.cpptests/test_set.cpp
| while (tok_.peek().type == TokenType::TK_COMMA) { | ||
| tok_.skip(); | ||
| AstNode* next_assign = parse_variable_assignment(nullptr); | ||
| if (next_assign) root->add_child(next_assign); | ||
| } |
There was a problem hiding this comment.
Do not silently ignore malformed assignments after commas.
At Line 32, Line 49, and Line 61, failed parse_variable_assignment(nullptr) results are skipped, so malformed trailing entries can still yield OK if an earlier child exists. Treat this as a parse failure once a comma has committed the parser to another assignment.
Proposed fix
- while (tok_.peek().type == TokenType::TK_COMMA) {
- tok_.skip();
- AstNode* next_assign = parse_variable_assignment(nullptr);
- if (next_assign) root->add_child(next_assign);
- }
+ while (tok_.peek().type == TokenType::TK_COMMA) {
+ tok_.skip();
+ AstNode* next_assign = parse_variable_assignment(nullptr);
+ if (!next_assign) return nullptr;
+ root->add_child(next_assign);
+ }Apply the same pattern to the corresponding loops in the CHARACTER SET and CHARSET branches.
Also applies to: 49-53, 61-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/sql_parser/set_parser.h` around lines 32 - 36, The loops that consume
TK_COMMA call parse_variable_assignment(nullptr) and ignore a null result, which
lets malformed trailing assignments pass; update each loop (the one shown and
the similar loops in the CHARACTER SET and CHARSET branches) to treat a null
next_assign as a parse failure by returning nullptr (or otherwise failing the
parse) instead of continuing — locate the loops that use
tok_.peek()/tok_.skip(), call parse_variable_assignment(nullptr) and
root->add_child(next_assign) and add an if (!next_assign) return nullptr;
immediately after the parse_variable_assignment call in each of those loops.
…nment loop The comma-separated assignment loops only called parse_variable_assignment(), so SET ... , NAMES utf8 would be parsed as SET ... , (var NAMES = utf8) instead of recognizing NAMES as a special SET keyword. Added parse_comma_item() helper that checks for NAMES, CHARSET, and CHARACTER SET keywords before falling through to variable assignment. All comma loops now use this helper. All 1213 tests pass.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Makefile (1)
112-117: 💤 Low value
PG_CFLAGSis not cleared whenPG_AVAILABLE=no, unlikePG_LIBS.
PG_CFLAGShas a hardcoded fallback (-I/usr/include/postgresqlat line 9) that is passed to every compilation rule whenpg_configis absent.MYSQL_CFLAGSdoesn't share this problem because it has no fallback and becomes empty naturally. For symmetry and to avoid passing a potentially non-existent include path on every compile invocation, add a corresponding clear:🔧 Proposed fix
ifeq ($(PG_AVAILABLE),no) PG_LIBS := +PG_CFLAGS := endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 112 - 117, When PG_AVAILABLE is set to "no" the Makefile clears PG_LIBS but not PG_CFLAGS, so the hardcoded fallback (-I/usr/include/postgresql) still gets passed; update the conditional that checks PG_AVAILABLE (the same block that currently sets "PG_LIBS :=") to also set "PG_CFLAGS :=" when PG_AVAILABLE=no so the PostgreSQL include flags are not injected into compilation when pg_config is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 33-40: Some sources still unconditionally include DB client
headers causing builds to fail when MYSQL_AVAILABLE/PG_AVAILABLE are unset; wrap
the DB-specific includes in conditional compilation and clear matching CFLAGS in
the Makefile: protect any inclusion of <mysql/mysql.h> and <libpq-fe.h> (e.g.,
in thread_safe_executor.h and multi_remote_executor.h and any tools or tests
that include those headers like tools/sqlengine.cpp, tools/mysql_server.cpp,
tools/bench_distributed.cpp, tools/engine_stress_test.cpp,
tests/test_distributed_real.cpp) with `#ifdef` MYSQL_AVAILABLE / `#endif` or `#ifdef`
PG_AVAILABLE guards (or refactor to forward-declare/use pimpl so non-DB clients
don’t see DB headers), and update the Makefile so when *_LIBS are emptied for
missing DBs the corresponding *_CFLAGS (include paths) are also cleared to keep
compilation consistent.
---
Nitpick comments:
In `@Makefile`:
- Around line 112-117: When PG_AVAILABLE is set to "no" the Makefile clears
PG_LIBS but not PG_CFLAGS, so the hardcoded fallback (-I/usr/include/postgresql)
still gets passed; update the conditional that checks PG_AVAILABLE (the same
block that currently sets "PG_LIBS :=") to also set "PG_CFLAGS :=" when
PG_AVAILABLE=no so the PostgreSQL include flags are not injected into
compilation when pg_config is absent.
🪄 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: b2a291dd-8f71-4764-85dc-b3fac1068076
📒 Files selected for processing (2)
Makefileinclude/sql_parser/set_parser.h
🚧 Files skipped from review as they are similar to previous changes (1)
- include/sql_parser/set_parser.h
| # Exclude MySQL-dependent engine sources when MySQL headers are not available | ||
| ifeq ($(MYSQL_AVAILABLE),no) | ||
| ENGINE_SRCS := $(filter-out $(ENGINE_SRC_DIR)/mysql_remote_executor.cpp $(ENGINE_SRC_DIR)/multi_remote_executor.cpp, $(ENGINE_SRCS)) | ||
| endif | ||
| # Exclude PostgreSQL-dependent engine sources when PG headers are not available | ||
| ifeq ($(PG_AVAILABLE),no) | ||
| ENGINE_SRCS := $(filter-out $(ENGINE_SRC_DIR)/pgsql_remote_executor.cpp $(ENGINE_SRC_DIR)/multi_remote_executor.cpp, $(ENGINE_SRCS)) | ||
| endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Identify non-excluded sources that include thread_safe_executor.h or multi_remote_executor.h
# (which unconditionally pull in mysql/mysql.h and pgsql headers)
echo "=== Files including thread_safe_executor.h ==="
rg -rn --type=cpp '#include.*thread_safe_executor' \
--glob '!src/sql_engine/mysql_remote_executor.cpp' \
--glob '!src/sql_engine/pgsql_remote_executor.cpp' \
--glob '!src/sql_engine/multi_remote_executor.cpp'
echo ""
echo "=== Files including multi_remote_executor.h ==="
rg -rn --type=cpp '#include.*multi_remote_executor' \
--glob '!src/sql_engine/mysql_remote_executor.cpp' \
--glob '!src/sql_engine/pgsql_remote_executor.cpp' \
--glob '!src/sql_engine/multi_remote_executor.cpp'
echo ""
echo "=== Test files still included when MySQL/PG unavailable that include engine headers ==="
rg -rn --type=cpp '#include.*sql_engine' tests/test_distributed_planner.cpp tests/test_dml.cpp tests/test_distributed_dml.cpp tests/test_local_txn.cpp tests/test_session.cpp tests/test_single_backend_txn.cpp tests/test_distributed_txn.cpp 2>/dev/nullRepository: ProxySQL/ParserSQL
Length of output: 2932
🏁 Script executed:
# Check the actual content of the headers mentioned
echo "=== Checking thread_safe_executor.h for mysql includes ==="
fd thread_safe_executor.h --type f | head -5 | xargs -I {} head -40 {}
echo ""
echo "=== Checking multi_remote_executor.h includes ==="
fd multi_remote_executor.h --type f | head -5 | xargs -I {} head -50 {}
echo ""
echo "=== Checking Makefile for PG_CFLAGS/PG_LIBS handling ==="
sed -n '103,120p' MakefileRepository: ProxySQL/ParserSQL
Length of output: 3482
🏁 Script executed:
# Get fresh include search with proper patterns
echo "=== Non-excluded .cpp files including thread_safe_executor.h ==="
rg 'include.*thread_safe_executor\.h' --type cpp -l
echo ""
echo "=== Non-excluded .cpp files including multi_remote_executor.h ==="
rg 'include.*multi_remote_executor\.h' --type cpp -l
echo ""
echo "=== Check for unconditional mysql/mysql.h includes ==="
rg 'include.*<mysql/mysql\.h>' --type cpp -l | head -20Repository: ProxySQL/ParserSQL
Length of output: 707
Fix unconditional header includes to prevent build failures when MySQL/PostgreSQL client headers are unavailable.
The Makefile exclusions of .cpp files are insufficient because multiple non-excluded files unconditionally include headers that pull in MySQL/PostgreSQL-specific code:
include/sql_engine/thread_safe_executor.h(line 25) unconditionally#include <mysql/mysql.h>with no conditional guardinclude/sql_engine/multi_remote_executor.hunconditionally includesmysql_remote_executor.handpgsql_remote_executor.h- Non-excluded tool sources (
tools/sqlengine.cpp,tools/mysql_server.cpp,tools/bench_distributed.cpp,tools/engine_stress_test.cpp) include both headers tests/test_distributed_real.cppincludesmulti_remote_executor.hand directly includes<mysql/mysql.h>
When MySQL or PostgreSQL headers are absent, these non-excluded files will fail to compile regardless of the .cpp exclusions. The fix requires wrapping the unconditional includes in #ifdef MYSQL_AVAILABLE / #ifdef PG_AVAILABLE guards, or reorganizing headers to avoid exposing DB-specific dependencies to consumers that don't directly use them.
Additionally, note that when database libraries are unavailable, only *_LIBS variables are cleared in the Makefile (lines 113–116), but the corresponding *_CFLAGS include paths are not—both should be cleared consistently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 33 - 40, Some sources still unconditionally include DB
client headers causing builds to fail when MYSQL_AVAILABLE/PG_AVAILABLE are
unset; wrap the DB-specific includes in conditional compilation and clear
matching CFLAGS in the Makefile: protect any inclusion of <mysql/mysql.h> and
<libpq-fe.h> (e.g., in thread_safe_executor.h and multi_remote_executor.h and
any tools or tests that include those headers like tools/sqlengine.cpp,
tools/mysql_server.cpp, tools/bench_distributed.cpp,
tools/engine_stress_test.cpp, tests/test_distributed_real.cpp) with `#ifdef`
MYSQL_AVAILABLE / `#endif` or `#ifdef` PG_AVAILABLE guards (or refactor to
forward-declare/use pimpl so non-DB clients don’t see DB headers), and update
the Makefile so when *_LIBS are emptied for missing DBs the corresponding
*_CFLAGS (include paths) are also cleared to keep compilation consistent.
The macOS GitHub Actions runner lacks MySQL and PostgreSQL client libraries, causing compilation failures in sql_engine test sources that include <mysql/mysql.h> and <libpq-fe.h>. Install both via Homebrew before building.
renecannao
force-pushed
the
fix/set-parser-issues-34-35-36
branch
from
e68ddbd to
e7c24a2
Compare
…e path mysql_config --cflags returns -I.../include/mysql, but source uses #include <mysql/mysql.h>, creating mysql/mysql/mysql.h on macOS. Pass the parent directory on the make command line instead.
mysql_config --libs on macOS includes -lzstd as a transitive dependency. Install zstd via brew and set LIBRARY_PATH so the linker can find it in /opt/homebrew/lib.
Add guard in parse_comma_item() to reject comma/semicolon/RPAREN/EOF tokens before falling through to parse_variable_assignment(). Without this, a double comma like SET a=1,,b=2 would create a NODE_IDENTIFIER with "," as the variable name, producing a corrupt AST. Separate the InvalidSyntaxReturnsPartial test (PARTIAL cases only) from the new LenientAcceptsUnusualSyntax test (OK cases including mixed SET forms, double comma, and DEFAULT charset name). CodeRabbit review: partially adopted strict comma handling suggestion. Full strict mode breaks PostgreSQL SET search_path TO value1, value2 because the comma is part of the value, not a statement separator. Kept lenient handling but with the corrupt-AST guard.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/sql_parser/set_parser.h (1)
91-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast when the first assignment parse fails.
After
parse_variable_assignment(...)fails, continuing into comma parsing can accept statements like malformed-first/valid-second assignments as success. Treat the first assignment failure as parse failure in these branches.Proposed fix
AstNode* assignment = parse_variable_assignment(&scope_tok); - if (assignment) root->add_child(assignment); + if (!assignment) return nullptr; + root->add_child(assignment); ... AstNode* assignment = parse_variable_assignment(nullptr); - if (assignment) root->add_child(assignment); + if (!assignment) return nullptr; + root->add_child(assignment);Also applies to: 115-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/sql_parser/set_parser.h` around lines 91 - 99, The code currently continues to parse comma-separated items even if the initial parse_variable_assignment(&scope_tok) returned nullptr, allowing malformed-first/valid-second cases to be accepted; update the logic in the block handling assignments so that if assignment == nullptr you immediately return nullptr before entering the comma loop (i.e., check the result of parse_variable_assignment and fail fast), and make the same change in the analogous block that uses parse_comma_item at the other occurrence (lines around the parse_variable_assignment / parse_comma_item pair referenced). Ensure you still call root->add_child only for non-null nodes and do not proceed to tok_.peek()/tok_.skip() comma handling when the first assignment failed.
♻️ Duplicate comments (1)
include/sql_parser/set_parser.h (1)
32-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not ignore failed comma items after consuming
,.Once a comma is consumed, a missing/invalid item should fail the statement. Right now,
nullptrfromparse_comma_item()is ignored, so malformed tails can still return success if an earlier child exists.Proposed fix
while (tok_.peek().type == TokenType::TK_COMMA) { tok_.skip(); AstNode* next_assign = parse_comma_item(); - if (next_assign) root->add_child(next_assign); + if (!next_assign) return nullptr; + root->add_child(next_assign); }Also applies to: 49-53, 61-65, 94-98, 117-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/sql_parser/set_parser.h` around lines 32 - 36, After consuming a comma (tok_.skip()) you must not ignore a nullptr from parse_comma_item(); instead treat it as a parse error and fail the whole construct: call parse_comma_item(), and if it returns nullptr, free any partially-built node (e.g., delete root) and return nullptr rather than continuing; apply this change to the other identical loops that use tok_.skip() + parse_comma_item() (the blocks referencing parse_comma_item(), tok_.skip(), and root->add_child()) so malformed tails cause the parse to fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 29-33: Replace the hardcoded /opt/homebrew paths in the "Install
dependencies" step of .github/workflows/ci.yml (the two echo lines that add
Homebrew binaries to GITHUB_PATH and the other occurrences noted in the comment)
with dynamic prefixes obtained from brew --prefix so the workflow works on both
Apple Silicon and Intel macOS; update the echo statements to construct the
mysql-client and zstd bin paths using brew --prefix (and any other Homebrew
package paths) instead of embedding /opt/homebrew directly.
---
Outside diff comments:
In `@include/sql_parser/set_parser.h`:
- Around line 91-99: The code currently continues to parse comma-separated items
even if the initial parse_variable_assignment(&scope_tok) returned nullptr,
allowing malformed-first/valid-second cases to be accepted; update the logic in
the block handling assignments so that if assignment == nullptr you immediately
return nullptr before entering the comma loop (i.e., check the result of
parse_variable_assignment and fail fast), and make the same change in the
analogous block that uses parse_comma_item at the other occurrence (lines around
the parse_variable_assignment / parse_comma_item pair referenced). Ensure you
still call root->add_child only for non-null nodes and do not proceed to
tok_.peek()/tok_.skip() comma handling when the first assignment failed.
---
Duplicate comments:
In `@include/sql_parser/set_parser.h`:
- Around line 32-36: After consuming a comma (tok_.skip()) you must not ignore a
nullptr from parse_comma_item(); instead treat it as a parse error and fail the
whole construct: call parse_comma_item(), and if it returns nullptr, free any
partially-built node (e.g., delete root) and return nullptr rather than
continuing; apply this change to the other identical loops that use tok_.skip()
+ parse_comma_item() (the blocks referencing parse_comma_item(), tok_.skip(),
and root->add_child()) so malformed tails cause the parse to fail.
🪄 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: d7559789-453d-4567-a367-33c09755411d
📒 Files selected for processing (3)
.github/workflows/ci.ymlinclude/sql_parser/set_parser.htests/test_set.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_set.cpp
| - name: Install dependencies | ||
| run: | | ||
| brew install mysql-client postgresql zstd | ||
| echo "/opt/homebrew/opt/mysql-client/bin" >> $GITHUB_PATH | ||
| echo "/opt/homebrew/opt/zstd/bin" >> $GITHUB_PATH |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "brew prefix: $(brew --prefix)"
echo "mysql-client prefix: $(brew --prefix mysql-client)"
echo "zstd prefix: $(brew --prefix zstd)"Repository: ProxySQL/ParserSQL
Length of output: 259
🏁 Script executed:
cat -n .github/workflows/ci.yml | head -50Repository: ProxySQL/ParserSQL
Length of output: 1696
🏁 Script executed:
rg "brew.*--prefix" --type shell --type yaml --type ymlRepository: ProxySQL/ParserSQL
Length of output: 92
🏁 Script executed:
rg "brew.*--prefix" --multilineRepository: ProxySQL/ParserSQL
Length of output: 44
🏁 Script executed:
rg "/opt/homebrew" --type yamlRepository: ProxySQL/ParserSQL
Length of output: 44
🏁 Script executed:
rg "/opt/homebrew"Repository: ProxySQL/ParserSQL
Length of output: 44
🏁 Script executed:
grep -n "homebrew" .github/workflows/ci.ymlRepository: ProxySQL/ParserSQL
Length of output: 362
🌐 Web query:
Homebrew brew --prefix best practice macOS CI
💡 Result:
In macOS CI environments like GitHub Actions, the best practice for handling Homebrew's brew --prefix is to use official Homebrew GitHub Actions for setup and caching, ensuring the standard prefix (/opt/homebrew on Apple Silicon, /usr/local on Intel) is properly configured and available in PATH. Use Homebrew/actions/setup-homebrew@main to set up Homebrew environment, which sources from standard locations and outputs paths including the prefix. For efficient package installation and caching across runs, use Homebrew/actions/cache-homebrew-prefix@main with with: install: 'pkg1 pkg2'. This caches the entire prefix (
Citations:
- 1: https://github.com/Homebrew/brew/blob/cbc2b248/docs/Installation.md
- 2: Use Homebrew/actions/cache-homebrew-prefix@main in workflows Homebrew/brew#21565
- 3: https://docs.brew.sh/Installation.html
- 4: https://github.com/Homebrew/actions/tree/main/setup-homebrew
- 5: Add cache-homebrew-prefix action Homebrew/actions#786
- 6: https://github.com/Homebrew/homebrew-cask/blob/main/.github/workflows/ci.yml
Use dynamic Homebrew prefixes instead of hardcoding /opt/homebrew.
Lines 32–33, line 37, and line 38 hardcode Homebrew paths to /opt/homebrew, which only works on Apple Silicon runners. If the runner image or architecture changes to Intel macOS (which uses /usr/local), dependency discovery and builds will fail.
Replace hardcoded paths with brew --prefix:
Proposed fix
- name: Install dependencies
run: |
brew install mysql-client postgresql zstd
- echo "/opt/homebrew/opt/mysql-client/bin" >> $GITHUB_PATH
- echo "/opt/homebrew/opt/zstd/bin" >> $GITHUB_PATH
+ echo "$(brew --prefix mysql-client)/bin" >> $GITHUB_PATH
+ echo "$(brew --prefix zstd)/bin" >> $GITHUB_PATH
- name: Build and test
- env:
- LIBRARY_PATH: "/opt/homebrew/lib"
- run: make -f Makefile clean && make -f Makefile all MYSQL_CFLAGS="-I/opt/homebrew/opt/mysql-client/include"
+ run: |
+ BREW_PREFIX="$(brew --prefix)"
+ MYSQL_PREFIX="$(brew --prefix mysql-client)"
+ LIBRARY_PATH="${BREW_PREFIX}/lib" \
+ make -f Makefile clean && \
+ make -f Makefile all MYSQL_CFLAGS="-I${MYSQL_PREFIX}/include"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 29 - 33, Replace the hardcoded
/opt/homebrew paths in the "Install dependencies" step of
.github/workflows/ci.yml (the two echo lines that add Homebrew binaries to
GITHUB_PATH and the other occurrences noted in the comment) with dynamic
prefixes obtained from brew --prefix so the workflow works on both Apple Silicon
and Intel macOS; update the echo statements to construct the mysql-client and
zstd bin paths using brew --prefix (and any other Homebrew package paths)
instead of embedding /opt/homebrew directly.
1 participant
Summary
Fixes #34, #35, #36.
Issue #34: SET NAMES + comma-separated assignments
SET NAMES utf8, sql_mode='TRADITIONAL'now correctly parses the comma-separated variable assignments after theNAMESclause. Same fix applied toSET CHARACTER SETandSET CHARSETbranches.Root cause:
set_parser.hreturned immediately after parsing the NAMES/CHARSET node without a comma loop.Issue #35: ON keyword not accepted as SET value
SET sql_auto_is_null = ONnow works.ONis tokenized asTK_ON(a keyword), which the expression parser rejected because it wasn't inis_keyword_as_identifier().Root cause:
expression_parser.his_keyword_as_identifier()allowlist was missingTK_ON.Issue #36: Invalid syntax returned OK instead of PARTIAL
SET(with nothing after it) now returnsParseResult::PARTIALinstead ofOKwith an empty AST. Added empty-AST guards to allSetParser::parse()branches and a defense-in-depth check inparse_set().Root cause:
NODE_SET_STMTroot was allocated before any parsing and always returned.parse_set()treated any non-null AST as OK.Test results
All 1213 tests pass (up from 1212 — added
InvalidSyntaxReturnsPartialtest).Files changed
include/sql_parser/set_parser.hinclude/sql_parser/expression_parser.hTK_ONtois_keyword_as_identifier()src/sql_parser/parser.cppast->first_childbefore reporting OKtests/test_set.cppInvalidSyntaxReturnsPartialtestSummary by CodeRabbit