-
-
Notifications
You must be signed in to change notification settings - Fork 619
fix 'filter with empty name' error #3828
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
fix 'filter with empty name' error #3828
Conversation
|
@sanikolaev hello, can you check this please ? the failing test was again the error that has nothing to do with the changes submitted. Thank you ! |
|
Hello @ricardopintottrdata
are failing because:
As said in the Github docs :
You can add these secrets, and that should stop the failures. We can also skip the tests if the variables are not set (implemented in this PR #3835), but I'm not sure it's a good solution. I'll check with my colleagues. |
|
@glookka please review the PR |
|
@sanikolaev @glookka heyy! do you need anything else from me for this PR also ? |
No, thank you! @glookka has been quite busy with other things lately, but be sure we'll find time to review and merge your PR if everything looks good. |
|
ok thank you @sanikolaev ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this solution works, it implicitly assumes that the last element in the filter tree is the HAVING filter. I would at least add an assert to check that m_pQuery->m_dFilterTree.Last().m_iFilterItem matches the id of the filter being removed.
…tent is the expected
|
Hey @glookka thanks for the input, i've just updated the PR, think now is much better, thanks for the help! |

Type of Change (select one):
Description of the Change:
Fixes the error "filter with empty name"
Related Issue (provide the link):
#887
How to test:
Replacing or with and solves the problem:
Removing HAVING also solves the problem: