Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(csharp/src/Apache.Arrow.Adbc): proof of concept - unit test - proxy server injection #1

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

birschick-bq
Copy link
Owner

No description provided.

@@ -37,7 +38,7 @@

namespace Apache.Arrow.Adbc.Drivers.Apache.Spark
{
public class SparkConnection : HiveServer2Connection
internal class SparkConnection : HiveServer2Connection
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only SparkDriver needs to be public

@@ -252,8 +253,8 @@ internal enum ColumnTypeId
TIMESTAMP_WITH_TIMEZONE = 2014,
}

internal SparkConnection(IReadOnlyDictionary<string, string> properties)
: base(properties)
internal SparkConnection(IReadOnlyDictionary<string, string>? properties, MockDataSourceBase<TCLIService.IAsync>? proxy = default)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only SparkDriver needs to be public


namespace Apache.Arrow.Adbc.Drivers.Apache.Spark
{
public class SparkDatabase : AdbcDatabase
internal class SparkDatabase : AdbcDatabase, IMockingDatabase<TCLIService.IAsync>
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only SparkDriver needs to be public

using Apache.Arrow.Ipc;
using Apache.Hive.Service.Rpc.Thrift;
using Thrift.Protocol;
using Thrift.Transport;

namespace Apache.Arrow.Adbc.Drivers.Apache.Hive2
{
public abstract class HiveServer2Connection : AdbcConnection
internal abstract class HiveServer2Connection : MockingConnection<TCLIService.IAsync>
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only SparkDriver needs to be public

@@ -22,7 +22,7 @@

namespace Apache.Arrow.Adbc.Drivers.Apache.Hive2
{
public abstract class HiveServer2Statement : AdbcStatement
internal abstract class HiveServer2Statement : AdbcStatement
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only SparkDriver needs to be public

@@ -28,7 +28,7 @@

namespace Apache.Arrow.Adbc.Drivers.Apache.Impala
{
public class ImpalaStatement : HiveServer2Statement
internal class ImpalaStatement : HiveServer2Statement
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only ImpalaDriver needs to be public

@@ -26,7 +26,7 @@

namespace Apache.Arrow.Adbc.Drivers.Apache.Impala
{
public class ImpalaConnection : HiveServer2Connection
internal class ImpalaConnection : HiveServer2Connection
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only ImpalaDriver needs to be public

connection.OpenAsync().Wait();
return connection;
}

private static IReadOnlyDictionary<TKey, TValue> MergeDictionaries<TKey, TValue>(params IReadOnlyDictionary<TKey, TValue>?[] dictionaries)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazingly, this isn't part of .NET to merge two or more dictionaries in the case there are duplicates.

/// <param name="connectionOptions">A dictionary of connection options.</param>
/// <param name="mock">An optional mocker server proxy implementation.</param>
/// <returns></returns>
protected AdbcConnection NewConnection(TConfig? testConfiguration = default, IReadOnlyDictionary<string, string>? connectionOptions = default, MockDataSourceBase<TMock>? mock = default)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adds a new override for a NewConnection where you can pass a mock


public Task<TGetTablesResp> GetTables(TGetTablesReq req, CancellationToken cancellationToken = default) => throw new NotImplementedException();

public Task<TGetTableTypesResp> GetTableTypes(TGetTableTypesReq req, CancellationToken cancellationToken = default)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a POC where it is "hard-coded" response to the GetTablesTypes call.

The intention is to have a generic caching "replay" mechanism.


public Task<TGetTypeInfoResp> GetTypeInfo(TGetTypeInfoReq req, CancellationToken cancellationToken = default) => throw new NotImplementedException();

public Task<TOpenSessionResp> OpenSession(TOpenSessionReq req, CancellationToken cancellationToken = default)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation should be fine to mock the OpenSsession. In fact, a recording of the values may not be desirable.

this.transport = protocol.Transport;
this.client = new TCLIService.Client(protocol);

this.client = DataSourceDriverProxy ?? await NewDataSourceDriverAsync();
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the proxy if it exists.

var s0 = await this.client.OpenSession(CreateSessionRequest());
this.sessionHandle = s0.SessionHandle;
}

internal async override Task<TCLIService.IAsync> NewDataSourceDriverAsync()
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make a factory for the actual connection available to the mock.

@@ -121,7 +125,7 @@ public override void Dispose()
this.client.CloseSession(r6).Wait();

this.transport?.Close();
this.client.Dispose();
if (this.client is IDisposable disposable) disposable.Dispose();
Copy link
Owner Author

@birschick-bq birschick-bq Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to be more generic here because we're not sure if it implements IDisposable.

birschick-bq added a commit that referenced this pull request Aug 12, 2024
birschick-bq added a commit that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant