#4200 fix: [postgres] advertise Long scalar columns as INT8 to fix id() IN $array round-trip

[robfrank]

Closes #4200

Summary

• Cypher id() became Long-encoded in id()` may return record-id strings and break numeric predicates #4183, but the Postgres wire announced every non-array scalar column as VARCHAR. Postgres clients (psycopg, JDBC, SQLAlchemy, …) therefore deserialized id() values as strings, and when a client resent those strings as the IN $ids array parameter the Cypher engine compared Long(id(n)) against String("<decimal>") and silently returned 0 rows.
• Fix at the wire layer in PostgresNetworkExecutor.getColumns(): advertise Long-valued scalar columns as INT8 so the round-trip preserves the numeric type. Other scalars stay VARCHAR for backward compatibility with existing clients.
• The narrower comparator-side coercion (Long ↔ numeric string in InExpression / ComparisonExpression) is deliberately avoided - it would violate the Cypher TCK invariant that 5 IN ["5"] returns false (verified: 5 TCK regressions when attempted).

Test plan

• Engine Issue4183IdInArrayParameterTest — 3 cases: Long list matches, RID-string list matches (legacy coercion), numeric-string list deliberately doesn't (TCK invariant)
• OpenCypherWhereClauseTest$IdFunctionWithInOperatorRegression — 4 existing tests still pass
• Issue4183IdFunctionNumericTest — 8 tests from id()` may return record-id strings and break numeric predicates #4183 still pass
• OpenCypherTCKSuite — full 3944 tests, zero regressions
• postgresw IT suite — all 100 tests pass, including the re-enabled PostgresWJdbcIT#cypherWithArrayParameterInClause (asserts both the INT8 column-type announcement and the parameter round-trip)
• e2e-python test_psycopg2_cypher_with_array_parameter_in_clause — CI

Files

• postgresw/src/main/java/com/arcadedb/postgres/PostgresNetworkExecutor.java — Long → INT8 column type
• postgresw/src/test/java/com/arcadedb/postgres/PostgresWJdbcIT.java — re-enabled and rewritten reproducer
• engine/src/test/java/com/arcadedb/query/opencypher/Issue4183IdInArrayParameterTest.java — new engine-level reproducer
• engine/src/main/java/com/arcadedb/query/opencypher/ast/InExpression.java, .../ComparisonExpression.java — comment-only clarification of the TCK boundary

[claude]

Code Review - PR #4201

Fix: advertise Long scalar columns as INT8 in Postgres wire protocol

Overview

This PR fixes a silent data-loss bug introduced by #4183 (where id() started returning Long-encoded RIDs): the Postgres wire layer was advertising every scalar column as VARCHAR, so clients deserialized id() values as strings, then re-sent them as a List<String> parameter — which the engine silently mismatched against Long(id(n)). The fix is applied at the correct layer (wire type advertisement in getColumns()) rather than the engine comparator, preserving Cypher TCK compliance (5 IN ["5"] = false).

The overall approach is sound and well-reasoned. Comments follow below.

---

PostgresNetworkExecutor.java

Simplification opportunity. The new else if branch puts pgType (already LONG) into the map - identical to what the array branch does. The three cases could be collapsed to:

if (pgType.isArrayType() || pgType == PostgresType.LONG)
  columns.put(p, pgType);
else
  columns.put(p, PostgresType.VARCHAR);

This is more concise and makes it trivially easy to extend to INT4, FLOAT8, etc. later without adding more branches.

Scope of the change is narrow. Only Long gets the native-type treatment; Integer-, Double-, and Boolean-valued properties still advertise as VARCHAR. That is a reasonable "do-no-harm" scope for this bug, but it may mean a follow-up is needed if clients round-trip those types through Cypher parameters too. Worth a comment or issue tracking this.

Wire-level serialization. Advertising INT8 only works correctly if the actual bytes written for that column are in the format the client requested (text vs. binary). If the executor always writes text format, this works fine — the decimal representation of a Long is valid text-format INT8. If binary format can be requested and the code falls through to a generic text serializer, there is a latent failure. Worth confirming that PostgresNetworkExecutor handles the INT8 type in all transfer formats it supports.

---

Issue4183IdInArrayParameterTest.java

The three-case structure (Long list, RID-string list, numeric-string negative case) is excellent coverage of the exact TCK boundary the PR is protecting.

One nit: {@code ...} and {@link ...} appear inside regular // block comments (not /** */ Javadoc), so they render as raw text in IDEs/docs. They do no harm, but switching either to plain text or to /** */ Javadoc would be cleaner.

The @AfterEach null-guard on database is correct defensive coding.

---

PostgresWJdbcIT.java

Test approach changed from PreparedStatement to inline literal. The original disabled test used conn.prepareStatement(... IN ?) with setArray(1, conn.createArrayOf("text", rids)). The new test inlines the ids as a Cypher list literal. The PR comment says this mirrors the failing Python test scenario, which makes sense — but the PreparedStatement/setArray path is now untested. If a JDBC client actually does use setArray, the path may still silently fail (or behave differently). Consider either re-adding the parameterized variant as a second assertion block, or leaving a // TODO tracking that case.

Hardcoded index access ids[0], ids[1], ids[2]. Fine for a 3-element test, but tying the array-build to a fixed size is fragile if the test data count ever changes. A String.join or stream would be more robust:

final String inList = "[" + Arrays.stream(ids).mapToObj(Long::toString).collect(Collectors.joining(",")) + "]";

assertThat(rs.getMetaData().getColumnTypeName(1)).isEqualToIgnoringCase("int8") - This is exactly the right assertion to lock in the wire-level contract. Good call.

---

InExpression.java / ComparisonExpression.java

Comment-only changes are clear and accurately describe the TCK boundary. The {@link ...} cross-reference between the two classes is helpful for future readers.

---

Summary

Area	Assessment
Root-cause fix (wire type advertisement)	Correct and well-placed
TCK compliance preservation	Verified end-to-end
Engine-level test coverage	Solid (3 cases)
JDBC IT coverage	Good, but PreparedStatement/setArray path no longer tested
Code simplification	Minor: else if (pgType == LONG) can fold into isArrayType() check
Other numeric types (Integer, Double)	Intentionally out of scope — worth tracking as follow-up

No blocking concerns. The simplification in getColumns() and re-adding the parameterized JDBC variant (or a tracking comment) would be nice-to-have before merge.

[gemini-code-assist]

Code Review

This pull request addresses issue #4183 by ensuring that Long-encoded RIDs are correctly advertised as INT8 in the Postgres wire protocol, preventing type mismatches when clients use these values in subsequent queries. The changes include updates to the Postgres network executor, enhanced documentation regarding Cypher TCK invariants in comparison expressions, and new integration tests. Review feedback suggests simplifying the type-mapping logic and identifies a potential consistency issue where property retrieval during column metadata discovery might not align with the actual data serialization logic.

[gemini-code-assist]

The logic for determining the advertised column type can be simplified for better readability. Additionally, there is a potential consistency issue: the value used here (retrieved at line 756) does not use the fallback logic found in writeDataRows (lines 966-970). If a property is only available on the element itself and not in the top-level result map, it will be detected as null here (resulting in VARCHAR), but correctly retrieved as a Long during serialization, leading to a type mismatch in the RowDescription.

          if (pgType.isArrayType() || pgType == PostgresType.LONG)
            columns.put(p, pgType);
          else
            columns.put(p, PostgresType.VARCHAR);

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

🟢 Coverage 100.00% diff coverage · -7.94% coverage variation

Metric	Results
Coverage variation	✅ -7.94% coverage variation
Diff coverage	✅ 100.00% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (8c65bd7)	126778	93937	74.10%
Head commit (8134d09)	158478 (+31700)	104841 (+10904)	66.15% (-7.94%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#4201)	1	1	100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[robfrank]

Iteration on the review (commit 8134d09):

Applied:

• Simplification in getColumns() — folded the LONG case into the array-type branch as suggested:

  if (pgType.isArrayType() || pgType == PostgresType.LONG)
    columns.put(p, pgType);
  else
    columns.put(p, PostgresType.VARCHAR);

• PostgresWJdbcIT IN-list construction — replaced the hardcoded ids[0],ids[1],ids[2] with a loop so the test is robust to data-size changes.

• {@code} rendering nit — only applied to the appended sentences in InExpression / ComparisonExpression; my added text in Issue4183IdInArrayParameterTest was already inside /** */ Javadoc and renders correctly there.

Not changing in this PR (tracked as follow-ups):

• Other numeric / temporal / boolean scalar types — filed as [postgres] Extend wire-layer type fidelity to Integer, Double, Boolean, Date scalar columns #4202. The PR deliberately limits blast radius to Long (the id() case that regressed). Expanding to Integer, Double, Boolean, Date, etc. each needs a round-trip test and a careful audit of serializeAsText against the corresponding Postgres text-format spec (especially BOOL and DATE/TIMESTAMP) before changing the column-type announcement clients depend on.

• PreparedStatement.setArray coverage — filed as [postgres] JDBC PreparedStatement.setArray for text[] hangs in extended-protocol Bind #4203. The reason the IT test inlines the IN list rather than using setArray("text", rids) is that the setArray path triggers a separate, unrelated protocol-level hang in PostgresType.deserializeBinary for array types (Binary deserialization for arrays not yet implemented). That blocks bindCommand mid-message, leaving both client and server stuck on socket reads — verified via jstack. Re-adding the parameterized variant before fixing [postgres] JDBC PreparedStatement.setArray for text[] hangs in extended-protocol Bind #4203 would have made the IT test deadlock on every run.

Not applicable:

• Binary format concern — verified: writeRowDescription hardcodes format code 0 (text) at line 907 and serializeAsText is the only DataRow serializer used. INT8 in text format is decimal digits, which Long.toString produces — no latent failure.

Verified after iteration:

• 3900 engine + TCK tests pass (zero regressions).
• PostgresWJdbcIT#cypherWithArrayParameterInClause passes.

[claude]

Code Review - PR #4201

Fix: advertise Long scalar columns as INT8 in the Postgres wire protocol

Overview

This PR fixes a silent data-loss regression introduced by #4183, where id() started returning a Long-encoded RID. The Postgres wire layer was advertising every non-array scalar as VARCHAR, so clients deserialised id() values as strings; when they re-sent those strings as an IN $ids parameter the Cypher engine compared Long against String and returned 0 rows. The fix is applied at the correct layer (column type advertisement in getColumns()), preserving the Cypher TCK invariant that 5 IN ["5"] returns false.

---

PostgresNetworkExecutor.getColumns()

The change is minimal and correct:

if (pgType.isArrayType() || pgType == PostgresType.LONG)
  columns.put(p, pgType);
else
  columns.put(p, PostgresType.VARCHAR);

Serialization is safe. serializeAsText for LONG falls through to value.toString(), emitting the decimal string (e.g., "4294967296"). Postgres text-format INT8 accepts a decimal string, so no binary-format mismatch arises. The comment at line 904 confirms the format code is always 0 (text) in RowDescription.

Null-first-row edge case (pre-existing). getColumns() sets a column's type on the first row that contains it, and then skips it (!columns.containsKey(p)). If a column is null in every row that precedes the first non-null occurrence, getTypeForValue(null) returns VARCHAR and the column is locked in as VARCHAR for the whole result set. For id() this cannot happen since every matched vertex has a non-null id, but it is a latent issue for nullable user properties. Pre-existing, not introduced here.

Scope is intentionally narrow. Integer, Double, and Boolean scalars still advertise as VARCHAR. That is a reasonable "do-no-harm" boundary for this fix, but it means those types could regress in the same way if round-tripped through clients. Worth a follow-up issue.

---

PostgresWJdbcIT.cypherWithArrayParameterInClause

The @Disabled annotation is removed and the test is rewritten. Two concerns:

1. The PreparedStatement/setArray path is no longer tested. The original (disabled) test used:

   conn.prepareStatement("... WHERE ID(n) IN ?")
   pst.setArray(1, conn.createArrayOf("text", rids))

   The new test inlines the ids as a Cypher list literal built with StringBuilder. Inline literals bypass Postgres parameter binding entirely, so the test does not exercise the client scenario that was actually breaking (client fetches ids, stores them in an array, re-sends via setArray). The assertThat(rs.getMetaData().getColumnTypeName(1)).isEqualToIgnoringCase("int8") assertion is excellent and validates the wire contract, but the parameter round-trip half of the original test is missing.

2. Query built by string concatenation. Inlining ids into a Cypher query string via StringBuilder is fine for the Cypher literal list case (Long values only, no user input), but it is worth a comment noting why this is safe here, to prevent future copy-paste that concatenates untrusted input.

---

Issue4183IdInArrayParameterTest

The three-case structure is exactly right:

• List<Long> matches all rows - verifies engine accepts the round-tripped type
• List<String> of RID strings matches all rows - preserves legacy callers
• List<String> of numeric strings matches nothing - pins the TCK boundary

The @AfterEach null-guard and database.drop() cleanup are correct.

Minor nit: {@code ...} and {@link ...} tags inside regular // block comments (not /** */ Javadoc) render as raw text in IDEs. Either switch to plain text or convert the enclosing comments to Javadoc.

---

InExpression.java / ComparisonExpression.java

Comment-only changes. The extended description accurately explains the TCK boundary and the {@link} cross-reference between the two classes is helpful for future readers.

---

Summary

Area	Assessment
Root-cause fix (wire type advertisement)	Correct, well-placed, and verified
Wire serialization safety	Confirmed - text-format decimal is valid INT8
TCK compliance	Preserved and tested with a negative case
Engine-level test coverage	Solid (3 cases covering the full contract)
JDBC IT coverage	Wire-type assertion is good; PreparedStatement/setArray path no longer tested
Other numeric types (Integer, Double, Boolean)	Intentionally out of scope - worth a follow-up issue

No blocking concerns. The main suggestion before merge: either restore a PreparedStatement/setArray variant in PostgresWJdbcIT alongside the current literal test, or explicitly document why the inline-literal approach is sufficient to cover the real client scenario.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.58%. Comparing base (8c65bd7) to head (8134d09).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4201      +/-   ##
==========================================
- Coverage   64.95%   64.58%   -0.37%     
==========================================
  Files        1645     1645              
  Lines      126778   126800      +22     
  Branches    27123    27132       +9     
==========================================
- Hits        82352    81900     -452     
- Misses      32870    33451     +581     
+ Partials    11556    11449     -107     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.