Copilot commented on code in PR #2373:
URL: https://github.com/apache/age/pull/2373#discussion_r3039034697
##########
drivers/python/age/age.py:
##########
@@ -208,14 +208,18 @@ def _validate_column(col: str) -> str:
name, type_name = parts
validate_identifier(name, "Column name")
validate_identifier(type_name, "Column type")
- return f"{name} {type_name}"
+ # Only the column name is double-quoted. The type name is left
+ # unquoted because PostgreSQL type names in column definitions
+ # are case-insensitive identifiers; double-quoting them would
+ # force exact-case matching and break user-defined type lookup.
Review Comment:
The rationale comment about not quoting `type_name` is a bit misleading:
double-quoting doesn’t inherently “break user-defined type lookup”; it changes
name-folding semantics (quoted identifiers become case-sensitive, unquoted fold
to lower-case). Consider rewording to clarify that leaving the type unquoted
preserves PostgreSQL’s default case-folding behavior and avoids surprising
resolution changes.
```suggestion
# Only the column name is double-quoted. The type name is left
# unquoted so PostgreSQL applies its default identifier folding
# for type names in column definitions. Double-quoting would make
# the type name case-sensitive and could change type resolution in
# surprising ways for user-defined types.
```
##########
drivers/python/age/age.py:
##########
@@ -225,16 +229,18 @@ def buildCypher(graphName:str, cypherStmt:str,
columns:list) ->str:
if validated:
columnExp.append(validated)
else:
- columnExp.append('v agtype')
+ columnExp.append('"v" agtype')
Review Comment:
The new always-quoted column output from buildCypher() / _validate_column()
requires updating any in-repo callers/tests that compare generated SQL strings
(e.g., security tests that look for `v agtype` or `n agtype`). Without updating
those expectations, the Python test suite will fail.
```suggestion
columnExp.append(_validate_column('v'))
```
##########
drivers/python/test_age_py.py:
##########
@@ -28,6 +34,72 @@
TEST_GRAPH_NAME = "test_graph"
+class TestBuildCypher(unittest.TestCase):
+ """Unit tests for buildCypher() and _validate_column() — no DB required."""
+
+ def test_simple_column(self):
+ result = buildCypher("g", "MATCH (n) RETURN n", ["n"])
+ self.assertIn('"n" agtype', result)
+
+ def test_column_with_type(self):
+ result = buildCypher("g", "MATCH (n) RETURN n", ["n agtype"])
+ self.assertIn('"n" agtype', result)
Review Comment:
This change will break existing Python tests that assert unquoted column
specs (e.g., drivers/python/test_security.py currently expects `v agtype` / `n
agtype` in buildCypher() output and `_validate_column()` return values). Update
those assertions to match the new always-quoted behavior so the test suite
stays consistent and CI doesn’t fail.
##########
drivers/python/age/age.py:
##########
@@ -208,14 +208,18 @@ def _validate_column(col: str) -> str:
name, type_name = parts
validate_identifier(name, "Column name")
validate_identifier(type_name, "Column type")
- return f"{name} {type_name}"
+ # Only the column name is double-quoted. The type name is left
+ # unquoted because PostgreSQL type names in column definitions
+ # are case-insensitive identifiers; double-quoting them would
+ # force exact-case matching and break user-defined type lookup.
+ return f'"{name}" {type_name}'
else:
validate_identifier(col, "Column name")
- return f"{col} agtype"
+ return f'"{col}" agtype'
Review Comment:
Always double-quoting column names changes PostgreSQL’s identifier folding
semantics: previously an unquoted alias like `V` would be exposed as `v`
(folded to lower-case), but now `"V"` will stay uppercase. If callers relied on
the old lowercasing behavior for `cols` values with uppercase letters, this is
a user-visible behavior change; consider normalizing `name` to lower-case
before quoting or documenting this change.
--
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]
