Copilot commented on code in PR #2415: URL: https://github.com/apache/age/pull/2415#discussion_r3150586350
########## regress/expected/reserved_keyword_alias.out: ########## @@ -0,0 +1,237 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/* + * Regression coverage for issue #2355: + * Cypher reserved keywords from the `safe_keywords` set must be accepted + * in alias positions (RETURN/WITH/YIELD ... AS <name>, UNWIND ... AS <name>). + * Conflicting tokens (END / NULL / TRUE / FALSE) must remain rejected. + * Pattern variable bindings are intentionally still restricted to plain + * identifiers. + */ + +LOAD 'age'; +SET search_path TO ag_catalog; + +SELECT * FROM create_graph('issue_2355'); Review Comment: The expected-output file also has CRLF / trailing `\r` characters (e.g., echoed `SET search_path` line). Please regenerate/normalize this file with LF-only line endings to keep regression outputs stable across platforms. ########## src/backend/parser/cypher_gram.y: ########## @@ -2337,6 +2337,31 @@ var_name: } ; +/* + * var_name_alias is used in alias positions (RETURN/WITH/YIELD ... AS x, + * UNWIND ... AS x) where the AS keyword removes any lookahead ambiguity. + * Beyond plain identifiers, it permits the same set of non-conflicting + * reserved keywords accepted by safe_keywords (and already accepted in + * func_name / schema_name), so that legitimate Cypher such as + * RETURN 1 AS count + * RETURN n AS exists + * UNWIND [1, 2] AS row + * is parsed correctly. Truly conflicting tokens (END, NULL, TRUE, FALSE) + * are listed under conflicted_keywords (not safe_keywords) and remain + * rejected here. See issue #2355. + * + * It is intentionally NOT used in pattern variable positions + * ((x:Label), [r:REL]) or named-path bindings (p = ...), because + * allowing reserved keywords there introduces shift/reduce ambiguity. + */ +var_name_alias: + var_name + | safe_keywords + { + $$ = pstrdup((char *) $1); Review Comment: In `var_name_alias`, the `safe_keywords` branch does an extra `pstrdup`, but `safe_keywords` already duplicates its keyword text via `KEYWORD_STRDUP(...)` (see `safe_keywords:` rule). This results in redundant allocations for every keyword alias; consider assigning `$$ = (char *) $1` directly (consistent with `schema_name: reserved_keyword`). ```suggestion /* safe_keywords already duplicates keyword text */ $$ = (char *) $1; ``` ########## regress/sql/reserved_keyword_alias.sql: ########## @@ -0,0 +1,90 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/* + * Regression coverage for issue #2355: + * Cypher reserved keywords from the `safe_keywords` set must be accepted + * in alias positions (RETURN/WITH/YIELD ... AS <name>, UNWIND ... AS <name>). + * Conflicting tokens (END / NULL / TRUE / FALSE) must remain rejected. + * Pattern variable bindings are intentionally still restricted to plain + * identifiers. + */ + +LOAD 'age'; +SET search_path TO ag_catalog; + +SELECT * FROM create_graph('issue_2355'); Review Comment: This new regression SQL file contains Windows-style CRLF / stray carriage-return characters (e.g., the `SET search_path` line is recorded with a trailing `\r`). Please normalize the file to LF-only line endings to avoid noisy diffs and platform-specific test failures. ########## regress/sql/reserved_keyword_alias.sql: ########## @@ -0,0 +1,90 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/* + * Regression coverage for issue #2355: + * Cypher reserved keywords from the `safe_keywords` set must be accepted + * in alias positions (RETURN/WITH/YIELD ... AS <name>, UNWIND ... AS <name>). + * Conflicting tokens (END / NULL / TRUE / FALSE) must remain rejected. + * Pattern variable bindings are intentionally still restricted to plain + * identifiers. + */ + +LOAD 'age'; +SET search_path TO ag_catalog; + +SELECT * FROM create_graph('issue_2355'); + +-- The exact reproducer from the issue (previously failed with +-- "syntax error at or near "count""). +SELECT * FROM cypher('issue_2355', $$ RETURN 1 AS count $$) AS (a agtype); + +-- Representative coverage across the safe_keywords set as RETURN aliases. +SELECT * FROM cypher('issue_2355', $$ RETURN 1 AS exists $$) AS (a agtype); +SELECT * FROM cypher('issue_2355', $$ RETURN 1 AS coalesce $$) AS (a agtype); +SELECT * FROM cypher('issue_2355', $$ RETURN 1 AS match $$) AS (a agtype); +SELECT * FROM cypher('issue_2355', $$ RETURN 1 AS return $$) AS (a agtype); +SELECT * FROM cypher('issue_2355', $$ RETURN 1 AS where $$) AS (a agtype); +SELECT * FROM cypher('issue_2355', $$ RETURN 1 AS order $$) AS (a agtype); +SELECT * FROM cypher('issue_2355', $$ RETURN 1 AS limit $$) AS (a agtype); +SELECT * FROM cypher('issue_2355', $$ RETURN 1 AS distinct $$) AS (a agtype); +SELECT * FROM cypher('issue_2355', $$ RETURN 1 AS optional $$) AS (a agtype); +SELECT * FROM cypher('issue_2355', $$ RETURN 1 AS detach $$) AS (a agtype); +SELECT * FROM cypher('issue_2355', $$ RETURN 1 AS contains $$) AS (a agtype); +SELECT * FROM cypher('issue_2355', $$ RETURN 1 AS starts $$) AS (a agtype); +SELECT * FROM cypher('issue_2355', $$ RETURN 1 AS ends $$) AS (a agtype); +SELECT * FROM cypher('issue_2355', $$ RETURN 1 AS in $$) AS (a agtype); +SELECT * FROM cypher('issue_2355', $$ RETURN 1 AS is $$) AS (a agtype); +SELECT * FROM cypher('issue_2355', $$ RETURN 1 AS not $$) AS (a agtype); +SELECT * FROM cypher('issue_2355', $$ RETURN 1 AS yield $$) AS (a agtype); +SELECT * FROM cypher('issue_2355', $$ RETURN 1 AS call $$) AS (a agtype); + +-- Multiple keyword aliases in one projection. +SELECT * FROM cypher('issue_2355', + $$ RETURN 1 AS count, 2 AS exists, 3 AS where $$ +) AS (count agtype, ex agtype, w agtype); + +-- WITH ... AS <safe_keyword>: alias binding works. +SELECT * FROM cypher('issue_2355', + $$ WITH 1 AS count RETURN 1 AS x $$ +) AS (a agtype); + +-- UNWIND ... AS <safe_keyword>: alias binding works. +SELECT * FROM cypher('issue_2355', + $$ UNWIND [1, 2, 3] AS row RETURN 1 AS x $$ +) AS (a agtype); + Review Comment: The grammar change also enables `YIELD ... AS <safe_keyword>` (via `yield_item: expr AS var_name_alias`), but the new regression test only covers RETURN/WITH/UNWIND. Consider adding at least one `CALL ... YIELD <expr> AS <safe_keyword>` case here (that doesn't reference the keyword alias later, since keyword-named references remain out of scope). ```suggestion -- CALL ... YIELD ... AS <safe_keyword>: alias binding works. Do not -- reference the keyword alias later; keyword-named references remain -- intentionally out of scope for this regression. SELECT * FROM cypher('issue_2355', $$ CALL db.labels() YIELD label AS exists RETURN 1 AS x $$ ) AS (a agtype); ``` -- 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]
