[PECOBLR-2409] Add CallableStatement support with IN parameters

[gopalldb]

Summary

• Implement DatabricksCallableStatement extending DatabricksPreparedStatement to support stored procedure invocation via Connection.prepareCall()
• IN parameters work via inherited setXXX(int, value) methods
• {call proc(?)} JDBC escape syntax is converted to CALL proc(?) at construction time, independent of the escapeProcessing flag
• OUT/INOUT parameters, named parameters, and {? = call ...} return-value syntax throw SQLFeatureNotSupportedException with clear messages
• Wire up all 3 prepareCall() overloads in DatabricksConnection with proper resultSetType/holdability validation
• shouldReturnResultSet is always true -- use execute() or executeQuery() to invoke procedures
• Java 8+ SQLType-based overrides explicitly implemented for consistent error messages

Known limitations

• Nested JDBC escape sequences inside {call ...} arguments are not supported -- consistent with StringUtil.convertJdbcEscapeSequences. Use native SQL syntax instead.
• {? = call ...} return-value syntax is rejected at construction time
• executeUpdate() always throws for callable statements since shouldReturnResultSet=true. Use execute() for DML procedures.

Test plan

• DatabricksCallableStatementTest -- 30 tests
• DatabricksConnectionTest -- 42 tests pass
• DatabricksPreparedStatementTest -- 48 tests, 0 regressions
• Full core test suite -- 3208 tests, 0 failures

This pull request was AI-assisted by Isaac.

[vikrantpuppala]

Code Review: DatabricksCallableStatement — CallableStatement with IN Parameters

Overall this is a solid implementation. The design choice to extend DatabricksPreparedStatement is the right call — it follows the JDBC hierarchy and gets all setXXX(int, value) / execution / batch functionality for free. The test coverage is thorough (30 tests covering construction, execution, OUT param rejection, named param rejection, and error messages). The {call ...} escape conversion at construction time is a clever solution to ensure shouldReturnResultSet works correctly regardless of escapeProcessing.

A few issues to address before merge:

---

Critical: Constructor validates return-value syntax AFTER calling super()

DatabricksCallableStatement.java:70-71

super(connection, convertCallEscapeSyntax(sql));   // line 70
validateNoReturnValueSyntax(sql);                   // line 71

If a user passes {? = call my_func()}, the CALL_ESCAPE_SYNTAX pattern (\{\s*call\s+...) does not match {? = call ...} (because ?\s*=\s* precedes call), so convertCallEscapeSyntax returns the original string unchanged. The parent constructor then receives {? = call my_func()} as-is, which:

1. Won't match CALL_PATTERN (^CALL\b) in shouldReturnResultSetWithConfig, so shouldReturnResultSet is set to false (wrong for a callable statement)
2. Partially initializes the parent object before the validation on line 71 throws

The statement gets added to statementSet in DatabricksConnection.prepareCall() only after the constructor returns, so there's no leak — but the ordering is still incorrect.

Fix: Move validation before the super() call using a static factory method:

public DatabricksCallableStatement(DatabricksConnection connection, String sql)
    throws SQLException {
  super(connection, validateAndConvert(sql));
  LOGGER.debug("Created DatabricksCallableStatement for SQL: {}", sql);
}

private static String validateAndConvert(String sql) throws SQLException {
  if (sql != null && RETURN_VALUE_SYNTAX.matcher(sql).find()) {
    throw new DatabricksSQLFeatureNotSupportedException(
        "Return value syntax {? = call ...} is not supported. "
            + "Use {call proc(...)} and retrieve results via the ResultSet.");
  }
  return convertCallEscapeSyntax(sql);
}

---

Major: Missing closed-connection check in prepareCall(String) and prepareCall(String, int, int)

DatabricksConnection.java

The 3-arg prepareCall(sql, type, concurrency, holdability) correctly checks isClosed(), but the 1-arg and 2-arg overloads do not. I note that prepareStatement(String) also lacks this check (pre-existing), so this is consistent with the existing pattern — but since you're adding the check in the 3-arg overload, it would be good to be consistent across all three prepareCall overloads.

The 2-arg overload delegates to the 1-arg, so adding the check to prepareCall(String) covers both:

@Override
public CallableStatement prepareCall(String sql) throws SQLException {
  throwExceptionIfConnectionIsClosed();
  LOGGER.debug(...);
  DatabricksCallableStatement statement = new DatabricksCallableStatement(this, sql);
  statementSet.add(statement);
  return statement;
}

Severity note: This is a pre-existing inconsistency (prepareStatement has the same gap), so I wouldn't block the PR on it. But since you're already touching these methods, it's a good opportunity to fix.

---

Major: executeUpdate() always throws for callable statements

Since CALL_PATTERN matches, shouldReturnResultSet is true, and DatabricksPreparedStatement.executeUpdate() throws when shouldReturnResultSet is true. This means executeUpdate() always fails for callable statements, even for DML procedures.

The Javadoc correctly documents the workaround ("use execute() and check getUpdateCount()"), but per JDBC spec Section 13.2, executeUpdate() should be usable for stored procedures that don't return result sets.

This is acceptable as a known limitation for v1, but consider:

1. Overriding executeUpdate() in DatabricksCallableStatement with a clearer error message specific to callable statements (rather than the generic prepared statement error)
2. Adding this to the "Known limitations" section of the PR description ✅ (already done)

---

Minor: RETURN_VALUE_SYNTAX pattern doesn't account for string literals

DatabricksCallableStatement.java:43

The pattern \{\s*\?\s*=\s*call\b performs a naive search of the full SQL string. A (contrived) SQL like:

CALL proc('{? = call foo}')

would be incorrectly rejected because the pattern appears inside a string literal argument. This is an unlikely edge case for callable statements, but worth a Javadoc note.

---

Minor: CALL_ESCAPE_SYNTAX uses [^}]* which doesn't handle nested braces

DatabricksCallableStatement.java:52

Already documented in the Javadoc as a known limitation consistent with StringUtil.convertJdbcEscapeSequences. No action needed — just confirming the documentation is accurate. 👍

---

Nit: Test testPrepareCallOnClosedConnectionThrows only tests the 3-arg overload

DatabricksCallableStatementTest.java:148-156

The closed-connection test only verifies the 3-arg prepareCall(sql, type, concurrency, holdability). If the closed-connection check is added to the 1-arg overload (per the suggestion above), add a test for that path too.

---

Nit: Debug log messages are method signatures

Throughout DatabricksCallableStatement, debug log messages are method signatures like "public void registerOutParameter(int parameterIndex, int sqlType)". This is consistent with the parent class's logging style, so not specific to this PR, but including actual parameter values would be more useful for debugging.

---

Summary

Severity	#	Key Items
Critical	1	Constructor validation ordering — validate before super()
Major	2	Missing closed-connection check on 1-arg/2-arg prepareCall; executeUpdate() always throws
Minor	2	Regex false positive on string literals; nested brace limitation (documented)
Nit	2	Closed-connection test coverage; debug log content

The critical item (constructor ordering) should be fixed before merge. The rest are improvements that could go in a follow-up.

Test coverage is strong — construction, execution, escape processing independence, batch operations, parameter metadata, exhaustive OUT/named parameter rejection, and error message validation are all well covered. Nice work. 👏

---

This comment was generated with GitHub MCP.