builtins: support third argument for array_position() built-in

[charlespnh]

Resolves: #109953

Release note (sql change): The array_position() built-in function with additional third argument was added.

[blathers-crl]

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

Before a member of our team reviews your PR, I have some potential action items for you:

• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[rafiss]

thanks for your contribution!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @charlespnh)

---

pkg/sql/sem/builtins/builtins.go line 3802 at r1 (raw file):

					start := int(tree.MustBeDInt(args[2])) - 1
					darray := tree.MustBeDArray(args[0]).Array
					if start > len(darray) || start < 1 {

i don't think the start < 1 condition matches the PostgreSQL behavior:

postgres=# SELECT array_position(ARRAY['sun','mon','tue','wed','thu','fri','sat','mon'], 'wed', 0);
 array_position
----------------
              4

postgres=# SELECT array_position(ARRAY['sun','mon','tue','wed','thu','fri','sat','mon'], 'wed', -1);
 array_position
----------------
              4

postgres=# SELECT array_position(ARRAY['sun','mon','tue','wed','thu','fri','sat','mon'], 'wed', -3);
 array_position
----------------
              4

postgres=# SELECT array_position(ARRAY['sun','mon','tue','wed','thu','fri','sat','mon'], 'wed', 1);
 array_position
----------------
              4

It looks like any start value less than 1 is implicitly treated as 1. Can you make that change (perhaps include a comment), and add tests for that?

---

pkg/sql/logictest/testdata/logic_test/array line 1140 at r1 (raw file):

SELECT array_position(ARRAY['sun','mon','tue','wed','thu','fri','sat','mon'], 'wed', 0)
----
NULL

in postgres, this returns 4

---

pkg/sql/logictest/testdata/logic_test/array line 1146 at r1 (raw file):

----
NULL

could you add these tests to verify that the second occurrence of an element is found correctly:

postgres=# SELECT array_position(ARRAY['sun','mon','tue','wed','thu','fri','sat','mon'], 'mon', 7);
 array_position
----------------
              8
(1 row)

postgres=# SELECT array_position(ARRAY['sun','mon','tue','wed','thu','fri','sat','mon'], 'mon', 1);
 array_position
----------------
              2

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[blathers-crl]

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[charlespnh]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

---

pkg/sql/sem/builtins/builtins.go line 3802 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i don't think the start < 1 condition matches the PostgreSQL behavior:

postgres=# SELECT array_position(ARRAY['sun','mon','tue','wed','thu','fri','sat','mon'], 'wed', 0);
 array_position
----------------
              4

postgres=# SELECT array_position(ARRAY['sun','mon','tue','wed','thu','fri','sat','mon'], 'wed', -1);
 array_position
----------------
              4

postgres=# SELECT array_position(ARRAY['sun','mon','tue','wed','thu','fri','sat','mon'], 'wed', -3);
 array_position
----------------
              4

postgres=# SELECT array_position(ARRAY['sun','mon','tue','wed','thu','fri','sat','mon'], 'wed', 1);
 array_position
----------------
              4

It looks like any start value less than 1 is implicitly treated as 1. Can you make that change (perhaps include a comment), and add tests for that?

Change is applied and comment has been added!

---

pkg/sql/logictest/testdata/logic_test/array line 1140 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

in postgres, this returns 4

Change is applied!

---

pkg/sql/logictest/testdata/logic_test/array line 1146 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

could you add these tests to verify that the second occurrence of an element is found correctly:

postgres=# SELECT array_position(ARRAY['sun','mon','tue','wed','thu','fri','sat','mon'], 'mon', 7);
 array_position
----------------
              8
(1 row)

postgres=# SELECT array_position(ARRAY['sun','mon','tue','wed','thu','fri','sat','mon'], 'mon', 1);
 array_position
----------------
              2

Tests added!

[charlespnh]

@rafiss Thanks for the feedback! Please let me know if there are any other meaningful tests I should have added

[rafiss]

i added one more test i just thought of, to check for null inputs, which required a change to null handling in the code. thanks for your contribution!

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)