Re: SQL:2023 JSON simplified accessor support

Hi Chao,

Thank you so much for your review!

I’ve attached v16.

My current goal is to get consensus on 0001 - 0005 and move them toward
commitment. I’d also appreciate feedback on 0006 - 0007 as follow-up.

In v16, I addressed your feedback regarding the isSlice argument in
SubscriptTransform() -- I removed it. I’ve also addressed your three
other comments, sorry for missing them earlier!

I did not change anything about the jb[0] array accessor for jsonb
objects, will discuss that in a follow-up email.

On Tue, Sep 2, 2025 at 8:56 PM Chao Li <li.evan.chao@gmail.com> wrote:

> On Sep 3, 2025, at 10:16, Alexandra Wang <alexandra.wang.oss@gmail.com>
> wrote:
> I don't like the idea of removing the "bool isSlice" argument from the
> *SubscriptTransform() function pointer. This patch series should make
> only minimal changes to the subscripting API. While it's true that
> each implementation can easily determine the boolean value of isSlice
> itself, I don't see a clear benefit to changing the interface. As you
> noted, arrsubs cares about isSlice; and users can also define custom
> data types that support subscripting, so the interface is not limited
> to built-in types.
>
> As the comment for *SubscriptTransform() explains:
>
>> (Of course, if the transform method
>> * does not care to support slicing, it can just throw an error if
>> isSlice.)
>
>
> It's possible that in the end the "isSlice" argument isn't needed at
> all, but I don’t think this patch set is the right place for that
> refactor.
>
> I agree we should keep the change minimum and tightly related to the core
> feature.
>
> My original suggestion was to move
>
> /*
> * Detect whether any of the indirection items are slice specifiers.
> *
> * A list containing only simple subscripts refers to a single container
> * element. If any of the items are slice specifiers (lower:upper), then
> * the subscript expression means a container slice operation.
> */
> foreach(idx, *indirection)
> {
> Node *ai = lfirst(idx);
>
> if (IsA(ai, A_Indices) && castNode(A_Indices, ai)->is_slice)
> {
> isSlice = true;
> break;
> }
> }
>
> To out of transformContainerSubscripts(). Because the function was called
> only once under “if”, now this patch change it to be called under “while”,
> which brings two issues:
>
>  * Original it was O(n) as it was under “if”, now it is O(n2) as it is
> under “while”.
> * Logically it feels wrong now. Because this loops over the entire
> indirection list to check is_slice, while the subsequent
> sbsroutines->transform() may only process a portion (prefix) of indirection
> list. Say, the 5th element is slice, but the first sbsroutines-transform()
> call will only process the first 3 elements of indirection list, then pass
> true to the first transform() call sounds wrong.
>
> if we pull the loop out of transformContainerSubscripts(), then we have to
> add a new parameter “bool is_slice” to it. But after some researching, I
> found that making that change is more complicated than removing “is_slice”
> parameter from SubscriptTransform(). That’s why I ended up suggesting
> removing “is_slice” from SubscriptTransform().
>
> Does that sound reasonable?
>

Yes, that makes total sense! The logical defect you found can indeed
cause bugs in arraysubs. In 0002, I’ve added a test in arrays.sql to
demonstrate the potential bug and updated the code accordingly:

    This change also updates the SubscriptTransform() API to remove the
    "bool isSlice" argument. Each container’s transform function now
    determines slice-ness itself, since it may only process a prefix of
    the indirection list. For example, array_subscript_transform() stops
    at dot notation, so "isSlice" should not depend on slice specifiers
    that appear afterward.

Thanks,
Alex