MDEV-39520: Add MySQL 8.0 extended syntax for REGEXP_INSTR#5035
MDEV-39520: Add MySQL 8.0 extended syntax for REGEXP_INSTR#5035MohamedM216 wants to merge 1 commit intoMariaDB:mainfrom
REGEXP_INSTR#5035Conversation
MohamedM216
force-pushed
the
mysql8-extended-regexp
branch
2 times, most recently
from
d2ac796 to
ef7d499
Compare
gkodinov
added
the
External Contribution
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
First of all: this is implementing just one sub-task of what https://jira.mariadb.org/browse/MDEV-39106 calls for. Either do all of it, as scoped in the jira, in a single go. Or create Jira sub-task(s) into the above jira and target these instead.
There's also some suggestions below.
| void set_const(bool arg) { m_is_const= arg; } | ||
| CHARSET_INFO * library_charset() const { return m_library_charset; } | ||
| void unset_flag(int flag) { m_library_flags&= ~flag; } | ||
| void set_library_flags(int flags) { m_library_flags= flags; } |
There was a problem hiding this comment.
Why don't you do the parsing inside this function? And merge parse_match_type_flags into it? There's no call sequence in your pull request where the two are not called one after another.
There was a problem hiding this comment.
MohamedM216
changed the title
REGEXP_INSTRREGEXP_INSTR
MohamedM216
force-pushed
the
mysql8-extended-regexp
branch
2 times, most recently
from
d0e479a to
2634104
Compare
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Please fix the failing buildbot tests. Some improvement suggestions below too.
| CREATE TABLE foo (cond VARCHAR(50)); | ||
| INSERT INTO foo VALUES ('c'), ('ic'); | ||
| SELECT REGEXP_INSTR('Abba', 'ABBA', 1, 1, 0, cond) FROM foo; | ||
| DROP TABLE foo; No newline at end of file |
There was a problem hiding this comment.
please always terminate the last line in any file with LF. We also customarily end the .test files with --echo End of 13.0 tests
| re.init(cmp_collation.collation, 0); | ||
| if (re.parse_match_type_flags(match_type_str, cmp_collation)) | ||
| return (null_value= true, 0); | ||
| if ((null_value= re.compile(args[1], false))) |
There was a problem hiding this comment.
Not exactly sure why you need to compile this unconditionally after you've just called recompile above.
I'd suggest moving the whole thing before the re-compile call.
There was a problem hiding this comment.
recompile() cannot replace compile() here because m_is_const=true for constant patterns blocks it, making the direct compile() call in the non-const match_type block unavoidable. I suggest keeping it as it is. I'd appreciate your feedback.
|
|
||
|
|
||
| #define PCRE2_STATIC 1 /* Important on Windows */ | ||
| #include "pcre2.h" /* pcre2 header file */ |
There was a problem hiding this comment.
I guess this is what causes windows builds to fail. Remove this from here and move the new inlined function into the .cc file: it already includes the right headers.
Extend REGEXP_INSTR to accept the full MySQL 8.0 signature:
REGEXP_INSTR(subject, pattern
[, pos [, occurrence [, return_option
[, match_type]]]])
Previously only 2 arguments were accepted. Changes:
- Switch Create_func_regexp_instr from Create_func_arg2 to
Create_native_func to allow 2-6 arguments.
- Add parse_match_type_flags() to Regexp_processor_pcre, which parses
the match_type flags (c/i/m/n/u) and overwrites m_library_flags
with the fully-resolved PCRE2 flag word.
- Call parse_match_type_flags() in fix_length_and_dec() before fix_owner()
when match_type is constant, so the pattern is compiled with the
correct flags. For constant patterns fix_owner() sets m_is_const=true,
making recompile() a permanent no-op.
- For non-constant match_type, resolve flags per-row in val_int()
and call compile() directly to bypass the recompile() no-op guard.
- Add MTR test: regexp_instr_mysql8.test
MohamedM216
force-pushed
the
mysql8-extended-regexp
branch
from
2634104 to
1518554
Compare
|
The current failing tests aren't related to my patch and they pass locally. |
2 participants
Reference Issue
MDEV-39520
Description
This PR is one of a series of PRs that are supposed to add MySQL 8.0-compatible extended syntax for all REGEXP functions
Test
regexp_instr_mysql8.test