Re: SQL:2023 JSON simplified accessor support

Hi Alex,

Thanks for addressing my comments. I have a few follow up comments:

> 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?

--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/