Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1utebp-00BxQx-VB for pgsql-hackers@arkaria.postgresql.org; Wed, 03 Sep 2025 03:56:35 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1utebo-006r3t-Se for pgsql-hackers@arkaria.postgresql.org; Wed, 03 Sep 2025 03:56:33 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1utebo-006r3f-Hp for pgsql-hackers@lists.postgresql.org; Wed, 03 Sep 2025 03:56:33 +0000 Received: from mail-pl1-x631.google.com ([2607:f8b0:4864:20::631]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1utebl-000HfS-0Q for pgsql-hackers@postgresql.org; Wed, 03 Sep 2025 03:56:32 +0000 Received: by mail-pl1-x631.google.com with SMTP id d9443c01a7336-2491f6b39a1so32527985ad.0 for ; Tue, 02 Sep 2025 20:56:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1756871788; x=1757476588; darn=postgresql.org; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:from:to:cc:subject:date:message-id:reply-to; bh=gq3c+eVIaXnc3V8ebIUw054O4aSXAE7kaXEwyy8cRpc=; b=f18mxHy2I1eiRInwC/9FsomXN2hqLrp3WQGLXOXJ/3yD/A3PFRsYKOEw3jNOO0YO4x 26Q9sd5otolqA7NYCFThwii1wv+1LjXXXEqVdZOopgGyqdTW8/oZ6if4sCCYxIhVLQx1 ODv+LzSHFKXrCbIuYpC7k6hsKIhc5lyZUFA0wAYel9djzYTcEquwf0VYc6L1R+Dr1e2l zC7nlugqVNwpNJ3KikPb3YTeiFLHsIg+/THyg0/OJflzLYPGME5KJ1EqikcJJKTn/XLO kWbfrIhPbheQ2Xg3lkgk4ZwM2OZWyMkqWdbf1SdCF5EGw1irP3W7ODFPTsVZ9OFfzZoO wPwA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1756871788; x=1757476588; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=gq3c+eVIaXnc3V8ebIUw054O4aSXAE7kaXEwyy8cRpc=; b=oxIEzgSsAEhRAnQKU9ZRKx3cN45PSkkToVO0tadBsS9CB2C+xD5B5voIyqJCdn01AE 5qahh2XdzaGo07mrqFVxFqet+ISMlbV7l3+gUjdMHUJvpTvKpYS+VxMVgfC+rg/t/0Dx +TNLDMfu9Yzc/SQdYhdkRL48GI0xwS6Aigo0vDvRnIYlSDbQNX15ndBej8ef6sEqZppp RKgVTVHVfcj6A1q/qQSkS1l/BVQDznXRRm2f9eKm/moRqYjXMnNbzywHs848UOV3p9kl BzPj1eI75ITAhJG6x2hxKvpBwPzgxJsnysjJ4Gpl3LS+Su56PYAwUlfCjcioO6N2k5em vpMQ== X-Forwarded-Encrypted: i=1; AJvYcCWXxTzAxmApRiRYuNmWHR5zeStE0fagWK1ufnpXINIGWdhApicacV5gSle1Xh1lZ4SIAniqLx6fXYMSKlqJ@postgresql.org X-Gm-Message-State: AOJu0YzizeJn8Ci5FjVi0cWt+Ws2cD5YfAgvFUky5FTRVf7gRBJb/qK/ BJXi0jQpmfqBbfsknl7tx21fgP6RCa43yUuYvEZVFFVeVg1Uocfk88vtyNdSXJ20jt5xAA== X-Gm-Gg: ASbGncvgpyupGD/2PY19O+ZIF6WjcABdwsxRSM0fLSN0Dm27KX9ZHVwIBNwsKdcX5wm BRzi4bRxFuuO5Bm18tn8X8893yT4cLaq0xM+5fQ/rB7aPth4Y8H9ngzBiciUpcRA20wh2eYZj6E Mtnmmx+MAaKxCc0IbRJIbBSeaxg0H4QjOVcxit5dWSVCO2tmlKpQtzZ0ntkaz9Bk7nZnnfPecPY N/32nbsAkGHEErcLIcWbpWNS4Ss/K9D6a3FzqPbpKqeLmvi8U4sELAAtT8VHCMTNk2VwkBN/1yg YxDtYJJIvEbkSrMG1sDm74s8ikP632hBXsN03Xn2rUESgc14u92BV3II7/fLE88xErpXwkDOSMo jAC1LdUocv4ay4e2uBaP/HZOlhqsGRTli X-Google-Smtp-Source: AGHT+IHWDOTshYEfZc/oLJ3IfKRpQEWcGqbcDuBY4MBFR8gfhslJDvyrRBd64NcmMbV/9rwne6ZW+A== X-Received: by 2002:a17:902:d4cb:b0:246:cc24:3934 with SMTP id d9443c01a7336-2494487342emr199626885ad.1.1756871787633; Tue, 02 Sep 2025 20:56:27 -0700 (PDT) Received: from smtpclient.apple ([142.171.105.12]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-24b2a5852a9sm24832895ad.150.2025.09.02.20.56.23 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Tue, 02 Sep 2025 20:56:27 -0700 (PDT) From: Chao Li Message-Id: <0D687585-0C6D-4B6E-B0B7-BDE084699CF3@gmail.com> Content-Type: multipart/alternative; boundary="Apple-Mail=_535E03CA-4D87-4B8A-A4F6-ED3690681F8C" Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\)) Subject: Re: SQL:2023 JSON simplified accessor support Date: Wed, 3 Sep 2025 11:56:10 +0800 In-Reply-To: Cc: jian he , Nikita Malakhov , Vik Fearing , Mark Dilger , Matheus Alcantara , Peter Eisentraut , Andrew Dunstan , Nikita Glukhov , PostgreSQL Hackers , "David E. Wheeler" To: Alexandra Wang References: <21fa36b4-6d56-4556-b524-9e3955b922c8@eisentraut.org> <290418A2-3A9B-4304-AAAD-4A40CFF1CB93@gmail.com> <98C6F445-773B-4C82-8198-7F5CD56D5252@gmail.com> X-Mailer: Apple Mail (2.3826.700.81) List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --Apple-Mail=_535E03CA-4D87-4B8A-A4F6-ED3690681F8C Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Hi Alex, Thanks for addressing my comments. I have a few follow up comments: > On Sep 3, 2025, at 10:16, Alexandra Wang = wrote: >=20 >=20 >=20 > 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. >=20 > 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.) >=20 > It's possible that in the end the "isSlice" argument isn't needed at > all, but I don=E2=80=99t think this patch set is the right place for = that > refactor. >=20 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 =3D lfirst(idx); if (IsA(ai, A_Indices) && castNode(A_Indices, ai)->is_slice) { isSlice =3D true; break; } } To out of transformContainerSubscripts(). Because the function was = called only once under =E2=80=9Cif=E2=80=9D, now this patch change it to = be called under =E2=80=9Cwhile=E2=80=9D, which brings two issues: * Original it was O(n) as it was under =E2=80=9Cif=E2=80=9D, now it is = O(n2) as it is under =E2=80=9Cwhile=E2=80=9D. * 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 =E2=80=9Cbool is_slice=E2=80=9D to it. But after = some researching, I found that making that change is more complicated = than removing =E2=80=9Cis_slice=E2=80=9D parameter from = SubscriptTransform(). That=E2=80=99s why I ended up suggesting removing = =E2=80=9Cis_slice=E2=80=9D from SubscriptTransform(). Does that sound reasonable? -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ --Apple-Mail=_535E03CA-4D87-4B8A-A4F6-ED3690681F8C Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 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=E2=80=99t = think this patch set is the right place for = that
refactor.

<= br>
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 = =3D lfirst(idx);

if (IsA(ai, A_Indices) && castNode(A_Indices, ai)->is_slice)
{
isSlice =3D true;
break;
}
}

To out of = transformContainerSubscripts(). Because the function was called only = once under =E2=80=9Cif=E2=80=9D, now this patch change it to be called = under =E2=80=9Cwhile=E2=80=9D, which brings two = issues:

 * Original it was O(n) as it was = under =E2=80=9Cif=E2=80=9D, now it is O(n2) as it is under = =E2=80=9Cwhile=E2=80=9D.
* 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 = =E2=80=9Cbool is_slice=E2=80=9D to it. But after some researching, I = found that making that change is more complicated than removing = =E2=80=9Cis_slice=E2=80=9D parameter from SubscriptTransform(). That=E2=80= =99s why I ended up suggesting removing =E2=80=9Cis_slice=E2=80=9D from = SubscriptTransform().

Does that sound = reasonable?

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




= --Apple-Mail=_535E03CA-4D87-4B8A-A4F6-ED3690681F8C--