Skip to content
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

Add REGEXP infix operator for MySQL #874

Closed
wants to merge 8 commits into from

Conversation

max-sixty
Copy link
Contributor

Would replace #868

But couple of issues:

  • I'm getting a test failure. What's the best test for this?
  • Should REGEXP be a keyword? If not, what would we parse?
  • Anything else I'm getting wrong?

@ankrgyl
Copy link
Contributor

ankrgyl commented May 10, 2023

I'm getting a test failure. What's the best test for this?

Looks like your code doesn't compile with no-std? https://github.com/sqlparser-rs/sqlparser-rs/actions/runs/4928867591/jobs/8807772910?pr=874

Should REGEXP be a keyword? If not, what would we parse?

Adding a keyword should work (they are not automatically reserved)

@max-sixty
Copy link
Contributor Author

I'm getting a test failure. What's the best test for this?

Looks like your code doesn't compile with no-std? https://github.com/sqlparser-rs/sqlparser-rs/actions/runs/4928867591/jobs/8807772910?pr=874

I think I can solve that one; but https://github.com/sqlparser-rs/sqlparser-rs/actions/runs/4928867591/jobs/8807773893?pr=874#step:5:504 needs some crate-specific guidance...

@ankrgyl
Copy link
Contributor

ankrgyl commented May 10, 2023

It looks like the new test you wrote is failing, and it reproduces locally with:

cargo test --test sqlparser_mysql

Can you be more specific about what kind of guidance you need?

@max-sixty
Copy link
Contributor Author

I'm fine reproducing the test. But I'm not sure what the best test for this is — is it verified_stmt? I don't see any tests on dialects' parse_infix (though there's a more general test of the functionality here).

Any thoughts why the test is failing? It looks like it's expecting the expression to finish after REGEXP...

@max-sixty
Copy link
Contributor Author

OK, one reason is that it never gets to parse_infix; this diff produces the same error:

diff --git a/src/dialect/mysql.rs b/src/dialect/mysql.rs
index 9e82702..62d2624 100644
--- a/src/dialect/mysql.rs
+++ b/src/dialect/mysql.rs
@@ -49,6 +49,7 @@ fn parse_infix(
         _expr: &crate::ast::Expr,
         _precedence: u8,
     ) -> Option<Result<crate::ast::Expr, crate::parser::ParserError>> {
+        panic!();
         // Parse REGEXP as an operator
         if _parser.parse_keyword(Keyword::REGEXP) {
             Some(Ok(Expr::BinaryOp {

...so this would benefit from some guidance from someone who knows this — thank you

@ankrgyl
Copy link
Contributor

ankrgyl commented May 10, 2023

Have you verified that the change works outside of the test?

@ankrgyl
Copy link
Contributor

ankrgyl commented May 10, 2023

OK, one reason is that it never gets to parse_infix; this diff produces the same error:

diff --git a/src/dialect/mysql.rs b/src/dialect/mysql.rs
index 9e82702..62d2624 100644
--- a/src/dialect/mysql.rs
+++ b/src/dialect/mysql.rs
@@ -49,6 +49,7 @@ fn parse_infix(
         _expr: &crate::ast::Expr,
         _precedence: u8,
     ) -> Option<Result<crate::ast::Expr, crate::parser::ParserError>> {
+        panic!();
         // Parse REGEXP as an operator
         if _parser.parse_keyword(Keyword::REGEXP) {
             Some(Ok(Expr::BinaryOp {

...so this would benefit from some guidance from someone who knows this — thank you

I'm not particularly familiar with this code, but it looks like you need to set a precedence in get_next_precedence.

@max-sixty
Copy link
Contributor Author

I'm not particularly familiar with this code, but it looks like you need to set a precedence in get_next_precedence.

Perfect, thanks!

This is working on my end now; let's see whether CI passes

@coveralls
Copy link

coveralls commented May 10, 2023

Pull Request Test Coverage Report for Build 4932692393

  • 20 of 20 (100.0%) changed or added relevant lines in 6 files are covered.
  • 315 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.04%) to 86.189%

Files with Coverage Reduction New Missed Lines %
src/ast/mod.rs 315 78.44%
Totals Coverage Status
Change from base Build 4860510463: 0.04%
Covered Lines: 14316
Relevant Lines: 16610

💛 - Coveralls

@alamb
Copy link
Contributor

alamb commented May 17, 2023

I feel like #868 is a better (simpler) approach -- given that do we still want to try and push this one forward?

@max-sixty
Copy link
Contributor Author

I feel like #868 is a better (simpler) approach -- given that do we still want to try and push this one forward?

I'm totally fine with #868! Tbh I was doing this to be a good citizen, as we're such heavy users of sqlparser-rs, rather than needing it per-se.

I could understand wanting either approach — you might want to actually parse the REGEXP for those dialects...

@alamb
Copy link
Contributor

alamb commented May 19, 2023

I'm totally fine with #868! Tbh I was doing this to be a good citizen, as we're such heavy users of sqlparser-rs, rather than needing it per-se.

Thank you for the kind works and the sense of civic responsibility ❤️

@alamb
Copy link
Contributor

alamb commented Jun 21, 2023

I believe #868 has superseded this PR

@alamb alamb closed this Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants