[PATCH] Fix wrong comment in JsonTablePlanJoinNextRow()

Started by 胡传文1 day ago4 messageshackers
Jump to latest
#1胡传文
463945512@qq.com

Hi,

Found a misleading comment in JsonTablePlanJoinNextRow() while reading
the JSON_TABLE execution code.

The function returns false when both siblings are exhausted (meaning no
more rows), but the comment says "there are more rows" — the exact
opposite of what's happening. The code itself is correct.

if (!JsonTablePlanNextRow(planstate->right))
{
/* Right sibling ran out of row, so there are more rows. */ /* wrong */
return false;
}

A reader might reasonably treat this as a bug and flip the return value,
which would cause JSON_TABLE UNION plans to loop indefinitely.

Patch attached.

Regards,
Chuanwen Hu

Attachments:

fix-jsontable-comment.patchapplication/octet-stream; charset=gb18030; name=fix-jsontable-comment.patchDownload+1-1
#2Chao Li
li.evan.chao@gmail.com
In reply to: 胡传文 (#1)
Re: [PATCH] Fix wrong comment in JsonTablePlanJoinNextRow()

On Apr 15, 2026, at 16:28, 胡传文 <463945512@qq.com> wrote:

Hi,
Found a misleading comment in JsonTablePlanJoinNextRow() while reading
the JSON_TABLE execution code.
The function returns false when both siblings are exhausted (meaning no
more rows), but the comment says "there are more rows" — the exact
opposite of what's happening. The code itself is correct.
if (!JsonTablePlanNextRow(planstate->right))
{
/* Right sibling ran out of row, so there are more rows. */ /* wrong */
return false;
}
A reader might reasonably treat this as a bug and flip the return value,
which would cause JSON_TABLE UNION plans to loop indefinitely.
Patch attached.
Regards,
Chuanwen Hu<fix-jsontable-comment.patch>

The fix looks correct to me. I guess “no” was just unintentionally missed.

A small comment, I just feel “too” you newly added might not be needed.

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

#3Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Chao Li (#2)
Re: [PATCH] Fix wrong comment in JsonTablePlanJoinNextRow()

Hi,

On Thu, Apr 16, 2026 at 11:05 AM Chao Li <li.evan.chao@gmail.com> wrote:

On Apr 15, 2026, at 16:28, 胡传文 <463945512@qq.com> wrote:

Hi,
Found a misleading comment in JsonTablePlanJoinNextRow() while reading
the JSON_TABLE execution code.
The function returns false when both siblings are exhausted (meaning no
more rows), but the comment says "there are more rows" — the exact
opposite of what's happening. The code itself is correct.
if (!JsonTablePlanNextRow(planstate->right))
{
/* Right sibling ran out of row, so there are more rows. */ /* wrong */
return false;
}
A reader might reasonably treat this as a bug and flip the return value,
which would cause JSON_TABLE UNION plans to loop indefinitely.
Patch attached.

Thanks for the report and the patch.

The fix looks correct to me. I guess “no” was just unintentionally missed.

A small comment, I just feel “too” you newly added might not be needed.

Actually, I'd keep it, because it ties the comment to the left-sibling
check just above.

Will push the attached down to v17.

--
Thanks, Amit Langote

Attachments:

v1-0001-Fix-incorrect-comment-in-JsonTablePlanJoinNextRow.patchapplication/octet-stream; name=v1-0001-Fix-incorrect-comment-in-JsonTablePlanJoinNextRow.patchDownload+1-2
#4Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Amit Langote (#3)
Re: [PATCH] Fix wrong comment in JsonTablePlanJoinNextRow()

On Thu, Apr 16, 2026 at 12:03 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Apr 16, 2026 at 11:05 AM Chao Li <li.evan.chao@gmail.com> wrote:

On Apr 15, 2026, at 16:28, 胡传文 <463945512@qq.com> wrote:

Hi,
Found a misleading comment in JsonTablePlanJoinNextRow() while reading
the JSON_TABLE execution code.
The function returns false when both siblings are exhausted (meaning no
more rows), but the comment says "there are more rows" — the exact
opposite of what's happening. The code itself is correct.
if (!JsonTablePlanNextRow(planstate->right))
{
/* Right sibling ran out of row, so there are more rows. */ /* wrong */
return false;
}
A reader might reasonably treat this as a bug and flip the return value,
which would cause JSON_TABLE UNION plans to loop indefinitely.
Patch attached.

Thanks for the report and the patch.

The fix looks correct to me. I guess “no” was just unintentionally missed.

A small comment, I just feel “too” you newly added might not be needed.

Actually, I'd keep it, because it ties the comment to the left-sibling
check just above.

Will push the attached down to v17.

Done.

--
Thanks, Amit Langote