lidavidm commented on code in PR #697: URL: https://github.com/apache/arrow-adbc/pull/697#discussion_r1202175572
########## csharp/src/Apache.Arrow.Adbc/Core/AdbcStatement.cs: ########## @@ -0,0 +1,276 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +using System; +using System.Linq; +using System.Text.RegularExpressions; +using System.Threading.Tasks; +using Apache.Arrow.Ipc; +using Apache.Arrow.Types; + +namespace Apache.Arrow.Adbc.Core +{ + /// <summary> + /// Statements may represent queries or prepared statements. Statements may be used multiple times and can be reconfigured (e.g. they can be reused to execute multiple different queries). + /// </summary> + public abstract class AdbcStatement : IDisposable + { + public AdbcStatement() + { + Timeout = 30; + } + + /// <summary> + /// Gets or sets a SQL query to be executed on this statement. + /// </summary> + public virtual string SqlQuery { get; set; } + + public virtual byte[] SubstraitPlan + { + get { throw new NotImplementedException(); } + set { throw new NotImplementedException(); } + } + + public virtual void Bind() + { + throw new NotImplementedException(); + } + + /// <summary> + /// Executes the statement and returns a tuple containing the number of records and the <see cref="IArrowArrayStream"/>.. + /// </summary> + /// <returns>A <see cref="ValueTuple"/> where the first item is the number of records and the second is the <see cref="IArrowArrayStream"/>.</returns> + public abstract QueryResult ExecuteQuery(); + + /// <summary> + /// Executes the statement and returns a tuple containing the number of records and the <see cref="IArrowArrayStream"/>.. + /// </summary> + /// <returns>A <see cref="ValueTuple"/> where the first item is the number of records and the second is the <see cref="IArrowArrayStream"/>.</returns> + public virtual async ValueTask<QueryResult> ExecuteQueryAsync() Review Comment: Is it normal to have sync and async variants of methods in the same class? Or would they be separate interfaces? ########## csharp/src/Apache.Arrow.Adbc/Core/AdbcStatement.cs: ########## @@ -0,0 +1,276 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +using System; +using System.Linq; +using System.Text.RegularExpressions; +using System.Threading.Tasks; +using Apache.Arrow.Ipc; +using Apache.Arrow.Types; + +namespace Apache.Arrow.Adbc.Core +{ + /// <summary> + /// Statements may represent queries or prepared statements. Statements may be used multiple times and can be reconfigured (e.g. they can be reused to execute multiple different queries). + /// </summary> + public abstract class AdbcStatement : IDisposable + { + public AdbcStatement() + { + Timeout = 30; + } + + /// <summary> + /// Gets or sets a SQL query to be executed on this statement. + /// </summary> + public virtual string SqlQuery { get; set; } + + public virtual byte[] SubstraitPlan + { + get { throw new NotImplementedException(); } + set { throw new NotImplementedException(); } + } + + public virtual void Bind() + { + throw new NotImplementedException(); + } + + /// <summary> + /// Executes the statement and returns a tuple containing the number of records and the <see cref="IArrowArrayStream"/>.. + /// </summary> + /// <returns>A <see cref="ValueTuple"/> where the first item is the number of records and the second is the <see cref="IArrowArrayStream"/>.</returns> + public abstract QueryResult ExecuteQuery(); + + /// <summary> + /// Executes the statement and returns a tuple containing the number of records and the <see cref="IArrowArrayStream"/>.. + /// </summary> + /// <returns>A <see cref="ValueTuple"/> where the first item is the number of records and the second is the <see cref="IArrowArrayStream"/>.</returns> + public virtual async ValueTask<QueryResult> ExecuteQueryAsync() + { + return await Task.Run(() => ExecuteQuery()); + } + + /// <summary> + /// Executes an update command and returns the number of records effected. + /// </summary> + /// <returns></returns> + /// <exception cref="NotImplementedException"></exception> + public abstract UpdateResult ExecuteUpdate(); + + // <summary> + /// Executes an update command and returns the number of records effected. + /// </summary> + /// <returns></returns> + /// <exception cref="NotImplementedException"></exception> + public virtual async Task<UpdateResult> ExecuteUpdateAsync() + { + return await Task.Run(() => ExecuteUpdate()); + } + + /// <summary> + /// Timeout (in seconds) for statement execution. + /// </summary> + /// <remarks>The default is 30 seconds.</remarks> + public virtual int Timeout { get; set; } + + /// <summary> + /// Execute a result set-generating query and get a list of partitions of the result set. + /// </summary> + /// <returns><see cref="PartitionedResult"/></returns> + public virtual PartitionedResult ExecutePartitioned() + { + throw AdbcException.NotImplemented("Statement does not support executePartitioned"); + } + + public virtual Schema GetParameterSchema() + { + throw AdbcException.NotImplemented("Statement does not support GetParameterSchema"); + } + + public virtual void Prepare() + { + throw AdbcException.NotImplemented("Statement does not support Prepare"); + } + + public virtual void Dispose() + { + } + + /// <summary> + /// Gets the .NET type based on the Arrow field metadata + /// </summary> + /// <param name="f"></param> + /// <returns></returns> + public virtual Type ConvertArrowType(Field f) Review Comment: It seems the below helpers should go somewhere else? Possibly even upstream into the Arrow library itself? ########## csharp/src/Apache.Arrow.Adbc.FlightSql/FlightSqlParameters.cs: ########## @@ -0,0 +1,30 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +namespace Apache.Arrow.Adbc.FlightSql +{ + /// <summary> + /// Parameters used for connecting to Flight SQL data sources. + /// </summary> + public class FlightSqlParameters + { + public const string ServerAddress = "FLIGHT_SQL_SERVER_ADRESS"; Review Comment: ```suggestion public const string ServerAddress = "FLIGHT_SQL_SERVER_ADDRESS"; ``` ########## csharp/test/Apache.Arrow.Adbc.FlightSql.Tests/DriverConnectionTests.cs: ########## @@ -0,0 +1,81 @@ +using System.Collections.Generic; +using System.IO; +using Apache.Arrow.Adbc.Core; +using Apache.Arrow.Adbc.FlightSql; +using Apache.Arrow.Adbc.FlightSql.Tests; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Newtonsoft.Json; + +namespace Apache.Arrow.Adbc.Tests +{ + [TestClass] + public class FlightSqlDriverConnectionTests : DriverConnectionTests + { + [TestMethod] + public override void CanDriverConnect() + { + FlightSqlTestConfiguration flightSqlTestConfiguration = GetFlightSqlTestConfiguration(); + + Dictionary<string, string> parameters = new Dictionary<string, string> + { + { FlightSqlParameters.ServerAddress, flightSqlTestConfiguration.ServerAddress }, + { FlightSqlParameters.RoutingTag, flightSqlTestConfiguration.RoutingTag }, + { FlightSqlParameters.RoutingQueue, flightSqlTestConfiguration.RoutingQueue }, + { FlightSqlParameters.Authorization, flightSqlTestConfiguration.Authorization} + }; + + Dictionary<string, string> options = new Dictionary<string, string>() + { + { FlightSqlParameters.ServerAddress, flightSqlTestConfiguration.ServerAddress }, + }; + + FlightSqlDriver flightSqlDriver = new FlightSqlDriver(); + FlightSqlDatabase flightSqlDatabase = flightSqlDriver.Open(parameters) as FlightSqlDatabase; + FlightSqlConnection connection = flightSqlDatabase.Connect(options) as FlightSqlConnection; + FlightSqlStatement statement = connection.CreateStatement() as FlightSqlStatement; + + statement.SqlQuery = flightSqlTestConfiguration.Query; + QueryResult queryResult = statement.ExecuteQuery(); + + long count = 0; + + while (true) + { + var nextBatch = queryResult.Stream.ReadNextRecordBatchAsync().Result; + if (nextBatch == null) { break; } + count += nextBatch.Length; + } + + Assert.AreEqual(flightSqlTestConfiguration.ExpectedResultsCount, count); + } + + public override void CanDriverUpdate() + { + throw new System.NotImplementedException(); + } + + public override void CanReadSchema() + { + throw new System.NotImplementedException(); + } + + public override void VerifyBadQueryGeneratesError() + { + throw new System.NotImplementedException(); + } + + public override void VerifyTypesAndValues() + { + throw new System.NotImplementedException(); + } + + private FlightSqlTestConfiguration GetFlightSqlTestConfiguration() + { + string json = File.ReadAllText("flightsqlconfig.pass"); Review Comment: I see this is explicitly excluded; it seems we need it to actually run tests though? ########## csharp/src/Apache.Arrow.Adbc/Extensions/MarshalExtensions.netstandard.cs: ########## @@ -0,0 +1,74 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#if NETSTANDARD +using System; +using System.Collections.Generic; +using System.Runtime.InteropServices; +using System.Text; + +namespace Apache.Arrow.Adbc.Extensions +{ + public static class MarshalExtensions + { + public static unsafe string PtrToStringUTF8(IntPtr intPtr) + { + if (intPtr == null) // IsNullOrWin32Atom(intPtr)) Review Comment: What's the intent of the commented code? ########## csharp/src/Apache.Arrow.Adbc/Extensions/MarshalExtensions.netstandard.cs: ########## @@ -0,0 +1,74 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#if NETSTANDARD +using System; +using System.Collections.Generic; +using System.Runtime.InteropServices; +using System.Text; + +namespace Apache.Arrow.Adbc.Extensions +{ + public static class MarshalExtensions + { + public static unsafe string PtrToStringUTF8(IntPtr intPtr) + { + if (intPtr == null) // IsNullOrWin32Atom(intPtr)) Review Comment: Ah, are these methods backports from newer .NET versions? ########## csharp/src/Apache.Arrow.Adbc/Extensions/MatchCollectionExtensions.netstandard.cs: ########## @@ -0,0 +1,23 @@ +#if NETSTANDARD +using System.Collections.Generic; +using System.Linq; +using System.Text.RegularExpressions; + +namespace System.Text.RegularExpressions +{ + public static class MatchCollectionExtensions + { + public static IEnumerable<Match> Where(this MatchCollection matchCollection, Func<Match,bool> predicate) Review Comment: Is it usual for a library to add its own extension methods to standard library types? ########## csharp/test/Apache.Arrow.Adbc.Tests/DriverConnectionTests.cs: ########## @@ -0,0 +1,35 @@ +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace Apache.Arrow.Adbc.Tests +{ + /// <summary> + /// Abstract class for the ADBC connection tests. + /// </summary> + public abstract class DriverConnectionTests Review Comment: The Java/Go/C++ implementations have a generic test suite written in terms of the public API only along with hooks for individual drivers to inject connection options, etc. Without that I'm not sure there's value in purely a common interface for tests ########## csharp/src/Apache.Arrow.Adbc/Extensions/MatchCollectionExtensions.netstandard.cs: ########## @@ -0,0 +1,23 @@ +#if NETSTANDARD +using System.Collections.Generic; +using System.Linq; +using System.Text.RegularExpressions; + +namespace System.Text.RegularExpressions +{ + public static class MatchCollectionExtensions + { + public static IEnumerable<Match> Where(this MatchCollection matchCollection, Func<Match,bool> predicate) + { + List<Match> matches = new List<Match>(); + + foreach (Match match in matchCollection) + { + matches.Add(match); + } + + return matches.Where(predicate); Review Comment: Could you avoid Where and just apply the predicate yourself in the loop? ########## csharp/src/Apache.Arrow.Adbc/Core/AdbcStatement.cs: ########## @@ -0,0 +1,276 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +using System; +using System.Linq; +using System.Text.RegularExpressions; +using System.Threading.Tasks; +using Apache.Arrow.Ipc; +using Apache.Arrow.Types; + +namespace Apache.Arrow.Adbc.Core +{ + /// <summary> + /// Statements may represent queries or prepared statements. Statements may be used multiple times and can be reconfigured (e.g. they can be reused to execute multiple different queries). + /// </summary> + public abstract class AdbcStatement : IDisposable + { + public AdbcStatement() + { + Timeout = 30; + } + + /// <summary> + /// Gets or sets a SQL query to be executed on this statement. + /// </summary> + public virtual string SqlQuery { get; set; } + + public virtual byte[] SubstraitPlan + { + get { throw new NotImplementedException(); } + set { throw new NotImplementedException(); } + } + + public virtual void Bind() Review Comment: This is missing the actual data to bind? ########## csharp/src/Apache.Arrow.Adbc.FlightSql/FlightSqlStatement.cs: ########## @@ -0,0 +1,191 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +using System; +using System.Threading.Tasks; +using Apache.Arrow.Adbc.Core; +using Apache.Arrow.Flight; +using Grpc.Core; + +namespace Apache.Arrow.Adbc.FlightSql +{ + /// <summary> + /// A Flight SQL implementation of <see cref="AdbcStatement"/>. + /// </summary> + public class FlightSqlStatement : AdbcStatement + { + private FlightSqlConnection flightSqlConnection; + + public FlightSqlStatement(FlightSqlConnection flightSqlConnection) + { + this.flightSqlConnection = flightSqlConnection; + } + + public override async ValueTask<QueryResult> ExecuteQueryAsync() + { + FlightInfo info = await GetInfo(this.SqlQuery, this.flightSqlConnection.Metadata); + + return new QueryResult(info.TotalRecords, new FlightSqlResult(this.flightSqlConnection, info)); + } + + public override QueryResult ExecuteQuery() + { + return ExecuteQueryAsync().Result; + } + + public override UpdateResult ExecuteUpdate() + { + throw new NotImplementedException(); + } + + public async ValueTask<FlightInfo> GetInfo(string query, Metadata headers) + { + FlightDescriptor commandDescripter = FlightDescriptor.CreateCommandDescriptor(query); + + return await this.flightSqlConnection.FlightClient.GetInfo(commandDescripter, headers).ResponseAsync; + } + + /// <summary> + /// Gets a value from the Arrow array at the specified index using the Arrow field for metadata. + /// </summary> + /// <param name="arrowArray"></param> + /// <param name="field"></param> + /// <param name="index"></param> + /// <returns></returns> + public override object GetValue(IArrowArray arrowArray, Field field, int index) Review Comment: Same here - this seems like it belongs somewhere else? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
