feat: replace tuple_every with tuple_reduce#5878
Conversation
prql-bot
left a comment
There was a problem hiding this comment.
A few observations on the tuple_reduce resolver implementation:
-
Sentinel-string collision. Using
"__missing"as the default value forinitialand matching against its quoted form means a user who passestuple_reduce initial:"__missing" some_fn listsilently hits the "no initial provided" path instead of seeing their string treated as the initial value. The risk is low in practice, but the design is a bit fragile — happy to defer to maintainer preference, but it would be worth at least a code comment explaining the convention. Other places instd.prqluse thenoresolve.*ident namespace for non-literal defaults (e.g.noresolve.side:inner); that pattern would be unambiguous here too. -
Backward compatibility.
tuple_everyis removed entirely. The PR description correctly identifies that no other in-tree code uses it, and there are no docs references either, but it was a public std-lib name. The PR description's own suggestion — keeping a one-line shimlet tuple_every = func list -> <bool> tuple_reduce initial:true std.and list— is cheap insurance for any external user. Worth a maintainer call. -
Test coverage. The new tests exercise
tuple_reducewithadd, which is great forinitial:semantics, but the originaltuple_everyuse case (boolean reduce viastd.and) is only covered transitively via theset_ops_removesnapshot. A small explicit test (e.g.tuple_reduce std.and {true, false, true}) would lock that path in.
Inline suggestion for the literal comparison below.
|
To comment on @prql-bot's observations:
will work as you would expect. I think that future support within PRQL for Symbol-type values (that aren't strings or anything else) makes sense, but is out of scope for this PR.
|
|
Thanks for the responses. On point 1: the ident-as-initial use case is a good reason to keep a string sentinel — agree that Points 2 and 3 are maintainer judgment calls — happy to defer. |
|
agree @kgutwin ! |
As I was looking at the
tuple_*functions in the std lib, I noticed thattuple_everyis a fairly straightforward function which essentially applies an "AND" reduce operation across its single tuple-value parameter. This PR replaces that single-purpose function withtuple_reduce, capable of performing a reduce across a tuple using any operator or two-parameter function.It behaves very similar to the Python
functools.reduce()function, in that it takes a function, tuple, and an optionalinitialnamed parameter. When theinitialparameter is provided, the first step of the reduction uses the initial value as the first parameter to the reduction operation. If the provided tuple is empty, the initial value will be returned. When theinitialparameter is omitted, if the tuple is empty, an error is raised. If the tuple has one entry, its value will be returned; otherwise, the reduce operation will proceed as normal.Example usage:
produces:
If we want to preserve
tuple_everyfor backwards compatibility, it could be defined in the std lib as:However, I wasn't sure that this was necessary, since apparently only the other std lib functions
intersectandremoveusetuple_every, and those were easily ported over to usetuple_reduce.