Skip to content

Supporting instance count in ARRAY_POSITION#15170

Merged
rongrong merged 1 commit intoprestodb:masterfrom
ssaumitra:i-13861
Jun 22, 2021
Merged

Supporting instance count in ARRAY_POSITION#15170
rongrong merged 1 commit intoprestodb:masterfrom
ssaumitra:i-13861

Conversation

@ssaumitra
Copy link
Contributor

@ssaumitra ssaumitra commented Sep 15, 2020

Test plan - Added new tests in TestArrayOperators . Also ran the existing test suites.

== RELEASE NOTES ==

General Changes
* Add support for ``ARRAY_POSITION(array, element, instance)``

@ssaumitra
Copy link
Contributor Author

ssaumitra commented Sep 15, 2020

Fixes #13861

kaikalur
kaikalur reviewed Sep 16, 2020
View reviewed changes
...o-main/src/main/java/com/facebook/presto/operator/scalar/ArrayPositionWithIndexFunction.java Outdated
Copy link
Contributor

@kaikalur kaikalur Sep 16, 2020

Choose a reason for hiding this comment

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

Hmm - I think we can just return null. Consider this a short hand for an OR which will return null in that situation.

Copy link
Contributor

@rongrong rongrong Sep 16, 2020

Choose a reason for hiding this comment

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

Let's discuss changing function behavior separately from extending function APIs. What the function should do when array contains null is not in scope of this PR.

Copy link
Contributor Author

@ssaumitra ssaumitra Sep 18, 2020

Choose a reason for hiding this comment

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

Okay. Kept as-is for now.

Copy link
Contributor Author

@ssaumitra ssaumitra Jun 13, 2021

Choose a reason for hiding this comment

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

@rongrong / @kaikalur Are any further changes required on this?

I have just rebased this diff on the latest master. Build failures look transient maven failures, so force-pushing and re-triggering the build.

Copy link
Contributor Author

@ssaumitra ssaumitra Jun 18, 2021

Choose a reason for hiding this comment

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

@rongrong / @kaikalur Gentle reminder. Is any further change required on this?

kaikalur
kaikalur requested changes Sep 16, 2020
View reviewed changes
Copy link
Contributor

@kaikalur kaikalur left a comment

Choose a reason for hiding this comment

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

See inline

@ssaumitra ssaumitra requested a review from rongrong September 18, 2020 01:01
@stale
Copy link

stale bot commented Jun 11, 2021

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the task, make sure you've addressed reviewer comments, and rebase on the latest master. Thank you for your contributions!

@stale stale bot added stale and removed stale labels Jun 11, 2021
@rongrong
Copy link
Contributor

rongrong commented Jun 15, 2021

Not sure why the tests are still failing. Can you rebase and push again?

@ssaumitra
Copy link
Contributor Author

ssaumitra commented Jun 15, 2021 •
edited
Loading

Not sure why the tests are still failing. Can you rebase and push again?

@rongrong I tried that on the weekend. I just rebased and force pushed again. In the future, if a flaky test fails, is there a way to re-run only a selected test in the new setup?

@ssaumitra
Copy link
Contributor Author

ssaumitra commented Jun 16, 2021

@rongrong They are passing now 👍🏼

rongrong
rongrong approved these changes Jun 21, 2021
View reviewed changes
...o-main/src/main/java/com/facebook/presto/operator/scalar/ArrayPositionWithIndexFunction.java Outdated
Copy link
Contributor

@rongrong rongrong Jun 21, 2021

Choose a reason for hiding this comment

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

Maybe just change the error message to something like 0 is an invalid instance number for array_position or something more obviously stating that 0 is invalid. "cannot take" sounds like "not supported", but I think here we actually mean it's not valid.

Copy link
Contributor Author

@ssaumitra ssaumitra Jun 21, 2021

Choose a reason for hiding this comment

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

Okay. I have now edited the message, rebased and pushed.

@kaikalur kaikalur self-requested a review June 21, 2021 23:10
kaikalur
kaikalur approved these changes Jun 21, 2021
View reviewed changes
Copy link
Contributor

@kaikalur kaikalur left a comment

Choose a reason for hiding this comment

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

LGTM

@rongrong rongrong merged commit 8378094 into prestodb:master Jun 22, 2021
@ssaumitra ssaumitra deleted the i-13861 branch June 23, 2021 10:21
@ajaygeorge ajaygeorge mentioned this pull request Jul 7, 2021
Merged
1 task
@feilong-liu feilong-liu mentioned this pull request Sep 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rongrong rongrong rongrong approved these changes

@kaikalur kaikalur kaikalur approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ssaumitra @rongrong @kaikalur