Skip to content

Conversation

@yuval-illumex
Copy link
Contributor

Add support to the function position as described in the docs

Example:

SELECT POSITION('@' in field)
FROM table

@coveralls
Copy link

coveralls commented Apr 17, 2022

Pull Request Test Coverage Report for Build 2215898239

  • 19 of 20 (95.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 90.38%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/parser.rs 10 11 90.91%
Totals Coverage Status
Change from base Build 2215333087: 0.01%
Covered Lines: 8023
Relevant Lines: 8877

💛 - Coveralls

@yuval-illumex yuval-illumex changed the title Add Support in the "Position Function" Add Support in the "Position" Function Apr 17, 2022
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @yuval-illumex -- Does POSITION('@' in 'foo') parse in sqlparser without changes today? Or is this PR required to parse the expression?

@yuval-illumex
Copy link
Contributor Author

@alamb Updated the PR, changed the POSITION function to be like any other function (But the parsed response must use a struct for POSITION)

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delayed review @yuval-illumex -- I have been quite busy recently.

I didn't appreciate that <expr> IN <expr> was only valid within the context of a POSITION() function. Sorry for my confusion

Can you please change this code so that the POSITION function is handled specially (I realize you did do that initially and I implied you should change)?

Thanks and sorry again.

@yuval-illumex
Copy link
Contributor Author

@alamb Thanks for the detailed feedback! Missed the bug with IN.
Appreciate new feedback after the fix

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me -- thank you @yuval-illumex ❤️

@alamb alamb merged commit 8f4f01e into apache:main Apr 25, 2022
@alamb
Copy link
Contributor

alamb commented Apr 25, 2022

I wrote up a small negative case here as well: #469

graham pushed a commit to graham/sqlparser-rs that referenced this pull request Apr 26, 2022
* Add support in the position function

* Add Test

* Add lint fixes

* Remove if

* Change from to in

* Remove special method for position

* Fix lint

* PR Review
@alamb alamb mentioned this pull request May 9, 2022
MazterQyou pushed a commit to cube-js/sqlparser-rs that referenced this pull request Oct 24, 2022
* Add support in the position function

* Add Test

* Add lint fixes

* Remove if

* Change from to in

* Remove special method for position

* Fix lint

* PR Review
MazterQyou pushed a commit to cube-js/sqlparser-rs that referenced this pull request Oct 25, 2022
* Add support in the position function

* Add Test

* Add lint fixes

* Remove if

* Change from to in

* Remove special method for position

* Fix lint

* PR Review
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.

3 participants