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: avoid using regex match, possibly containing alternation, as a key condition. #54696
Conversation
This is an automated comment for commit f52e87f with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page Successful checks
|
@@ -421,7 +421,12 @@ const KeyCondition::AtomMap KeyCondition::atom_map | |||
if (value.getType() != Field::Types::String) | |||
return false; | |||
|
|||
String prefix = extractFixedPrefixFromRegularExpression(value.get<const String &>()); | |||
const String & expression = value.get<const String &>(); | |||
// This optimization can't process alternation - this would require a comprehensive parsing of regular expression. |
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.
I thought a bit
- whether the check should rather be moved into
extractFixedPrefixFromRegularExpression()
but I think it is okay to not do so - whether
extractFixedPrefixFromRegularExpression()
could be replaced by the more sophisticated regex parsing inOptimizedRegularExpression::analyze()
, but again no.::analyze()
searches the longest substring and that's not necessarily the prefix.
Therefore, we can merge the fix. Thanks.
Strange that it's not backported into LTS releases but the issue was marked as major |
True. Let's backport it. |
…c2cf48099eff68c494067ff5bcbc0 Cherry pick #54696 to 23.3: Fix: avoid using regex match, possibly containing alternation, as a key condition.
…ining alternation, as a key condition.
…c2cf48099eff68c494067ff5bcbc0 Cherry pick #54696 to 23.7: Fix: avoid using regex match, possibly containing alternation, as a key condition.
…ining alternation, as a key condition.
…c2cf48099eff68c494067ff5bcbc0 Cherry pick #54696 to 23.8: Fix: avoid using regex match, possibly containing alternation, as a key condition.
…ining alternation, as a key condition.
Backport #54696 to 23.8: Fix: avoid using regex match, possibly containing alternation, as a key condition.
Backport #54696 to 23.7: Fix: avoid using regex match, possibly containing alternation, as a key condition.
Backport #54696 to 23.3: Fix: avoid using regex match, possibly containing alternation, as a key condition.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fixed bug of match() function (regex) with pattern containing alternation produces incorrect key condition.
closes #53222