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

sql: allow strpos() builtin function to support bit and bytes array #46875

Merged

Conversation

abhishek20123g
Copy link
Contributor

Fixes #45849

This commit modified strpos builtin function to allow it to
support bit and byte array and add their respective testcases.

This PR creates separate overloads for both bit and bytes as:
strpos(varbit, varbit)
strpos(bytes, bytes)
since POSITION is an alias for strpos, at the parser level.
Therefore this PR affect POSITION too.

Release justification: low-risk change to existing functionality.

Release note (sql change): This PR modified strpos() function
to allow it support bit and byte array.

Fixes cockroachdb#45849

This commit modified strpos builtin function to allow it to
support bit and byte array and add their respective testcases.

This PR creates separate overloads for both bit and bytes as:
strpos(varbit, varbit)
strpos(bytes, bytes)
since POSITION is an alias for strpos, at the parser level.
Therefore this PR affect POSITION too.

Release justification: low-risk change to existing functionality.

Release note (sql change): This PR modified strpos() function
to allow it support bit and byte array.
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@abhishek20123g
Copy link
Contributor Author

small overview

  1. Above implementation can be done without writing functions bytesOverload2 and bitsOverload2 but written to maintain the overall uniformity of code and these function may be used in future.
  2. For bit array, this can be done without typecasting to string.
index := strings.Index(bitString.BitArray.String(),bitSubstring.BitArray.String())

for that, we have to implement a separate function like strings.Index inside
bitarray.BitArray which indeed end up the same implementation as
strings.Index.

@rohany rohany requested a review from otan April 1, 2020 20:26
Copy link
Contributor

@otan otan left a comment

Choose a reason for hiding this comment

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

LGTM!


leaving this here as a comment to myself, but if you want to nerd it out you can have a squiz!

it looks like postgres handles POSITION as a position C function (which is not available as a builtin)

https://github.com/postgres/postgres/blob/de3bbfcc962f24c1a20a17b76c604e5973a05817/src/backend/parser/gram.y#L13946

whereas we use strpos

$$.val = &tree.FuncExpr{Func: tree.WrapFunction("strpos"), Exprs: $3.exprs()}

one could argue strpos should only be strings, not bits and bytes. but doing a position builtin will make parsing angry as it will fight with the line mentioned above. so whatever /shrug

@otan
Copy link
Contributor

otan commented Apr 2, 2020

bors r+

@otan
Copy link
Contributor

otan commented Apr 3, 2020

bors r-

bors is stick due to github issue

@otan
Copy link
Contributor

otan commented Apr 3, 2020

bors r+

@abhishek20123g
Copy link
Contributor Author

TF[YT]R

Hi @otan,
Sorry for asking this here, if you have any issue in mind other than good first issue which I could take a shot with my current knowledge, please let me know,
I will be very happy to tackle it 😊.

@otan
Copy link
Contributor

otan commented Apr 7, 2020

looks like this never went in

bors r+

looks like we'll be out of builtins (for now) ...

#44616 and #41333 should be good.

@craig
Copy link
Contributor

craig bot commented Apr 7, 2020

Build failed

@otan
Copy link
Contributor

otan commented Apr 7, 2020 via email

@craig
Copy link
Contributor

craig bot commented Apr 7, 2020

Build succeeded

@craig craig bot merged commit 6365a3f into cockroachdb:master Apr 7, 2020
@abhishek20123g abhishek20123g deleted the sql-bit-support-for-strpos branch April 7, 2020 05:22
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.

sql: strpos not supported with bit types
3 participants