-
-
Notifications
You must be signed in to change notification settings - Fork 597
Always 0 for unexisting @@variables #3109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
the related ticket is #1273 daemon always returns valid response for unknown Then the buddy present the response looks like
then the buddy disable the response looks like
The supported variables are: However all other variables just returns 0 or In case we are going to change this behavior it worth to add new For it should have format: like
That need as we do not have list of clients these depends on this behavior. |
Another related issue is https://gitlab.com/manticoresearch/dev/-/issues/3009 and the commit as the result of it - 8240fce05e . It doesn't explain why not |
Probably because in MySQL it's not considered an error either. I suggest we do the following:
Let's implement these changes in a branch, build the daemon and Buddy packages, and then:
|
I've just pushed the change into #3117 There I keep the current behavior for daemon runs without buddy and while buddy running daemon should fail query with any unknown sysvar and route these into buddy. the new behavior
the old behavior
need to check all failed cases at the PR and implement fixes at the buddy. |
@donhardman pls test the updated version or prepare a clear test plan for @PavelShilin89 . |
Test Cases1. Basic System Variable Tests1.1 Test Existing System VariablesPurpose: Verify that valid system variables return correct values
1.2 Test Non-existent System VariablesPurpose: Verify error handling for non-existent variables
1.3 Test Partial MatchesPurpose: Verify behavior with variables that partially match valid ones
2. Query Context Tests2.1 Test in Multi-Select QueriesPurpose: Verify behavior when mixing valid and invalid variables
2.2 Test in Complex QueriesPurpose: Verify behavior in more complex query structures
3. Client Integration Tests3.1 Test with mysqldump/mariadb-dumpPurpose: Verify that backup tools work correctly with the new behavior
3.2 Test with MySQL-Related IntegrationsPurpose: Verify compatibility with common tools
4. User Variables Tests4.1 Test User Variables vs System VariablesPurpose: Verify that user variables (@var) are handled differently from system variables (@@var)
5. Edge Cases5.1 Test Variable Name Edge CasesPurpose: Verify behavior with unusual variable names
5.2 Test Case SensitivityPurpose: Verify case sensitivity handling
Expected Results Summary
|
Testing is done in this PR - #3361. Only the points and conditions that do not meet expectations are highlighted: Point 1.1: Point 2.2: Point 3.1: Point 4.1: Point 5.1: |
@tomatolog @donhardman A list of all problematic requests:
Expectation: 0 2.
Expectation: readable information, e.g. 3. Buddy: parser error instead of unknown sysvar
Expectation: ERROR 1064 (42000): unknown sysvar @@nonexistent_variable
4. Buddy: синтаксическая ошибка вместо unknown sysvar в WHERE
Expectation: ERROR 1064 (42000): unknown sysvar @@nonexistent_variable
5. Buddy: mysqldump aborts due to unknown variable
Expectation: successful dump
6. Error when setting a user variable
Expectation:
Fact:
7. Without Buddy: syntax error for @@@123
Expectation:
Fact:
8. Without Buddy: syntax error for @@'quoted_name'
Expectation:
Fact:
|
I've pushed the fixes of the points 1,7,8 into branch at #3117
for all other points I suggest to create another separate tickets as these are not related to the original ticket. for point 2
this seems generated at for points 3. 4I'd move all issues (point 3 and 4) related to
for points 5I'd move issue (point 5) related to
for points 6I'd move issue (point 6) related to set
|
@tomatolog @donhardman I confirm the fixes mentioned in the comment. |
@donhardman Let's simplify the test case so it only covers what's been fixed in this issue, so this is not blocked because of the other issues of lower priority with no significant reason. |
@donhardman Testing done in this PR - #3361 |
@tomatolog pls merge "master" into #3117 and reassign back to @PavelShilin89 to finalize. |
I've rolled back the latest commit at the branch and merged master into https://github.com/manticoresoftware/manticoresearch/tree/sysvar_buddy but the CLT still failed at CLT / CLT (Buddy, test/clt-tests/buddy/) |
Thanks for the update! However, the internal checklist is not yet complete. A Manticore team member will close this issue once everything is checked off. 🛠️ |
Bug Description:
There is an issue that blocks a task to be done: manticoresoftware/manticoresearch-buddy#388
When we do a select of any variable that does not exist at all, we return 0, which looks incorrect.
Let's return an error that this variable does not exist, because setting 0 as int for everything is not a good approach. So when we return an error here, we will be able to process such cases with Buddy.
Manticore Search Version:
Latest dev
Operating System Version:
Ubuntu
Have you tried the latest development version?
Yes
Internal Checklist:
To be completed by the assignee. Check off tasks that have been completed or are not applicable.
The text was updated successfully, but these errors were encountered: