You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "birschick-bq (via GitHub)" <gi...@apache.org> on 2025/08/08 23:06:37 UTC
[PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
birschick-bq opened a new pull request, #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256
Refactor API to improve handling of request and responses to simplify number of overloads.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "toddmeng-db (via GitHub)" <gi...@apache.org>.
toddmeng-db commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2271357859
##########
csharp/src/Drivers/Databricks/Reader/BaseDatabricksReader.cs:
##########
@@ -44,8 +48,19 @@ protected BaseDatabricksReader(IHiveServer2Statement statement, Schema schema, b
Review Comment:
Actually, it might be better if it's in DatabricksCompositeReader? DatabricksCompositeReader doesn't inherit from BaseDatabricksReader (sorry I know that's a bit confusing), and better represents the stream's lifecycle. It's possible the BaseDatabricksReader (activeReader) owned by DatabricksCompositeReader is not yet initialized.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "birschick-bq (via GitHub)" <gi...@apache.org>.
birschick-bq commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2268057383
##########
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:
Still working ...
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "birschick-bq (via GitHub)" <gi...@apache.org>.
birschick-bq commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2270967061
##########
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:
@CurtHagenlocher
I refactored such that the Statement is no longer responsible for managing the (latest) response/operation. It is now the responsibility of the Stream to close the operation and manage its lifetime.
Let me know if this is too radical a change and can go back to the previous implementation.
Thanks for your input.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "toddmeng-db (via GitHub)" <gi...@apache.org>.
toddmeng-db commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2271357859
##########
csharp/src/Drivers/Databricks/Reader/BaseDatabricksReader.cs:
##########
@@ -44,8 +48,19 @@ protected BaseDatabricksReader(IHiveServer2Statement statement, Schema schema, b
Review Comment:
Actually, it might be better if it's in DatabricksCompositeReader? DatabricksCompositeReader doesn't inherit from BaseDatabricksReader (sorry I know that's a bit confusing, probably needs a rename), and better represents the stream's lifecycle. It's possible the BaseDatabricksReader (activeReader) owned by DatabricksCompositeReader is not yet initialized when DatabricksCompositeReader is initialized.
Currently this is how they relate:
DatabricksCompositeReader is created first. If there is directresults, we initialize the BaseDatabricksReader. If there is no directresults, we wait for the first FetchResultsResponse and inspect the result to determine which BaseDatabricksReader we want to use
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "CurtHagenlocher (via GitHub)" <gi...@apache.org>.
CurtHagenlocher merged PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "birschick-bq (via GitHub)" <gi...@apache.org>.
birschick-bq commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2268036437
##########
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:
Note: retained Response as a property otherwise Visual Studio static analysis wouldn't trust the previous null check.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "birschick-bq (via GitHub)" <gi...@apache.org>.
birschick-bq commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2271198857
##########
csharp/src/Drivers/Databricks/Reader/BaseDatabricksReader.cs:
##########
@@ -44,8 +48,19 @@ protected BaseDatabricksReader(IHiveServer2Statement statement, Schema schema, b
Review Comment:
@toddmeng-db - If you could run the E2E tests against a Databricks server to confirm I haven't caused any regressions. If you could check that there are no open operations after correct Dispose of streams - that would be great, too.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "CurtHagenlocher (via GitHub)" <gi...@apache.org>.
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "birschick-bq (via GitHub)" <gi...@apache.org>.
birschick-bq commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2274542453
##########
csharp/src/Drivers/Databricks/Reader/BaseDatabricksReader.cs:
##########
@@ -44,8 +48,19 @@ protected BaseDatabricksReader(IHiveServer2Statement statement, Schema schema, b
Review Comment:
Thanks for confirming!
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "CurtHagenlocher (via GitHub)" <gi...@apache.org>.
CurtHagenlocher commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2270972855
##########
csharp/src/Drivers/Apache/Hive2/IHiveServer2Statement.cs:
##########
@@ -40,7 +34,9 @@ internal interface IHiveServer2Statement : ITracingStatement
/// Checks if direct results are available.
/// </summary>
/// <returns>True if direct results are available and contain result data, false otherwise.</returns>
- bool HasDirectResults { get; }
+ bool HasDirectResults(IResponse response);
+
+ bool TryGetDirectResults(IResponse response, out TSparkDirectResults? directResults);
Review Comment:
Given that all the other methods in the interface have doc comments, could you add them here too?
##########
csharp/src/Drivers/Apache/Hive2/HiveServer2Statement.cs:
##########
@@ -384,20 +375,7 @@ private void UpdateBatchSizeIfValid(string key, string value) => BatchSize = !st
public override void Dispose()
{
- this.TraceActivity(activity =>
- {
- if (Response != null && Response.OperationHandle != null && Response.DirectResults?.CloseOperation?.Status?.StatusCode != TStatusCode.SUCCESS_STATUS)
- {
- CancellationToken cancellationToken = ApacheUtility.GetCancellationToken(QueryTimeoutSeconds, ApacheUtility.TimeUnit.Seconds);
- activity?.AddTag(SemanticConventions.Db.Operation.OperationId, Response.OperationHandle!.OperationId.Guid, "N");
- TCloseOperationReq request = new TCloseOperationReq(Response.OperationHandle!);
- TCloseOperationResp resp = Connection.Client.CloseOperation(request, cancellationToken).Result;
- HiveServer2Connection.HandleThriftResponse(resp.Status, activity);
- }
- Response = null;
-
- base.Dispose();
- });
+ base.Dispose();
Review Comment:
Can we just remove this method?
##########
csharp/src/Drivers/Databricks/Reader/DatabricksOperationStatusPoller.cs:
##########
@@ -69,7 +73,7 @@ private async Task PollOperationStatus(CancellationToken cancellationToken)
{
while (!cancellationToken.IsCancellationRequested)
{
- var operationHandle = _statement.Response!.OperationHandle;
+ TOperationHandle? operationHandle = _response?.OperationHandle;
Review Comment:
`_response` isn't marked as nullable so the `?` shouldn't be needed here.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "birschick-bq (via GitHub)" <gi...@apache.org>.
birschick-bq commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2271174244
##########
csharp/src/Drivers/Databricks/Reader/BaseDatabricksReader.cs:
##########
@@ -44,8 +48,19 @@ protected BaseDatabricksReader(IHiveServer2Statement statement, Schema schema, b
Review Comment:
@toddmeng-db - When you say "this logic" are you referring to the `CloseOperation` call in the `Dispose` method?
Both `BaseDatabricksReader` and `DatabricksCompositeReader` inherit from `TracingReader`. So I've coded the `CloseOperation` call in both of their `Dispose` methods.
I'm I missing something?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "toddmeng-db (via GitHub)" <gi...@apache.org>.
toddmeng-db commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2271023452
##########
csharp/src/Drivers/Databricks/Reader/BaseDatabricksReader.cs:
##########
@@ -44,8 +48,19 @@ protected BaseDatabricksReader(IHiveServer2Statement statement, Schema schema, b
Review Comment:
small nit, I wonder if this logic should go in DatabricksCompositeReader
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "birschick-bq (via GitHub)" <gi...@apache.org>.
birschick-bq commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2267936616
##########
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:
I agree with your concern. Specifically, the Dispose can set it to `null`. I'll make the 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "birschick-bq (via GitHub)" <gi...@apache.org>.
birschick-bq commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2268036437
##########
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:
Note: retained Response as a property otherwise Visual Studio static analysis wouldn't trust the previous null check.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "toddmeng-db (via GitHub)" <gi...@apache.org>.
toddmeng-db commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2271357859
##########
csharp/src/Drivers/Databricks/Reader/BaseDatabricksReader.cs:
##########
@@ -44,8 +48,19 @@ protected BaseDatabricksReader(IHiveServer2Statement statement, Schema schema, b
Review Comment:
Actually, it might be better if it's in DatabricksCompositeReader? DatabricksCompositeReader doesn't inherit from BaseDatabricksReader (sorry I know that's a bit confusing), and better represents the stream's lifecycle. It's possible the BaseDatabricksReader (activeReader) owned by DatabricksCompositeReader is not yet initialized.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "toddmeng-db (via GitHub)" <gi...@apache.org>.
toddmeng-db commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2271203554
##########
csharp/src/Drivers/Databricks/Reader/BaseDatabricksReader.cs:
##########
@@ -44,8 +48,19 @@ protected BaseDatabricksReader(IHiveServer2Statement statement, Schema schema, b
Review Comment:
Oh, sorry. DatabricksCompositeReader will end up calling duplicate CloseOperation during BaseDatabricksReader.Dispose() (through activeReader.dispose()), do we want to avoid this?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "birschick-bq (via GitHub)" <gi...@apache.org>.
birschick-bq commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2271228746
##########
csharp/src/Drivers/Databricks/Reader/BaseDatabricksReader.cs:
##########
@@ -44,8 +48,19 @@ protected BaseDatabricksReader(IHiveServer2Statement statement, Schema schema, b
Review Comment:
Removed the duplicate call to `CloseOperation` in `DatabricksCompositeReader`. It should be in the BaseDatabricksReader so that inheritors get the benefit.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "toddmeng-db (via GitHub)" <gi...@apache.org>.
toddmeng-db commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2271203554
##########
csharp/src/Drivers/Databricks/Reader/BaseDatabricksReader.cs:
##########
@@ -44,8 +48,19 @@ protected BaseDatabricksReader(IHiveServer2Statement statement, Schema schema, b
Review Comment:
Oh, sorry. DatabricksCompositeReader will end up calling duplicate BaseDatabricksReader.Dispose() (through activeReader.dispose()), do we want to avoid this?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "birschick-bq (via GitHub)" <gi...@apache.org>.
birschick-bq commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2271412388
##########
csharp/src/Drivers/Databricks/Reader/BaseDatabricksReader.cs:
##########
@@ -44,8 +48,19 @@ protected BaseDatabricksReader(IHiveServer2Statement statement, Schema schema, b
Review Comment:
@toddmeng-db - Added a "stateful" close operation on the `BaseDatabricksReader`. Let me know if this is too complicated a solution.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "CurtHagenlocher (via GitHub)" <gi...@apache.org>.
CurtHagenlocher commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2271004959
##########
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:
> @CurtHagenlocher I refactored such that the Statement is no longer responsible for managing the (latest) response/operation. It is now the responsibility of the Stream to close the operation and manage its lifetime.
This is great! If we ever want to change the connection to support multiple concurrent statements, this will have done most of the heavy lifting.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "birschick-bq (via GitHub)" <gi...@apache.org>.
birschick-bq commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2271174244
##########
csharp/src/Drivers/Databricks/Reader/BaseDatabricksReader.cs:
##########
@@ -44,8 +48,19 @@ protected BaseDatabricksReader(IHiveServer2Statement statement, Schema schema, b
Review Comment:
@toddmeng-db - When you say "this logic" are you referring to the `CloseOperation` call in the `Dispose` method?
Both `BaseDatabricksReader` and `DatabricksCompositeReader` inherit from `TracingReader`. So I've coded the `CloseOperation` call in both of their `Dispose` methods.
Am I missing something?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "birschick-bq (via GitHub)" <gi...@apache.org>.
birschick-bq commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2271210385
##########
csharp/src/Drivers/Databricks/Reader/BaseDatabricksReader.cs:
##########
@@ -44,8 +48,19 @@ protected BaseDatabricksReader(IHiveServer2Statement statement, Schema schema, b
Review Comment:
Ah. **Good catch**. I didn't see the containment of `BaseDatabricksReading` inside `DatabricksCompositeReader`.
Thanks!
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "birschick-bq (via GitHub)" <gi...@apache.org>.
birschick-bq commented on PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#issuecomment-3176387015
@CurtHagenlocher - Resolved the merge conflicts. Added a check to ensure the HiveServer2Statement doesn't get re-used as that would change the Response object and make it inconsistent for the QueryResult/Stream.
@jadewang-db Can you run the E2E test against a Databrick system to confirm no regressions?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "toddmeng-db (via GitHub)" <gi...@apache.org>.
toddmeng-db commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2271357859
##########
csharp/src/Drivers/Databricks/Reader/BaseDatabricksReader.cs:
##########
@@ -44,8 +48,19 @@ protected BaseDatabricksReader(IHiveServer2Statement statement, Schema schema, b
Review Comment:
Actually, it might be better if it's in DatabricksCompositeReader? DatabricksCompositeReader doesn't inherit from BaseDatabricksReader (sorry I know that's a bit confusing, probably needs a rename), and better represents the stream's lifecycle. It's possible the BaseDatabricksReader (activeReader) owned by DatabricksCompositeReader is not yet initialized when DatabricksCompositeReader is initialized.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(csharp/src/Drivers/Apache): Cleanup HiveServer2-based code with shared Thrift request/response interfaces [arrow-adbc]
Posted by "toddmeng-db (via GitHub)" <gi...@apache.org>.
toddmeng-db commented on code in PR #3256:
URL: https://github.com/apache/arrow-adbc/pull/3256#discussion_r2274539949
##########
csharp/src/Drivers/Databricks/Reader/BaseDatabricksReader.cs:
##########
@@ -44,8 +48,19 @@ protected BaseDatabricksReader(IHiveServer2Statement statement, Schema schema, b
Review Comment:
Tests look good to me
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org