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.

Sign up for GitHub

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

Jump to bottom

Support key callback parameter for min(), max() starlark builtin functions #15022

Closed
tetromino opened this issue Mar 10, 2022 · 5 comments
Closed

Support key callback parameter for min(), max() starlark builtin functions #15022

tetromino opened this issue Mar 10, 2022 · 5 comments
Assignees
@tetromino
Labels
not stale Issues or PRs that are inactive but not considered stale P2 We'll consider working on this in future. (Assignee optional) team-Starlark-Interpreter Issues involving the Starlark interpreter used by Bazel type: feature request

Comments

@tetromino
Copy link
Contributor

See https:/bazelbuild/starlark/blob/master/spec.md#max
@tetromino tetromino added type: feature request P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Starlark-Interpreter Issues involving the Starlark interpreter used by Bazel labels Mar 10, 2022
@tetromino tetromino mentioned this issue Mar 10, 2022
Merged
@Tomsons Tomsons mentioned this issue Mar 25, 2022
Closed
@tetromino
Copy link
Contributor Author

tetromino commented Mar 25, 2022 •
edited
Loading

I light of #15122 - let me clarify how I see the feature needs to be implemented. Naturally, we do not want to call sorted (finding a min or max is an O(n) time and O(1) space problem; creating a temporary a sorted copy of the list is O(n log n) time and O(n) space). But we also do not want trivial comparator-based implementations that might evaluate key twice on each element of the list - the key function might be heavy, it might hypothetically have side effects, we want to call it only once on each element.

Thus ideally, I'd like to see an efficient, lightweight sort-key helper that would be shared between the implementations of sorted, min and max.

@brandjon brandjon added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) and removed P3 We're not considering working on this, but happy to review a PR. (No assignee) labels Oct 26, 2022
@brandjon brandjon mentioned this issue Oct 26, 2022
Closed
@github-actions GitHub Actions
Copy link

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Dec 31, 2023
@github-actions GitHub Actions
Copy link

This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please post @bazelbuild/triage in a comment here and we'll take a look. Thanks!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 31, 2024
@tetromino tetromino self-assigned this Apr 9, 2024
@tetromino tetromino reopened this Apr 9, 2024
@tetromino tetromino added P2 We'll consider working on this in future. (Assignee optional) and removed stale Issues or PRs that are stale (no activity for 30 days) P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) labels Apr 9, 2024
@brentleyjones brentleyjones added the not stale Issues or PRs that are inactive but not considered stale label Apr 9, 2024
@fmeum
Copy link
Collaborator

fmeum commented Apr 10, 2024

@bazel-io fork 7.2.0

@bazel-io bazel-io mentioned this issue Apr 10, 2024
Closed
bazel-io pushed a commit to bazel-io/bazel that referenced this issue Apr 10, 2024
@tetromino @bazel-io
Support key callback in Starlark min/max builtins
49746a3 
This is required by the language spec, but was not implemented in Bazel.

See https:/bazelbuild/starlark/blob/master/spec.md#max

Fixes bazelbuild#15022

Also take the opportunity to adjust sorted's signature for `key` to match.

RELNOTES: Starlark `min` and `max` buitins now allow a `key` callback,
similarly to `sorted`.
PiperOrigin-RevId: 623547043
Change-Id: I71d44aa715793f9f2260f9b20b876694154ff352
@bazel-io bazel-io mentioned this issue Apr 10, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Apr 10, 2024
@bazel-io @tetromino
[7.2.0] Support key callback in Starlark min/max builtins (#21960)
4729529 
This is required by the language spec, but was not implemented in Bazel.

See https:/bazelbuild/starlark/blob/master/spec.md#max

Fixes #15022

Also take the opportunity to adjust sorted's signature for `key` to
match.

RELNOTES: Starlark `min` and `max` buitins now allow a `key` callback,
similarly to `sorted`.
PiperOrigin-RevId: 623547043
Change-Id: I71d44aa715793f9f2260f9b20b876694154ff352

Commit
cf66672

Co-authored-by: Googler <[email protected]>
Kila2 pushed a commit to Kila2/bazel that referenced this issue May 13, 2024
@tetromino @Kila2
Support key callback in Starlark min/max builtins
824f89b 
This is required by the language spec, but was not implemented in Bazel.

See https:/bazelbuild/starlark/blob/master/spec.md#max

Fixes bazelbuild#15022

Also take the opportunity to adjust sorted's signature for `key` to match.

RELNOTES: Starlark `min` and `max` buitins now allow a `key` callback,
similarly to `sorted`.
PiperOrigin-RevId: 623547043
Change-Id: I71d44aa715793f9f2260f9b20b876694154ff352
@iancha1992
Copy link
Member

A fix for this issue has been included in Bazel 7.2.0 RC1. Please test out the release candidate and report any issues as soon as possible.
If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=7.2.0rc1. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tetromino tetromino

Labels
not stale Issues or PRs that are inactive but not considered stale P2 We'll consider working on this in future. (Assignee optional) team-Starlark-Interpreter Issues involving the Starlark interpreter used by Bazel type: feature request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@tetromino @brentleyjones @brandjon @fmeum @iancha1992