[xborder]

What Changed

• This change simplifies the Flight SQL JDBC request flow for Statement.executeQuery/executeUpdate avoiding the PreparedStatement request flow
• Preserved the current requirement that Statement.execute(String) still flows through PreparedStatement
  • Right now, Arrow Flight does not offer a good solution for this use case. There is no way for the client to disambiguate between a CommandStatementUpdate or CommandStatemenQuery without doing a server call
• Moved prepared-statement-specific creation and execution concerns into ArrowFlightPreparedStatement
• Similarly, created ArrowFlightStatement with specific logic to handle Statements
• Removed statement/prepared-statement construction details and logic from ArrowFlightMetaImpl. ArrowFlightMetaImpl was left it focused on orchestration

Closes #62.

This PR was assisted by AI

[ennuite]

Perhaps a nitpick, but if we run multiple queries with the same statement using execute we will only close one of the handles in the server.

Code for repro:

    try (Connection connection = DriverManager.getConnection(jdbcUrl, properties);
         Statement statement = connection.createStatement()) {

      for (int i = 1; i <= n; i++) {
        String sql = "SELECT " + i;
        log("Executing: " + sql);

        boolean isResultSet = statement.execute(sql);
        if (isResultSet) {
          try (ResultSet rs = statement.getResultSet()) {
            // consume result set
          }
        } else {
          System.out.println("Updated rows: " + statement.getUpdateCount());
        }
      }
    }

A new prepared statement will be created on each query, but only one ClosePreparedStatement is sent to the server. I believe most Flight SQL servers are stateless and this won't be problematic in that scenario, but in case the server is holding some resources associated with the handle(s) of the prepared statement(s) that aren't closed there will be a leak.

Could we send the close after execution?

[xborder]

Good catch. This issue existed before the refactor. I've created #1129 to be addressed separately

[jbonofre]

@xborder thanks for the update! I will do the review.

[lidavidm]

I wonder, could we separate this change out into its own PR so it shows up more clearly in the changelog/history and is easier to review?

[lidavidm]

Albeit if it can't be separated then that's fine

[xborder]

This is to ensure that we close the result sets for the original statements when the connection is closed or reset, even if their entries in Avatica’s statement map have been replaced.

[jbonofre]

Calling Statement.execute(String) again on the same statement leaks the previous server-side prepared statement.

On the second call, statementMap holds the internal ArrowFlightPreparedStatement created by the first call, so ArrowFlightMetaImpl.prepareAndExecute routes here. We build a new prepared statement under the same handle, and its constructor replaces the statementMap entry, but this.preparedStatement is never closed. No ClosePreparedStatement is sent for it. A loop of N execute() calls leaves N-1 prepared statements open on the server, and stmt.close() only closes the last one.

The old statementHandlePreparedStatementMap.put(...) dropped the previous entry the same way, so this is not a regression. Since this method is new, could we close the previous one once the replacement is built?

final ArrowFlightPreparedStatement replacement =
    ArrowFlightPreparedStatement.builder(getConnection())
        .withQuery(query)
        .withExistingStatement(this)
        .build();
closePreparedResources();
return replacement.prepareAndExecuteInternal(callback);

A test that calls execute() a few times on one statement and checks how many prepared statements the mock producer closed would cover this.

[xborder]

I had already identified this issue. It already exists on main. I suggest addressing it in a follow-up PR after this one merges.

[jbonofre]

That works for me. Thanks!

[jbonofre]

If anything throws between prepare() and the end of build(), the server side prepared statement is never cloud. One concrete case: a parameter schema with a ListView, LargeListView or RunEndEncoded field makes ConvertUtils.convertArrowFieldsToAvaticaParameters throw UnsupportedOperationException from buildSignature.

The old prepareForHandle had the same gap. Now that build() owns the whole lifecycle, could we close it on failure?

final ArrowFlightPreparedStatement statement;
try {
  final Signature signature =
      ArrowFlightMetaImpl.buildSignature(
          query,
          preparedStatement.getDataSetSchema(),
          preparedStatement.getParameterSchema(),
          preparedStatement.isUpdate());
  statement =
      new ArrowFlightPreparedStatement(
          connection,
          generateHandle ? null : handle,
          signature,
          preparedStatement,
          resultSetType,
          resultSetConcurrency,
          resultSetHoldability);
} catch (final Exception e) {
  try {
    preparedStatement.close();
  } catch (final Exception closeException) {
    e.addSuppressed(closeException);
  }
  throw e;
}

[xborder]

#1090 (comment)

[jbonofre]

Thanks for the iterations @xborder!

Great work!