CurtHagenlocher commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2267856494
##########
csharp/src/Drivers/Databricks/DatabricksStatement.cs:
##########
@@ -120,22 +120,14 @@ protected override void
SetStatementProperties(TExecuteStatementReq statement)
statement.MaxBytesPerFile = maxBytesPerFile;
statement.RunAsync = runAsyncInThrift;
- if (Connection.AreResultsAvailableDirectly)
- {
- statement.GetDirectResults =
DatabricksConnection.defaultGetDirectResults;
- }
+ Connection.TrySetGetDirectResults(statement);
}
/// <summary>
/// Checks if direct results are available.
/// </summary>
/// <returns>True if direct results are available and contain result
data, false otherwise.</returns>
- public bool HasDirectResults => DirectResults?.ResultSet != null &&
DirectResults?.ResultSetMetadata != null;
-
- public TSparkDirectResults? DirectResults
- {
- get { return _directResults; }
- }
+ public bool HasDirectResults => Response!.DirectResults?.ResultSet !=
null && Response!.DirectResults?.ResultSetMetadata != null;
Review Comment:
```suggestion
public bool HasDirectResults => Response?.DirectResults?.ResultSet
!= null && Response!.DirectResults?.ResultSetMetadata != null;
```
More broadly, I wonder if we shouldn't have a `GetResponse()` extension
method on `IHiveServer2Statement` which throws a specific exception if
`Response` is null and returns it otherwise. There's a lot of `Response!` in
this change and it makes me mildly uncomfortable even if my version might have
had the same property.
I don't feel super strongly about this so please push back.
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
