maybe we can use zmalloc_usable to get more memory to store more items

Fixed by 686e024

i mean zmalloc_usable may return a bigger memory than we want, we can use the extra memory to store items. but i am okay now since we will release RC1

I feel like the finalization logic is somewhat similar to ardelCommand; perhaps we could refactor it into a separate function.

ARSET and ARMSET also have the similar issue.

if ar->insert_idx is larger than ar_len, should set it to ar_len - 1?

Ok you mean something like:

DEL a
ARSET a 0 zero one two three four
ARSEEK a 100
ARLASTITEMS a 3

That would produce NULLs? This si indeed a degenerated case. And should instead be anchored to the top of the array. Thanks!

Wait, you mean to also change the cursor as a side effect?

No, I just think that the effective_count will be adjusted to the maximum of count. When the insert_idx is invalid, does it also need to be adjusted? For example, in the following case

DEL a
ARSET a 0 zero one two three four
ARSEEK a 10
ARLASTITEMS a 10

Because the value of "count" has been corrected, it will now return

127.0.0.1:6379> ARLASTITEMS a 10
1) (nil)
2) (nil)
3) (nil)
4) (nil)
5) (nil)

But I'm wondering, since insert_idx is valid, why wouldn't it return:

127.0.0.1:6379> ARLASTITEMS a 10
 1) "zero"
 2) "one"
 3) "two"
 4) "three"
 5) "four"
 6) (nil)
 7) (nil)
 8) (nil)
 9) (nil)
10) (nil)

First tre_add_tag_right error silently discarded

Medium Severity

In ADDTAGS_AFTER_UNION_RIGHT, status is assigned the result of the first tre_add_tag_right call but immediately overwritten by the second call. If the first call returns REG_ESPACE (memory allocation failure) while the second succeeds, the error is silently lost and compilation continues with a partially-modified AST. The outer while loop's status == REG_OK guard never sees the failure, potentially allowing a corrupt regex to be used for matching.

 

^{Reviewed by Cursor Bugbot for commit 74d1fcd. Configure here.}

Allocated saved_states array is never used

Low Severity

saved_states is allocated with xmalloc, initialized with -1 values, and freed at the end of tre_add_tags, but it is never read or used in any computation between allocation and free. The allocation is proportional to tnfa->num_submatches + 1 and occurs on every regex compilation that requires tag setup, making it a needless overhead.

 

^{Reviewed by Cursor Bugbot for commit 74d1fcd. Configure here.}

fixed in the upstream.

A very sparse array can cause the debug digest to get stuck.

ARSET key 1000000000 x
debug digest

opinion (only): this "truncates floats toward zero" is not an obvious behaviour; if I request a bitwise operation, I expect it to operate on the exact bits I gave it, or error. Totally fine for the operation to only be defined on integers, if that is the intent; but since there is no ARINCRBY (unless I missed it), I don't expect any rounding errors, so this scenario seems really unexpected (and: if I was using ARINCRBY for floating-point incrs, I would have zero expectation for a bitwise operation to be meaningful... because I'm talking about FP).

Or rephrased: in what scenario is this behaviour expected and useful?

(it also gets weird in large values - for example a FP number above 2⁵³, where exact integers start to break down, but integer bitwise ops on the entire 64-bit range are normally anticipated to be meaningful)

oof; not a fan of these arbitrary "sometimes it is jagged, sometimes it is flat" decisions, especially when it doesn't even make use of any RESP3 intrinsics (map, etc)

I made this modification to keep it in sync with ZRANGE.
Perhaps from the client's perspective, it may not be very good.
@oranagra WDYT?

It probably impacts the "raw" clients more than the opinionated ones - it makes another pain point for people changing from RESP2 to RESP3. Consistency is not an unreasonable reason. I don't know what the "right" answer is here.

on the plus side: it reminded me that I should be using my own inbuilt helper types to make my own life easier... StackExchange/StackExchange.Redis@37cf357

I understand the discussion here is about using a completely different "layout" in RESP2 vs RESP3, and not the fact that it's a different layout when a WITHVALUES argument is passed (that's already well embedded in many other commands).

I understand that that's how it's in ZRANGE, and i don't recall why it was like that, but i assume we tried to "fix" (improve) WITHSCORES, and did that only for RESP3 to avoid breaking RESP2, which was probably a mistake.
and i think that in the case of the new command we can use a nested array in both RESP3 and RESP2.

@mgravell changed to use nested all the time when with WITHVALUES argument.
d740cce

noted, thanks

Xor is a weird one; is there a genuine use-case for this, or is it here "because we could"? In particular, in the context of sparse arrays, are nil values skipped? Or treated as zero? The latter is presumably inconsistent with how And behaves; the former means you need to know how many values were skipped to understand the result. But I question whether this even has a valid use case.

Xor is a weird one; is there a genuine use-case for this, or is it here "because we could"? In particular, in the context of sparse arrays, are nil values skipped? Or treated as zero?

nil values are skipped in all aggregation modes.