Copilot commented on code in PR #2294:
URL: https://github.com/apache/age/pull/2294#discussion_r2651502607
##########
src/backend/parser/cypher_expr.c:
##########
@@ -600,6 +600,34 @@ static Node *transform_AEXPR_IN(cypher_parsestate
*cpstate, A_Expr *a)
Assert(is_ag_node(a->rexpr, cypher_list));
+ rexpr = (cypher_list *)a->rexpr;
+
+ /*
+ * Handle empty list case: x IN [] is always false, x NOT IN [] is always
true.
+ * We need to check this before processing to avoid returning NULL result
+ * which causes "cache lookup failed for type 0" error.
+ */
+ if (rexpr->elems == NIL || list_length((List *)rexpr->elems) == 0)
+ {
+ Datum bool_value;
+ Const *const_result;
+
+ /* If operator is <> (NOT IN), result is true; otherwise (IN) result
is false */
Review Comment:
The comment mentions "x NOT IN []" which is misleading since, as noted in
the PR description, Cypher does not have NOT IN syntax. The comment should
clarify that this refers to the internal operator representation (the "<>"
operator name), not a Cypher language construct. Consider rewording to: "Handle
empty list case: x IN [] is always false. The <> operator (internal
representation) would return true."
```suggestion
* Handle empty list case: x IN [] is always false. The <> operator
* (internal representation of NOT IN) would return true. We need to
* check this before processing to avoid returning NULL result which
* causes "cache lookup failed for type 0" error.
if (rexpr->elems == NIL || list_length((List *)rexpr->elems) == 0)
{
Datum bool_value;
Const *const_result;
/* If operator name is <> (internal NOT IN), result is true;
otherwise (IN) result is false */
```
##########
src/backend/parser/cypher_expr.c:
##########
@@ -600,6 +600,34 @@ static Node *transform_AEXPR_IN(cypher_parsestate
*cpstate, A_Expr *a)
Assert(is_ag_node(a->rexpr, cypher_list));
+ rexpr = (cypher_list *)a->rexpr;
+
+ /*
+ * Handle empty list case: x IN [] is always false, x NOT IN [] is always
true.
+ * We need to check this before processing to avoid returning NULL result
+ * which causes "cache lookup failed for type 0" error.
+ */
+ if (rexpr->elems == NIL || list_length((List *)rexpr->elems) == 0)
Review Comment:
The condition `rexpr->elems == NIL || list_length((List *)rexpr->elems) ==
0` is redundant. If `rexpr->elems` is NIL, then `list_length` will return 0.
The first check `rexpr->elems == NIL` is sufficient and more efficient since it
avoids the unnecessary function call.
```suggestion
if (rexpr->elems == NIL)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
