You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/11/17 15:43:51 UTC
[GitHub] [arrow] Ulimo opened a new pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Ulimo opened a new pull request #8694:
URL: https://github.com/apache/arrow/pull/8694
This adds basic support for both a flight server and a flight client for Net Core.
I hope that this PR can help create some discussion on how the interfaces should look etc, and if this looks like an accetable interface for flight in net core.
This implementation uses InternalsVisibleTo, to hinder a bump of .netstandard version from 1.3 to 1.5.
So the Apache.Arrow project still uses netstandard1.3.
All flight code is in a seperate project Apache.Arrow.Flight
This also required changing build version from 2.2 to 3.0
The code does not include:
* Handshake - the reason is that AspNetCore contains features already for authentication/authorization for gRPC. Can be added later ofcourse.
* DoExchange - I feel that more feedback/discussion is required before DoExchange can be implemented.
Note:
Sourcelink did not work when using grpc.tools to have code compilation in the build step. So I had to generate the grpc code manually for sourcelink to work. This means that there are alot of extra code in this PR that are auto generated.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r529362242
##########
File path: csharp/src/Apache.Arrow.Flight/Client/FlightClientRecordBatchStreamWriter.cs
##########
@@ -0,0 +1,52 @@
+// 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.Collections.Generic;
+using System.Text;
+using System.Threading.Tasks;
+using Apache.Arrow.Flight.Protocol;
+using Apache.Arrow.Flight.Internal;
+using Grpc.Core;
+
+namespace Apache.Arrow.Flight.Client
+{
+ public class FlightClientRecordBatchStreamWriter : FlightRecordBatchStreamWriter, IClientStreamWriter<RecordBatch>
+ {
+ private readonly IClientStreamWriter<FlightData> _clientStreamWriter;
+ private bool _completed = false;
+ internal FlightClientRecordBatchStreamWriter(IClientStreamWriter<FlightData> clientStreamWriter, FlightDescriptor flightDescriptor) : base(clientStreamWriter, flightDescriptor)
+ {
+ _clientStreamWriter = clientStreamWriter;
+ }
+
+ protected override void Dispose(bool disposing)
+ {
+ CompleteAsync().Wait();
Review comment:
Added a throw
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r527932397
##########
File path: csharp/src/Apache.Arrow.Flight/Action.cs
##########
@@ -0,0 +1,71 @@
+// 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.Collections.Generic;
+using System.Text;
+using Google.Protobuf;
+
+namespace Apache.Arrow.Flight
+{
+ public class Action
Review comment:
This is kind of an unfortunate name since it conflicts with `System.Action`. Any time someone tries using this class (or System.Action) and they have both namespaces in their `usings`, they will get:
```
Severity Code Description Project File Line Suppression State
Error CS0104 'Action' is an ambiguous reference between 'Apache.Arrow.Flight.Action' and 'System.Action' ArrowFlightTest F:\LinuxShare\ArrowFlight\Program.cs 15 Active
```
What do you think about calling this `FlightAction` to help disambiguate?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525558034
##########
File path: csharp/src/Apache.Arrow.Flight/Generated/Flight.cs
##########
@@ -0,0 +1,3536 @@
+// <auto-generated>
Review comment:
That did the trick, thank you! I removed the generated files and added Grpc.Tools instead.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] JamesNK commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
JamesNK commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525552375
##########
File path: csharp/src/Apache.Arrow.Flight/Generated/Flight.cs
##########
@@ -0,0 +1,3536 @@
+// <auto-generated>
Review comment:
I think `<EmbedUntrackedSources>true</EmbedUntrackedSources>` will fix 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r527927449
##########
File path: csharp/src/Apache.Arrow.Flight/FlightClient.cs
##########
@@ -0,0 +1,90 @@
+// 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.Collections.Generic;
+using System.Threading.Tasks;
+using Apache.Arrow.Flight.Protocol;
+using Apache.Arrow.Flight.Writer;
+using Grpc.Core;
+using Grpc.Net.Client;
+
+namespace Apache.Arrow.Flight
+{
+ public class FlightClient
+ {
+ private readonly FlightService.FlightServiceClient _client;
+ public FlightClient(GrpcChannel grpcChannel)
+ {
+ _client = new FlightService.FlightServiceClient(grpcChannel);
+ }
+
+ public IAsyncStreamReader<FlightInfo> ListFlights(Criteria criteria = null, Metadata headers = null)
+ {
+ if(criteria == null)
+ {
+ criteria = new Criteria();
+ }
+
+ var response = _client.ListFlights(criteria.ToProtocol(), headers);
+ return new StreamReader<Protocol.FlightInfo, FlightInfo>(response.ResponseStream, inFlight => new FlightInfo(inFlight));
+ }
+
+ public IAsyncStreamReader<ActionType> ListActions(Metadata headers = null)
+ {
+ var response = _client.ListActions(new Empty(), headers);
Review comment:
Similar comment for `new Empty()`. Instead make an `internal static readonly Empty Instance = new Empty()` field and use it 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] JamesNK commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
JamesNK commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525540207
##########
File path: csharp/src/Apache.Arrow.Flight/Apache.Arrow.Flight.csproj
##########
@@ -0,0 +1,18 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+ <PropertyGroup>
+ <TargetFramework>netstandard2.1</TargetFramework>
+ <LangVersion>8.0</LangVersion>
+ </PropertyGroup>
+
+ <ItemGroup>
+ <PackageReference Include="Google.Protobuf" Version="3.14.0" />
+ <PackageReference Include="Grpc" Version="2.33.1" />
Review comment:
Only need the tooling, and only at build.
##########
File path: csharp/src/Apache.Arrow.Flight/Apache.Arrow.Flight.csproj
##########
@@ -0,0 +1,18 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+ <PropertyGroup>
+ <TargetFramework>netstandard2.1</TargetFramework>
+ <LangVersion>8.0</LangVersion>
+ </PropertyGroup>
+
+ <ItemGroup>
+ <PackageReference Include="Google.Protobuf" Version="3.14.0" />
+ <PackageReference Include="Grpc" Version="2.33.1" />
Review comment:
```suggestion
<PackageReference Include="Grpc.Tools" Version="2.33.1" PrivateAssets="All" />
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt edited a comment on pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt edited a comment on pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#issuecomment-731202315
UPDATE: I resolved the below issue with this answer: https://stackoverflow.com/a/58053460/3680432
____
@Ulimo - how much of this is expected to work end-to-end? I've been trying to use it against an example server I found in the repo: https://github.com/apache/arrow/blob/master/python/examples/flight/server.py.
However, I am not able to successfully call anything against the server. Here is my test program:
```C#
static async Task Main(string[] args)
{
GrpcChannel channel = GrpcChannel.ForAddress("http://localhost:5005");
FlightClient flightClient = new FlightClient(channel);
var actions = flightClient.ListActions();
while (await actions.MoveNext(default))
{
Console.WriteLine(actions.Current);
}
System.Console.WriteLine("Done");
}
```
And I get this exception:
```
Unhandled exception. Grpc.Core.RpcException: Status(StatusCode="Internal", Detail="Error starting gRPC call. HttpRequestException: An error occurred while sending the request. IOException: The response ended prematurely.", DebugException="System.Net.Http.HttpRequestException: An error occurred while sending the request.
---> System.IO.IOException: The response ended prematurely.
at System.Net.Http.HttpConnection.FillAsync()
at System.Net.Http.HttpConnection.ReadNextResponseHeaderLineAsync(Boolean foldedHeadersAllowed)
at System.Net.Http.HttpConnection.SendAsyncCore(HttpRequestMessage request, CancellationToken cancellationToken)
--- End of inner exception stack trace ---
at System.Net.Http.HttpConnection.SendAsyncCore(HttpRequestMessage request, CancellationToken cancellationToken)
at System.Net.Http.HttpConnectionPool.SendWithNtConnectionAuthAsync(HttpConnection connection, HttpRequestMessage request, Boolean doRequestAuth, CancellationToken cancellationToken)
at System.Net.Http.HttpConnectionPool.SendWithRetryAsync(HttpRequestMessage request, Boolean doRequestAuth, CancellationToken cancellationToken)
at System.Net.Http.RedirectHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
at Grpc.Net.Client.Internal.GrpcCall`2.RunCall(HttpRequestMessage request, Nullable`1 timeout)")
at Grpc.Net.Client.Internal.HttpContentClientStreamReader`2.MoveNextCore(CancellationToken cancellationToken)
at Apache.Arrow.Flight.StreamReader`2.MoveNext(CancellationToken cancellationToken) in /Users/eerhardt/git/arrow/csharp/src/Apache.Arrow.Flight/StreamReader.cs:line 46
at ArrowFlightTest.Program.Main(String[] args) in /Users/eerhardt/DotNetTest/ArrowFlightTest/Program.cs:line 17
at ArrowFlightTest.Program.<Main>(String[] args)
```
One part that confuses me is the example server says the address is `grpc+tcp://localhost:5005`, but the .NET gRPC client throws with that address saying only `http` and `https` schemes are supported. So that's why I used `http://localhost:5005`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r528265410
##########
File path: csharp/src/Apache.Arrow.Flight/Apache.Arrow.Flight.csproj
##########
@@ -0,0 +1,21 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+ <PropertyGroup>
+ <TargetFramework>netstandard2.1</TargetFramework>
+ </PropertyGroup>
+
+ <ItemGroup>
+ <PackageReference Include="Google.Protobuf" Version="3.14.0" />
+ <PackageReference Include="Grpc.Net.Client" Version="2.33.1" />
+ <PackageReference Include="Grpc.Tools" Version="2.33.1" PrivateAssets="All" />
+ </ItemGroup>
+
+ <ItemGroup>
+ <ProjectReference Include="..\Apache.Arrow\Apache.Arrow.csproj" />
+ </ItemGroup>
+
+ <ItemGroup>
+ <Protobuf Include="..\..\..\format\Flight.proto" />
Review comment:
All the protocol classes are now internal.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525553034
##########
File path: csharp/src/Apache.Arrow.Flight/Generated/Flight.cs
##########
@@ -0,0 +1,3536 @@
+// <auto-generated>
Review comment:
Ok, I will give it a shot directly, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#issuecomment-731254431
@eerhardt I tested it against that solution myself, I will check why its not working.
I would love to have a docker container of the java application and use testcontainers so we have unit tests that we are compliant with the java stuff. But that would require adding some more in the build process itself to build a container of the java stuff. Maybe in a future PR?
the dot net grpc stuff only accepts http/https thats true I think a utility class or something to change from grpc+tcp to http/https is required. I also hoped that could come in a future PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#issuecomment-735020461
I believe the next round of releases is scheduled for sometime in January. (See [this recent dev-list thread](https://lists.apache.org/thread.html/r913292d53736affaff5933c1416fedbb193c4ff68ced4bcf1d02e7ec%40%3Cdev.arrow.apache.org%3E))
Would you need something earlier than that? If so, one option would be to build the nuget package yourself (`dotnet pack` in the csharp folder), and push it to a private feed you can use until an official one get pushed to nuget.org.
Potential options for private feeds are Azure DevOps or myget.org.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] JamesNK commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
JamesNK commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525543295
##########
File path: csharp/src/Apache.Arrow.Flight/Generated/Flight.cs
##########
@@ -0,0 +1,3536 @@
+// <auto-generated>
Review comment:
Why are you checking generated code in? You should be able to reference Grpc.Tools and generate this file automatically from the proto file.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] JamesNK commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
JamesNK commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525540754
##########
File path: csharp/src/Apache.Arrow.Flight/Apache.Arrow.Flight.csproj
##########
@@ -0,0 +1,18 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+ <PropertyGroup>
+ <TargetFramework>netstandard2.1</TargetFramework>
+ <LangVersion>8.0</LangVersion>
+ </PropertyGroup>
+
+ <ItemGroup>
+ <PackageReference Include="Google.Protobuf" Version="3.14.0" />
+ <PackageReference Include="Grpc" Version="2.33.1" />
Review comment:
Actually, `Grpc` doesn't bring in `Grpc.Tools`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#issuecomment-731658726
@eerhardt first of all thank you so much for your support and feedback and sorry for this wall of text. Based on your last feedback that the API is hard to change etc in the future, I really took a long sitting to go through all the classes etc, and found some issues that I really needed to address.
This resulted in a small overhaul on the structure etc. I also went through so the gRPC schema information is actually exposed to the user as well. This also resulted in some rewrites. The feedback you have given has been exactly what I needed to come to the conclusions etc on structure, so I am once again extremely thankful for all the feedback you have given.
This ofcourse increased the scope of the PR a bit again, but I hope it helps give a more real look and feel on how the framework will behave, and if the implemented design actually works.
Here comes information on the work that has been done, and also some checklists to see that all the required information is exposed etc, and why some classes are public.
# Major changes
* All exposed classes start with prefix "Flight".
* New project Apache.Arrow.Flight.AspNetCore, required since generated code is internal, this project references grpc.aspnetcore.server which is netcoreapp3.1 only, and contains extensions only to add flight support for aspnetcore projects.
* All flight client methods now follows gRPC API more closely, allows reading response headers for all calls.
* Exposing application metadata required rewriting a bit of the logic in client and servers, read more in its chapter.
* Moved client specific classes under .Client namespace.
* Moved server specific classes under .Server namespace.
* Internal classes are now under the .Internal namespace.
* Only mutual used classes are now in the .Flight namespace.
# Minor changes
* Removed multi property exposure of bytestring, to only bytestring.
* Exposed TotalBytes and TotalRecords in FlightInfo.
* Moved public abstract reader, writer classes to internal namespace since they cant be used by the user.
* Added FlightServerRecordBatchStreamReader to expose FlightDescriptor only to the server since the client wont use it.
* Added FlightClientRecordBatchStreamReader which just extends FlightRecordBatchStreamReader which is now abstract.
* StreamWriter made internal
* PutResult does not expose ArrowBuffer (was taken from java project), instead it exposes bytestring like the other classes.
This was done so the user can choose what type of metadata to return, and also follow more closely the other classes.
* Added public static Empty on PutResult, can be useful for implementations when metadata is not used, and empty put result should be returned.
* Added a new test case to get metadata.
* Added a new test case to put metadata.
* Added a new test case for get schema.
* Added a new test case for DoAction.
* Added a new test case for ListFlights.
Test coverage is now above 80% on the non generated code.
# Application metadata addition
To enable users to write and read application metadata, the class FlightRecordBatchStreamingCall was added that
still follow gRPC look and feel, but exposes FlightRecordBatchStreamReader as response stream instead where the user can
get application metadata.
Application metadata can be read with each record batch, it is implemented as a list since hypothetically metadata can
be sent with the schema aswell. Since the packages sent follow this pattern at the moment:
Schema -> record batch -> record batch -> ... -> done
So the first message could contains metadata as well. When a user gets a new record batch the metadata is cleared similar
to how the java implementation is done.
For writing, this is done by an additional method in FlightRecordBatchStreamWriter which looks like this:
```
public Task WriteAsync(RecordBatch message, ByteString applicationMetadata);
```
# gRPC support
Here is a check on what gRPC methods and properties are exposed and can be read/used by a user of the framework, mostly to check that everything that is required to be exposed are exposed at this time.
## gRPC methods exposed or not exposed to the user
* ListFlights - Exposed
* GetFlightInfo - Exposed
* GetSchema - Exposed
* DoGet - Exposed
* DoPut - Exposed
* DoAction - Exposed
* ListActions -Exposed
* **Handshake - Not exposed**
* **DoExchange - Not exposed**
## gRPC properties exposed or not exposed to the user
### HandshakeRequest
**Handshake is not exposed at all**
* **protocol_version - not exposed**
* **payload - not exposed**
### HandshakeResponse
**Handshake is not exposed at all**
* **protocol_version - not exposed**
* **payload - not exposed**
### BasicAuth
**Basic auth is not exposed at all**
* **username - not exposed**
* **password - not exposed**
### ActionType
* Type - exposed
* Description - exposed
### Criteria
* Expression - exposed
### Action
* Type - exposed
* Body - exposed
### Result
* Body - exposed
### SchemaResult
*Schema result is not mapped to its own type, but returns a schema directly*
* Schema - exposed
### FlightDescriptor
* Type - exposed
* Path - exposed
* Cmd - exposed
### FlightInfo
* Schema - exposed
* FlightDescriptor - exposed
* endpoints - exposed
* TotalRecords - exposed
* TotalBytes - exposed
### FlightEndpoint
* Ticket - exposed
* Locations - exposed
### Location
* Uri - exposed
### Ticket
* Ticket - exposed
### FlightData
Flight data is not exposed as a class, but the data is exposed in getStream, startPut for client, and DoGet and DoPut for server.
* **FlightDescriptor**
* **client can only send flight descriptor for flight data**
* **server can only read for flight data**
* Desc: *The descriptor of the data. This is only relevant when a client is starting a new DoPut stream.*
* **Data header - used to read header of message, ex: schema, record batch message etc, not explicitly exposed and used internally only**
* Application Metadata
* Client can write metadata through *ClientRecordBatchStreamWriter*
* Client can read metadata through *FlightRecordBatchStreamReader*
* Server can write metadata through FlightServerRecordBatchStreamWriter
* Server can read metadata through *FlightRecordBatchStreamReader*
* **Data body - same as data header, exposure is done through RecordBatch**
### PutResult
* Application metadata - exposed
# Classes public or internal
Here is a list of classes and if they are public or internal, and motivation on why they are public.
* FlightRecordBatchStreamingCall - public, required since it exposes FlightRecordBatchStreamReader which is required to read metadata.
* FlightRecordBatchStreamReader - public, required to allow read on schema, flight descriptor, application metadata
* RecordBatcReaderImplementation - internal
* FlightClientRecordBatchStreamWriter - public, implements CompleteAsync for clients, extends FlightRecordBatchStreamWriter wich allows user to write application metadata.
* FlightDataStream - internal
* FlightRecordBatchDuplexStreamingCall - public, exposes FlightClientRecordBatchStreamWriter which is required to write application metadata.
* FlightRecordBatchStreamWriter - public abstract with private protected constructor, client and server implementations extend this one.
* FlightServerRecordBatchStreamWriter - public, extends FlightRecordBatchStreamWriter which allows user to write application metadata.
* SchemaWriter - internal
* FlightAction - public, allows user to read/write type, and body in Actions.
* FlightActionType - public, allows user to read/write action types with type and description from ListActions.
* FlightClient - public, allows user to call all the different endpoints.
* FlightCriteria - public, exposes Expression that servers can implement to filter result from ListFlights.
* FlightDescriptor - public, exposes DescriptorType, Paths and Cmd for user.
* FlightDescriptorType - public, contains descriptor types.
* FlightEndpoint - public, exposes flight ticket and flight locations.
* FlightInfo - public, exposes flight descriptor, schema, total bytes, total records and endpoints.
* FlightLocation - public, exposes uri to the user.
* FlightMessageSerializer - internal
* FlightPutResult - public, recieved when doing doPut, exposes application metadata.
* FlightResult - public, contains body from DoAction calls.
* FlightServerImplementation - internal, forced internal from putting generated code to internal.
* FlightTicket - public, exposes the bytestring from the ticket to the user.
* IFlightServer - public, interface to implement a flight server.
* StreamReader - internal
* StreamWriter - internal
* FlightIEndpointRouteBuilderExtensions - public, allows user to map the flight endpoint in asp net core.
* FlightIGrpcServerBuilderExtensions - public, allows the user to add their implementation of IFlightServer in a nicer way.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525544709
##########
File path: csharp/src/Apache.Arrow.Flight/Generated/Flight.cs
##########
@@ -0,0 +1,3536 @@
+// <auto-generated>
Review comment:
I had problems in the build process, with sourcelink, I wrote it in the description, sourcelink could not find the auto generated code.
If you know a fix I would greatly appreciate it :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525574841
##########
File path: csharp/test/Apache.Arrow.Flight.TestWeb/Apache.Arrow.Flight.TestWeb.csproj
##########
@@ -0,0 +1,16 @@
+<Project Sdk="Microsoft.NET.Sdk.Web">
+
+ <PropertyGroup>
+ <TargetFramework>netcoreapp3.0</TargetFramework>
+ </PropertyGroup>
+
+ <ItemGroup>
+ <PackageReference Include="Grpc" Version="2.33.1" />
Review comment:
This is just for the tests where I use some of the extension methods, such as ToListAsync() etc, Is it considered bad practice to use it even in those cases?
Edit: my bad, sorry, removing it
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] JamesNK commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
JamesNK commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525540050
##########
File path: csharp/src/Apache.Arrow.Flight/Apache.Arrow.Flight.csproj
##########
@@ -0,0 +1,18 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+ <PropertyGroup>
+ <TargetFramework>netstandard2.1</TargetFramework>
+ <LangVersion>8.0</LangVersion>
+ </PropertyGroup>
+
+ <ItemGroup>
+ <PackageReference Include="Google.Protobuf" Version="3.14.0" />
+ <PackageReference Include="Grpc" Version="2.33.1" />
Review comment:
```suggestion
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r528208964
##########
File path: csharp/src/Apache.Arrow.Flight/Action.cs
##########
@@ -0,0 +1,71 @@
+// 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.Collections.Generic;
+using System.Text;
+using Google.Protobuf;
+
+namespace Apache.Arrow.Flight
+{
+ public class Action
Review comment:
@eerhardt I think I will actually prefix all names by Flight if that sounds ok to you as well. A lot of the names are quite generic such as Action, Location, Result, Criteria. There mightbe conflicts with other libraries for the other names as well. So I were thinking that having the same prefix gives it some kind of uniformity.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r527926764
##########
File path: csharp/src/Apache.Arrow.Flight/FlightClient.cs
##########
@@ -0,0 +1,90 @@
+// 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.Collections.Generic;
+using System.Threading.Tasks;
+using Apache.Arrow.Flight.Protocol;
+using Apache.Arrow.Flight.Writer;
+using Grpc.Core;
+using Grpc.Net.Client;
+
+namespace Apache.Arrow.Flight
+{
+ public class FlightClient
+ {
+ private readonly FlightService.FlightServiceClient _client;
+ public FlightClient(GrpcChannel grpcChannel)
+ {
+ _client = new FlightService.FlightServiceClient(grpcChannel);
+ }
+
+ public IAsyncStreamReader<FlightInfo> ListFlights(Criteria criteria = null, Metadata headers = null)
+ {
+ if(criteria == null)
+ {
+ criteria = new Criteria();
Review comment:
We could save on some allocations here by doing something like:
```C#
public class Criteria
{
internal static readonly Criteria Empty = new Criteria();
```
and then here instead of always allocating an empty `Criteria` object each time this is called -
```C#
if (criteria == null)
{
criteria = Criteria.Empty;
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r527966241
##########
File path: csharp/src/Apache.Arrow.Flight/FlightInfo.cs
##########
@@ -0,0 +1,69 @@
+// 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.Collections.Generic;
+using System.Text;
+using Apache.Arrow.Flight.Writer;
+using Apache.Arrow.Ipc;
+
+namespace Apache.Arrow.Flight
+{
+ public class FlightInfo
+ {
+ private readonly Schema _schema;
+ private readonly FlightDescriptor _flightDescriptor;
+ private readonly IList<FlightEndpoint> _flightEndpoints;
Review comment:
We should also expose `TotalRecords` and `TotalByteLength` properties.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo edited a comment on pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo edited a comment on pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#issuecomment-731658726
@eerhardt first of all thank you so much for your support and feedback and sorry for this wall of text. Based on your last feedback that the API is hard to change etc in the future, I really took a long sitting to go through all the classes etc, and found some issues that I really needed to address.
This resulted in a small overhaul on the structure etc. I also went through so the gRPC schema information is actually exposed to the user as well. This also resulted in some rewrites. The feedback you have given has been exactly what I needed to come to the conclusions etc on structure, so I am once again extremely thankful for all the feedback you have given.
This ofcourse increased the scope of the PR a bit again, but I hope it helps give a more real look and feel on how the framework will behave, and if the implemented design actually works.
Here comes information on the work that has been done, and also some checklists to see that all the required information is exposed etc, and why some classes are public.
# Major changes
* All exposed classes start with prefix "Flight".
* Generated code is now internal.
* New project Apache.Arrow.Flight.AspNetCore, required since generated code is internal, this project references grpc.aspnetcore.server which is netcoreapp3.1 only, and contains extensions only to add flight support for aspnetcore projects.
* All flight client methods now follows gRPC API more closely, allows reading response headers for all calls.
* Exposing application metadata required rewriting a bit of the logic in client and servers, read more in its chapter.
* Moved client specific classes under .Client namespace.
* Moved server specific classes under .Server namespace.
* Internal classes are now under the .Internal namespace.
* Only mutual used classes are now in the .Flight namespace.
# Minor changes
* Removed multi property exposure of bytestring, to only bytestring.
* Exposed TotalBytes and TotalRecords in FlightInfo.
* Moved public abstract reader, writer classes to internal namespace since they cant be used by the user.
* Added FlightServerRecordBatchStreamReader to expose FlightDescriptor only to the server since the client wont use it.
* Added FlightClientRecordBatchStreamReader which just extends FlightRecordBatchStreamReader which is now abstract.
* StreamWriter made internal
* PutResult does not expose ArrowBuffer (was taken from java project), instead it exposes bytestring like the other classes.
This was done so the user can choose what type of metadata to return, and also follow more closely the other classes.
* Added public static Empty on PutResult, can be useful for implementations when metadata is not used, and empty put result should be returned.
* Added a new test case to get metadata.
* Added a new test case to put metadata.
* Added a new test case for get schema.
* Added a new test case for DoAction.
* Added a new test case for ListFlights.
Test coverage is now above 80% on the non generated code.
# Application metadata addition
To enable users to write and read application metadata, the class FlightRecordBatchStreamingCall was added that
still follow gRPC look and feel, but exposes FlightRecordBatchStreamReader as response stream instead where the user can
get application metadata.
Application metadata can be read with each record batch, it is implemented as a list since hypothetically metadata can
be sent with the schema aswell. Since the packages sent follow this pattern at the moment:
Schema -> record batch -> record batch -> ... -> done
So the first message could contains metadata as well. When a user gets a new record batch the metadata is cleared similar
to how the java implementation is done.
For writing, this is done by an additional method in FlightRecordBatchStreamWriter which looks like this:
```
public Task WriteAsync(RecordBatch message, ByteString applicationMetadata);
```
# gRPC support
Here is a check on what gRPC methods and properties are exposed and can be read/used by a user of the framework, mostly to check that everything that is required to be exposed are exposed at this time.
## gRPC methods exposed or not exposed to the user
* ListFlights - Exposed
* GetFlightInfo - Exposed
* GetSchema - Exposed
* DoGet - Exposed
* DoPut - Exposed
* DoAction - Exposed
* ListActions -Exposed
* **Handshake - Not exposed**
* **DoExchange - Not exposed**
## gRPC properties exposed or not exposed to the user
### HandshakeRequest
**Handshake is not exposed at all**
* **protocol_version - not exposed**
* **payload - not exposed**
### HandshakeResponse
**Handshake is not exposed at all**
* **protocol_version - not exposed**
* **payload - not exposed**
### BasicAuth
**Basic auth is not exposed at all**
* **username - not exposed**
* **password - not exposed**
### ActionType
* Type - exposed
* Description - exposed
### Criteria
* Expression - exposed
### Action
* Type - exposed
* Body - exposed
### Result
* Body - exposed
### SchemaResult
*Schema result is not mapped to its own type, but returns a schema directly*
* Schema - exposed
### FlightDescriptor
* Type - exposed
* Path - exposed
* Cmd - exposed
### FlightInfo
* Schema - exposed
* FlightDescriptor - exposed
* endpoints - exposed
* TotalRecords - exposed
* TotalBytes - exposed
### FlightEndpoint
* Ticket - exposed
* Locations - exposed
### Location
* Uri - exposed
### Ticket
* Ticket - exposed
### FlightData
Flight data is not exposed as a class, but the data is exposed in getStream, startPut for client, and DoGet and DoPut for server.
* **FlightDescriptor**
* **client can only send flight descriptor for flight data**
* **server can only read for flight data**
* Desc: *The descriptor of the data. This is only relevant when a client is starting a new DoPut stream.*
* **Data header - used to read header of message, ex: schema, record batch message etc, not explicitly exposed and used internally only**
* Application Metadata
* Client can write metadata through *ClientRecordBatchStreamWriter*
* Client can read metadata through *FlightRecordBatchStreamReader*
* Server can write metadata through FlightServerRecordBatchStreamWriter
* Server can read metadata through *FlightRecordBatchStreamReader*
* **Data body - same as data header, exposure is done through RecordBatch**
### PutResult
* Application metadata - exposed
# Classes public or internal
Here is a list of classes and if they are public or internal, and motivation on why they are public.
* FlightRecordBatchStreamingCall - public, required since it exposes FlightRecordBatchStreamReader which is required to read metadata.
* FlightRecordBatchStreamReader - public, required to allow read on schema, flight descriptor, application metadata
* RecordBatcReaderImplementation - internal
* FlightClientRecordBatchStreamWriter - public, implements CompleteAsync for clients, extends FlightRecordBatchStreamWriter wich allows user to write application metadata.
* FlightDataStream - internal
* FlightRecordBatchDuplexStreamingCall - public, exposes FlightClientRecordBatchStreamWriter which is required to write application metadata.
* FlightRecordBatchStreamWriter - public abstract with private protected constructor, client and server implementations extend this one.
* FlightServerRecordBatchStreamWriter - public, extends FlightRecordBatchStreamWriter which allows user to write application metadata.
* SchemaWriter - internal
* FlightAction - public, allows user to read/write type, and body in Actions.
* FlightActionType - public, allows user to read/write action types with type and description from ListActions.
* FlightClient - public, allows user to call all the different endpoints.
* FlightCriteria - public, exposes Expression that servers can implement to filter result from ListFlights.
* FlightDescriptor - public, exposes DescriptorType, Paths and Cmd for user.
* FlightDescriptorType - public, contains descriptor types.
* FlightEndpoint - public, exposes flight ticket and flight locations.
* FlightInfo - public, exposes flight descriptor, schema, total bytes, total records and endpoints.
* FlightLocation - public, exposes uri to the user.
* FlightMessageSerializer - internal
* FlightPutResult - public, recieved when doing doPut, exposes application metadata.
* FlightResult - public, contains body from DoAction calls.
* FlightServerImplementation - internal, forced internal from putting generated code to internal.
* FlightTicket - public, exposes the bytestring from the ticket to the user.
* IFlightServer - public, interface to implement a flight server.
* StreamReader - internal
* StreamWriter - internal
* FlightIEndpointRouteBuilderExtensions - public, allows user to map the flight endpoint in asp net core.
* FlightIGrpcServerBuilderExtensions - public, allows the user to add their implementation of IFlightServer in a nicer way.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r528208964
##########
File path: csharp/src/Apache.Arrow.Flight/Action.cs
##########
@@ -0,0 +1,71 @@
+// 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.Collections.Generic;
+using System.Text;
+using Google.Protobuf;
+
+namespace Apache.Arrow.Flight
+{
+ public class Action
Review comment:
@eerhardt I think I will actually prefix all class names by Flight if that sounds ok to you as well. A lot of the names are quite generic such as Action, Location, Result, Criteria. There mightbe conflicts with other libraries for the other names as well. So I were thinking that having the same prefix gives it some kind of uniformity.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r527930299
##########
File path: csharp/src/Apache.Arrow.Flight/FlightInfo.cs
##########
@@ -0,0 +1,69 @@
+// 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.Collections.Generic;
+using System.Text;
+using Apache.Arrow.Flight.Writer;
+using Apache.Arrow.Ipc;
+
+namespace Apache.Arrow.Flight
+{
+ public class FlightInfo
+ {
+ private readonly Schema _schema;
+ private readonly FlightDescriptor _flightDescriptor;
+ private readonly IList<FlightEndpoint> _flightEndpoints;
Review comment:
We should expose the `FlightDescriptor` and `Schema` publicly so callers can inspect them.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r526101764
##########
File path: csharp/src/Apache.Arrow.Flight/Apache.Arrow.Flight.csproj
##########
@@ -0,0 +1,28 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+ <PropertyGroup>
+ <TargetFramework>netstandard2.1</TargetFramework>
+ <LangVersion>8.0</LangVersion>
+ <EmbedUntrackedSources>true</EmbedUntrackedSources>
Review comment:
This was changed in #8702 so it can be tested/approved seperately.
##########
File path: csharp/src/Apache.Arrow.Flight/Apache.Arrow.Flight.csproj
##########
@@ -0,0 +1,28 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+ <PropertyGroup>
+ <TargetFramework>netstandard2.1</TargetFramework>
+ <LangVersion>8.0</LangVersion>
Review comment:
This was changed in #8702 so it can be tested/approved seperately.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r527973234
##########
File path: csharp/src/Apache.Arrow.Flight/FlightDescriptor.cs
##########
@@ -0,0 +1,105 @@
+// 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.Collections.Generic;
+using System.Linq;
+using System.Text;
+using Google.Protobuf;
+
+namespace Apache.Arrow.Flight
+{
+ public class FlightDescriptor
+ {
+ private readonly Protocol.FlightDescriptor _flightDescriptor;
+
+ private FlightDescriptor(ByteString command)
+ {
+ _flightDescriptor = new Protocol.FlightDescriptor()
+ {
+ Cmd = command,
+ Type = Protocol.FlightDescriptor.Types.DescriptorType.Cmd
+ };
+ }
+
+ private FlightDescriptor(params string[] paths)
+ {
+ _flightDescriptor = new Protocol.FlightDescriptor()
+ {
+ Type = Protocol.FlightDescriptor.Types.DescriptorType.Path
+ };
+
+ foreach(var path in paths)
+ {
+ _flightDescriptor.Path.Add(path);
+ }
+ }
+
+
+ public static FlightDescriptor Command(byte[] command)
+ {
+ return new FlightDescriptor(ByteString.CopyFrom(command));
+ }
+
+ public static FlightDescriptor Command(string command)
+ {
+ return new FlightDescriptor(ByteString.CopyFromUtf8(command));
+ }
+
+ public static FlightDescriptor Path(params string[] paths)
+ {
+ return new FlightDescriptor(paths);
+ }
+
+
+ internal FlightDescriptor(Protocol.FlightDescriptor flightDescriptor)
+ {
+ if(flightDescriptor.Type != Protocol.FlightDescriptor.Types.DescriptorType.Cmd && flightDescriptor.Type != Protocol.FlightDescriptor.Types.DescriptorType.Path)
+ {
+ throw new NotSupportedException();
+ }
+ _flightDescriptor = flightDescriptor;
+ }
+
+ internal Protocol.FlightDescriptor ToProtocol()
+ {
+ return _flightDescriptor;
+ }
+
+ public bool IsCommand => _flightDescriptor.Type == Protocol.FlightDescriptor.Types.DescriptorType.Cmd;
Review comment:
I think we should expose the `DescriptorType` as an enum instead of this `bool IsCommand` property. When callers are checking for a `PATH`, they would write `!IsCommand`. But this is an issue because:
1. If it isn't a Command, then what is it? It's not obvious from reading the code.
2. If there ever was a new kind of descriptor added, all existing code would be broken.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] JamesNK commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
JamesNK commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525573316
##########
File path: csharp/test/Apache.Arrow.Flight.TestWeb/Apache.Arrow.Flight.TestWeb.csproj
##########
@@ -0,0 +1,16 @@
+<Project Sdk="Microsoft.NET.Sdk.Web">
+
+ <PropertyGroup>
+ <TargetFramework>netcoreapp3.0</TargetFramework>
+ </PropertyGroup>
+
+ <ItemGroup>
+ <PackageReference Include="Grpc" Version="2.33.1" />
Review comment:
Not needed
```suggestion
```
##########
File path: csharp/test/Apache.Arrow.Flight.TestWeb/Apache.Arrow.Flight.TestWeb.csproj
##########
@@ -0,0 +1,16 @@
+<Project Sdk="Microsoft.NET.Sdk.Web">
+
+ <PropertyGroup>
+ <TargetFramework>netcoreapp3.0</TargetFramework>
+ </PropertyGroup>
+
+ <ItemGroup>
+ <PackageReference Include="Grpc" Version="2.33.1" />
+ <PackageReference Include="Grpc.AspNetCore" Version="2.27.0" />
Review comment:
Use latest version
```suggestion
<PackageReference Include="Grpc.AspNetCore" Version="2.33.1" />
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525561695
##########
File path: csharp/src/Apache.Arrow.Flight/Apache.Arrow.Flight.csproj
##########
@@ -0,0 +1,18 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+ <PropertyGroup>
+ <TargetFramework>netstandard2.1</TargetFramework>
+ <LangVersion>8.0</LangVersion>
+ </PropertyGroup>
+
+ <ItemGroup>
+ <PackageReference Include="Google.Protobuf" Version="3.14.0" />
+ <PackageReference Include="Grpc" Version="2.33.1" />
Review comment:
I removed the reference to Grpc
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r527931177
##########
File path: csharp/src/Apache.Arrow.Flight/Ticket.cs
##########
@@ -0,0 +1,60 @@
+// 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.Collections.Generic;
+using System.Text;
+using Google.Protobuf;
+
+namespace Apache.Arrow.Flight
+{
+ public class Ticket
+ {
+ private readonly Protocol.Ticket _ticket;
+ internal Ticket(Protocol.Ticket ticket)
+ {
+ _ticket = ticket;
+ }
+
+ public Ticket(ByteString ticket)
+ {
+ _ticket = new Protocol.Ticket()
+ {
+ Ticket_ = ticket
+ };
+ }
+
+ public Ticket(string ticket)
+ : this(ByteString.CopyFromUtf8(ticket))
+ {
+ }
+
+ public Ticket(byte[] bytes)
+ : this(ByteString.CopyFrom(bytes))
+ {
+ }
+
+ public string TicketString => _ticket.Ticket_.ToStringUtf8();
+
+ public ByteString TicketByteString => _ticket.Ticket_;
+
+ public byte[] TicketBytes => _ticket.Ticket_.ToByteArray();
Review comment:
Note, this apply elsewhere where we have the same 3 public properties. Ex. `FlightDescriptor.Command`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r529341339
##########
File path: csharp/src/Apache.Arrow.Flight/Client/FlightRecordBatchDuplexStreamingCall.cs
##########
@@ -0,0 +1,92 @@
+// 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 Grpc.Core;
+
+namespace Apache.Arrow.Flight.Client
+{
+ public class FlightRecordBatchDuplexStreamingCall : IDisposable
+ {
+ private readonly Func<Status> _getStatusFunc;
+ private readonly Func<Metadata> _getTrailersFunc;
+ private readonly Action _disposeAction;
+
+ internal FlightRecordBatchDuplexStreamingCall(
+ FlightClientRecordBatchStreamWriter requestStream,
+ IAsyncStreamReader<FlightPutResult> responseStream,
+ Task<Metadata> responseHeadersAsync,
+ Func<Status> getStatusFunc,
+ Func<Metadata> getTrailersFunc,
+ Action disposeAction)
+ {
+ RequestStream = requestStream;
+ ResponseStream = responseStream;
+ ResponseHeadersAsync = responseHeadersAsync;
+ _getStatusFunc = getStatusFunc;
+ _getTrailersFunc = getTrailersFunc;
+ _disposeAction = disposeAction;
+ }
+
+ /// <summary>
+ /// Async stream to read streaming responses.
+ /// </summary>
+ public IAsyncStreamReader<FlightPutResult> ResponseStream { get; }
+
+ /// <summary>
+ /// Async stream to send streaming requests.
+ /// </summary>
+ public FlightClientRecordBatchStreamWriter RequestStream { get; }
+
+ /// <summary>
+ /// Asynchronous access to response headers.
+ /// </summary>
+ public Task<Metadata> ResponseHeadersAsync { get; }
+
+ /// <summary>
+ /// Provides means to cleanup after the call. If the call has already finished normally
+ /// (response stream has been fully read), doesn't do anything. Otherwise, requests
+ /// cancellation of the call which should terminate all pending async operations
+ /// associated with the call. As a result, all resources being used by the call should
+ /// be released eventually.
+ /// </summary>
+ /// <remarks>
+ /// Normally, there is no need for you to dispose the call unless you want to utilize
+ /// the "Cancel" semantics of invoking Dispose.
+ /// </remarks>
+ public void Dispose()
+ {
+ _disposeAction();
+ }
+
+ //
+ // Summary:
+ // Gets the call status if the call has already finished. Throws InvalidOperationException
Review comment:
The caller ends the stream, so they call CompleteAsync on the request stream.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo edited a comment on pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo edited a comment on pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#issuecomment-732091242
@eerhardt to be honest after I went through and really thought regarding the calls, I had one final thing I was not too happy with.
The record batch stream in for get and put, I did not change it since compared to moving files around etc this felt a tiny bit bigger.
The API in itself feels quite easy to use, but I think I overspecified it for RecordBatch, and there will be trouble if we add dictionary support etc.
It also does not work well with DoExchange, and do exchange requires another implementation to work good.
The solution I can think of would reduce on ease of use thought (I think?), which would be:
* New classes, FlightData (abstract), FlightDataSchema, FlightDataRecordBatch
* Stream returns FlightData
* internal code still checks flight "correctness" (schema first message etc).
FlightData structure:
```
pubic abstract class FlightData {
public FlightDescriptor FlightDescriptor { get; }
public FlightDataType Type { get; } //Contains what type of arrow object it was
public ByteString ApplicationMetadata { get; } //app metadata can be changed from list in streamreader to correct single in FlightData.
public abstract T GetValue<T>();
public abstract void Accept(FlightDataVisitor visitor); // Maybe a visitor pattern?
}
public class FlightDataRecordBatch : FlightData {
public RecordBatch RecordBatch { get; }
public GetValue<T>() {
if(typeof(T) != typeof(RecordBatch)) { throw new Exception(...); }
return RecordBatch;
}
public Accept(...) {...};
}
enum FlightDataType {
Schema = 1,
RecordBatch = 2,
...
}
```
This stream solution could also have a FlightDataVisitor for the different types also, and I think that it allows
easier addition in the future of new arrow objects. But it of course might be a bit harder to use.
It also follows the gRPC schema much more closely, which makes DoExchange simple to implement.
If not using a visitor, the loop would not be as "pretty" but still functional with easier additions of new types (atleast in my opinion):
```
while(stream.MoveNext()) {
if(stream.Current.Type == FlightDataType.RecordBatch) {
stream.Current.GetValue<RecordBatch>();
// do stuff with record batch
}
}
```
Would love to get your input, so the flight framework starts with a good foundation.
**EDIT: Doing put operations would be a bit wierder with this though, it would require the user to have knowledge to send the schema as the first message. I think the putStream should still be similar to the existing solution even with this, but with extra write commands such as Write(ArrowDictionary) etc.**
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] JamesNK commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
JamesNK commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525577342
##########
File path: csharp/test/Apache.Arrow.Flight.TestWeb/Apache.Arrow.Flight.TestWeb.csproj
##########
@@ -0,0 +1,16 @@
+<Project Sdk="Microsoft.NET.Sdk.Web">
+
+ <PropertyGroup>
+ <TargetFramework>netcoreapp3.0</TargetFramework>
+ </PropertyGroup>
+
+ <ItemGroup>
+ <PackageReference Include="Grpc" Version="2.33.1" />
Review comment:
Grpc brings in some big (50+ meg) NuGet packages. I'd copy the source code to avoid brining them in.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#issuecomment-767155085
FYI - @Ulimo, the `3.0.0` nuget packages have been released. You can see them at:
https://www.nuget.org/packages/Apache.Arrow.Flight/
https://www.nuget.org/packages/Apache.Arrow.Flight.AspNetCore/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r528917658
##########
File path: csharp/src/Apache.Arrow.Flight/FlightInfo.cs
##########
@@ -0,0 +1,78 @@
+// 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.Collections.Generic;
+using System.Text;
+using Apache.Arrow.Flight.Internal;
+using Apache.Arrow.Ipc;
+
+namespace Apache.Arrow.Flight
+{
+ public class FlightInfo
+ {
+ internal FlightInfo(Protocol.FlightInfo flightInfo)
+ {
+ Schema = FlightMessageSerializer.DecodeSchema(flightInfo.Schema.Memory);
+ FlightDescriptor = new FlightDescriptor(flightInfo.FlightDescriptor);
+
+ var endpoints = new List<FlightEndpoint>();
+ foreach(var endpoint in flightInfo.Endpoint)
+ {
+ endpoints.Add(new FlightEndpoint(endpoint));
+ }
+ Endpoints = endpoints;
+
+ TotalBytes = flightInfo.TotalBytes;
+ TotalRecords = flightInfo.TotalRecords;
+ }
+
+ public FlightInfo(Schema schema, FlightDescriptor flightDescriptor, IReadOnlyList<FlightEndpoint> flightEndpoints, long totalRecords = 0, long totalBytes = 0)
+ {
+ Schema = schema;
+ FlightDescriptor = flightDescriptor;
+ Endpoints = flightEndpoints;
+ TotalBytes = totalBytes;
+ TotalRecords = totalRecords;
+ }
+
+ public FlightDescriptor FlightDescriptor { get; }
Review comment:
I think this property can just be named `Descriptor`. The property name doesn't need a `Flight` prefix.
```suggestion
public FlightDescriptor Descriptor { get; }
```
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs
##########
@@ -343,7 +343,7 @@ private protected void WriteRecordBatchInternal(RecordBatch recordBatch)
FinishedWritingRecordBatch(bodyLength + bodyPaddingLength, metadataLength);
}
- private Tuple<ArrowRecordBatchFlatBufferBuilder, VectorOffset> PreparingWritingRecordBatch(RecordBatch recordBatch)
+ private protected Tuple<ArrowRecordBatchFlatBufferBuilder, VectorOffset> PreparingWritingRecordBatch(RecordBatch recordBatch)
Review comment:
Is this change necessary? I don't see additional callers of this method.
##########
File path: csharp/src/Apache.Arrow.Flight/FlightDescriptor.cs
##########
@@ -0,0 +1,102 @@
+// 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.Collections.Generic;
+using System.Linq;
+using System.Text;
+using Google.Protobuf;
+
+namespace Apache.Arrow.Flight
+{
+ public class FlightDescriptor
+ {
+ private readonly Protocol.FlightDescriptor _flightDescriptor;
+
+ private FlightDescriptor(ByteString command)
+ {
+ _flightDescriptor = new Protocol.FlightDescriptor()
+ {
+ Cmd = command,
+ Type = Protocol.FlightDescriptor.Types.DescriptorType.Cmd
+ };
+ }
+
+ private FlightDescriptor(params string[] paths)
+ {
+ _flightDescriptor = new Protocol.FlightDescriptor()
+ {
+ Type = Protocol.FlightDescriptor.Types.DescriptorType.Path
+ };
+
+ foreach(var path in paths)
+ {
+ _flightDescriptor.Path.Add(path);
+ }
+ }
+
+
+ public static FlightDescriptor Command(byte[] command)
+ {
+ return new FlightDescriptor(ByteString.CopyFrom(command));
+ }
+
+ public static FlightDescriptor Command(string command)
+ {
+ return new FlightDescriptor(ByteString.CopyFromUtf8(command));
+ }
+
+ public static FlightDescriptor Path(params string[] paths)
+ {
+ return new FlightDescriptor(paths);
+ }
+
+
+ internal FlightDescriptor(Protocol.FlightDescriptor flightDescriptor)
+ {
+ if(flightDescriptor.Type != Protocol.FlightDescriptor.Types.DescriptorType.Cmd && flightDescriptor.Type != Protocol.FlightDescriptor.Types.DescriptorType.Path)
+ {
+ throw new NotSupportedException();
+ }
+ _flightDescriptor = flightDescriptor;
+ }
+
+ internal Protocol.FlightDescriptor ToProtocol()
+ {
+ return _flightDescriptor;
+ }
+
+ public FlightDescriptorType Type => (FlightDescriptorType)_flightDescriptor.Type;
+
+ public IEnumerable<string> Paths => _flightDescriptor.Path;
+
+ public ByteString Cmd => _flightDescriptor.Cmd;
Review comment:
We should spell out the full word. .NET design guidelines say not to use abbreviations in public APIs.
https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/general-naming-conventions
```suggestion
public ByteString Command => _flightDescriptor.Cmd;
```
##########
File path: csharp/src/Apache.Arrow.Flight/Client/FlightRecordBatchDuplexStreamingCall.cs
##########
@@ -0,0 +1,92 @@
+// 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 Grpc.Core;
+
+namespace Apache.Arrow.Flight.Client
+{
+ public class FlightRecordBatchDuplexStreamingCall : IDisposable
+ {
+ private readonly Func<Status> _getStatusFunc;
+ private readonly Func<Metadata> _getTrailersFunc;
+ private readonly Action _disposeAction;
+
+ internal FlightRecordBatchDuplexStreamingCall(
+ FlightClientRecordBatchStreamWriter requestStream,
+ IAsyncStreamReader<FlightPutResult> responseStream,
+ Task<Metadata> responseHeadersAsync,
+ Func<Status> getStatusFunc,
+ Func<Metadata> getTrailersFunc,
+ Action disposeAction)
+ {
+ RequestStream = requestStream;
+ ResponseStream = responseStream;
+ ResponseHeadersAsync = responseHeadersAsync;
+ _getStatusFunc = getStatusFunc;
+ _getTrailersFunc = getTrailersFunc;
+ _disposeAction = disposeAction;
+ }
+
+ /// <summary>
+ /// Async stream to read streaming responses.
+ /// </summary>
+ public IAsyncStreamReader<FlightPutResult> ResponseStream { get; }
+
+ /// <summary>
+ /// Async stream to send streaming requests.
+ /// </summary>
+ public FlightClientRecordBatchStreamWriter RequestStream { get; }
+
+ /// <summary>
+ /// Asynchronous access to response headers.
+ /// </summary>
+ public Task<Metadata> ResponseHeadersAsync { get; }
+
+ /// <summary>
+ /// Provides means to cleanup after the call. If the call has already finished normally
+ /// (response stream has been fully read), doesn't do anything. Otherwise, requests
+ /// cancellation of the call which should terminate all pending async operations
+ /// associated with the call. As a result, all resources being used by the call should
+ /// be released eventually.
+ /// </summary>
+ /// <remarks>
+ /// Normally, there is no need for you to dispose the call unless you want to utilize
+ /// the "Cancel" semantics of invoking Dispose.
+ /// </remarks>
+ public void Dispose()
+ {
+ _disposeAction();
+ }
+
+ //
+ // Summary:
+ // Gets the call status if the call has already finished. Throws InvalidOperationException
Review comment:
How do callers know when the call has finished? Do they `await` on the `ResponseHeadersAsync`?
##########
File path: csharp/src/Apache.Arrow.Flight/Internal/FlightMessageSerializer.cs
##########
@@ -0,0 +1,57 @@
+// 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.Buffers.Binary;
+using System.Collections.Generic;
+using System.IO;
+using System.Text;
+using Apache.Arrow.Ipc;
+using FlatBuffers;
+
+namespace Apache.Arrow.Flight
+{
+ internal static class FlightMessageSerializer
+ {
+ public static Schema DecodeSchema(ReadOnlyMemory<byte> buffer)
+ {
+ int bufferPosition = 0;
+ int schemaMessageLength = BinaryPrimitives.ReadInt32LittleEndian(buffer.Span.Slice(bufferPosition));
+ bufferPosition += sizeof(int);
+
+ if (schemaMessageLength == MessageSerializer.IpcContinuationToken)
+ {
+ // ARROW-6313, if the first 4 bytes are continuation message, read the next 4 for the length
+ if (buffer.Length <= bufferPosition + sizeof(int))
+ {
+ throw new InvalidDataException("Corrupted IPC message. Received a continuation token at the end of the message.");
+ }
+
+ schemaMessageLength = BinaryPrimitives.ReadInt32LittleEndian(buffer.Span.Slice(bufferPosition));
+ bufferPosition += sizeof(int);
+ }
+
+ ByteBuffer schemaBuffer = ArrowReaderImplementation.CreateByteBuffer(buffer.Slice(bufferPosition));
+ var Schema = MessageSerializer.GetSchema(ArrowReaderImplementation.ReadMessage<Flatbuf.Schema>(schemaBuffer));
+ return Schema;
+ }
+
+ internal static Schema DecodeSchema(ByteBuffer schemaBuffer)
+ {
+ var Schema = MessageSerializer.GetSchema(ArrowReaderImplementation.ReadMessage<Flatbuf.Schema>(schemaBuffer));
+ return Schema;
Review comment:
(nit) variable names should be camelCased. Or just skip the variable altogether.
```suggestion
return MessageSerializer.GetSchema(ArrowReaderImplementation.ReadMessage<Flatbuf.Schema>(schemaBuffer));
}
internal static Schema DecodeSchema(ByteBuffer schemaBuffer)
{
return MessageSerializer.GetSchema(ArrowReaderImplementation.ReadMessage<Flatbuf.Schema>(schemaBuffer));
```
##########
File path: csharp/src/Apache.Arrow.Flight/Client/FlightClientRecordBatchStreamWriter.cs
##########
@@ -0,0 +1,52 @@
+// 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.Collections.Generic;
+using System.Text;
+using System.Threading.Tasks;
+using Apache.Arrow.Flight.Protocol;
+using Apache.Arrow.Flight.Internal;
+using Grpc.Core;
+
+namespace Apache.Arrow.Flight.Client
+{
+ public class FlightClientRecordBatchStreamWriter : FlightRecordBatchStreamWriter, IClientStreamWriter<RecordBatch>
+ {
+ private readonly IClientStreamWriter<FlightData> _clientStreamWriter;
+ private bool _completed = false;
+ internal FlightClientRecordBatchStreamWriter(IClientStreamWriter<FlightData> clientStreamWriter, FlightDescriptor flightDescriptor) : base(clientStreamWriter, flightDescriptor)
+ {
+ _clientStreamWriter = clientStreamWriter;
+ }
+
+ protected override void Dispose(bool disposing)
+ {
+ CompleteAsync().Wait();
Review comment:
Is this necessary? Having sync-over-async is typically not good.
What are the ramifications if someone doesn't call CompleteAsync() before disposing? Should we just `throw` in that case?
##########
File path: csharp/src/Apache.Arrow.Flight/Client/FlightRecordBatchDuplexStreamingCall.cs
##########
@@ -0,0 +1,92 @@
+// 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 Grpc.Core;
+
+namespace Apache.Arrow.Flight.Client
+{
+ public class FlightRecordBatchDuplexStreamingCall : IDisposable
+ {
+ private readonly Func<Status> _getStatusFunc;
+ private readonly Func<Metadata> _getTrailersFunc;
+ private readonly Action _disposeAction;
+
+ internal FlightRecordBatchDuplexStreamingCall(
+ FlightClientRecordBatchStreamWriter requestStream,
+ IAsyncStreamReader<FlightPutResult> responseStream,
+ Task<Metadata> responseHeadersAsync,
+ Func<Status> getStatusFunc,
+ Func<Metadata> getTrailersFunc,
+ Action disposeAction)
+ {
+ RequestStream = requestStream;
+ ResponseStream = responseStream;
+ ResponseHeadersAsync = responseHeadersAsync;
+ _getStatusFunc = getStatusFunc;
+ _getTrailersFunc = getTrailersFunc;
+ _disposeAction = disposeAction;
+ }
+
+ /// <summary>
+ /// Async stream to read streaming responses.
+ /// </summary>
+ public IAsyncStreamReader<FlightPutResult> ResponseStream { get; }
+
+ /// <summary>
+ /// Async stream to send streaming requests.
+ /// </summary>
+ public FlightClientRecordBatchStreamWriter RequestStream { get; }
+
+ /// <summary>
+ /// Asynchronous access to response headers.
+ /// </summary>
+ public Task<Metadata> ResponseHeadersAsync { get; }
+
+ /// <summary>
+ /// Provides means to cleanup after the call. If the call has already finished normally
+ /// (response stream has been fully read), doesn't do anything. Otherwise, requests
+ /// cancellation of the call which should terminate all pending async operations
+ /// associated with the call. As a result, all resources being used by the call should
+ /// be released eventually.
+ /// </summary>
+ /// <remarks>
+ /// Normally, there is no need for you to dispose the call unless you want to utilize
+ /// the "Cancel" semantics of invoking Dispose.
+ /// </remarks>
+ public void Dispose()
+ {
+ _disposeAction();
+ }
+
+ //
+ // Summary:
Review comment:
Can these be `xml` comments?
##########
File path: csharp/src/Apache.Arrow.Flight/Internal/FlightRecordBatchStreamWriter.cs
##########
@@ -0,0 +1,81 @@
+// 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.Collections.Generic;
+using System.Text;
+using System.Threading.Tasks;
+using Apache.Arrow.Flight.Protocol;
+using Google.Protobuf;
+using Grpc.Core;
+
+namespace Apache.Arrow.Flight.Internal
+{
+ public abstract class FlightRecordBatchStreamWriter : IAsyncStreamWriter<RecordBatch>, IDisposable
+ {
+ private FlightDataStream _flightDataStream;
+ private readonly IAsyncStreamWriter<FlightData> _clientStreamWriter;
+ private readonly FlightDescriptor _flightDescriptor;
+
+ private bool _disposed;
+
+ private protected FlightRecordBatchStreamWriter(IAsyncStreamWriter<FlightData> clientStreamWriter, FlightDescriptor flightDescriptor)
+ {
+ _clientStreamWriter = clientStreamWriter;
+ _flightDescriptor = flightDescriptor;
+ }
+
+ private void SetupStream(Schema schema)
+ {
+ _flightDataStream = new FlightDataStream(_clientStreamWriter, _flightDescriptor, schema);
+ }
+
+ public WriteOptions WriteOptions { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }
+
+ public Task WriteAsync(RecordBatch message)
+ {
+ return WriteAsync(message, default);
+ }
+
+ public Task WriteAsync(RecordBatch message, ByteString applicationMetadata)
+ {
+ if (_flightDataStream == null)
+ {
+ SetupStream(message.Schema);
+ }
+
+ return _flightDataStream.Write(message, applicationMetadata);
+ }
+
+ protected virtual void Dispose(bool disposing)
+ {
+ if (!_disposed)
+ {
+ _flightDataStream.Dispose();
+ _disposed = true;
+ }
+ }
+
+ ~FlightRecordBatchStreamWriter()
Review comment:
We should remove any finalizers. The only reason to use a finalizer is when a class is directly managing native resources. And even then, we try to use SafeHandles when possible.
##########
File path: csharp/src/Apache.Arrow.Flight/Internal/FlightRecordBatchStreamReader.cs
##########
@@ -0,0 +1,65 @@
+// 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.Collections.Generic;
+using System.Text;
+using System.Threading;
+using System.Threading.Tasks;
+using Apache.Arrow.Flatbuf;
+using Apache.Arrow.Flight.Protocol;
+using Apache.Arrow.Ipc;
+using Google.Protobuf;
+using Grpc.Core;
+
+namespace Apache.Arrow.Flight.Internal
+{
+ /// <summary>
+ /// Stream of record batches
+ ///
+ /// Use MoveNext() and Current to iterate over the batches.
+ /// There are also gRPC helper functions such as ToListAsync() etc.
+ /// </summary>
+ public abstract class FlightRecordBatchStreamReader : IAsyncStreamReader<RecordBatch>
Review comment:
Can this class also implement `IAsyncEnumerable<RecordBatch>`? Then callers can just directly `await foreach` it, without having to call `ReadAllAsync()`.
My understanding of why `IAsyncStreamReader<T>` doesn't implement `IAsyncEnumerable<T>` is because the Grpc interface came first, and it couldn't be changed without a breaking change. @JamesNK - is this the case?
##########
File path: csharp/src/Apache.Arrow.Flight/Internal/FlightRecordBatchStreamReader.cs
##########
@@ -0,0 +1,65 @@
+// 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.Collections.Generic;
+using System.Text;
+using System.Threading;
+using System.Threading.Tasks;
+using Apache.Arrow.Flatbuf;
+using Apache.Arrow.Flight.Protocol;
+using Apache.Arrow.Ipc;
+using Google.Protobuf;
+using Grpc.Core;
+
+namespace Apache.Arrow.Flight.Internal
Review comment:
Can we move this to the root namespace? I don't really like public types in an `Internal` namespace. It feels like an anti-pattern 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r528265664
##########
File path: csharp/src/Apache.Arrow.Flight/StreamWriter.cs
##########
@@ -0,0 +1,51 @@
+// 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.Collections.Generic;
+using System.Text;
+using System.Threading.Tasks;
+using Grpc.Core;
+
+namespace Apache.Arrow.Flight
+{
+ public class StreamWriter<TIn, TOut> : IAsyncStreamWriter<TIn>
Review comment:
changed to internal
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#issuecomment-733176431
Do you think you can fill out this table with the current support?
https://github.com/apache/arrow/blob/master/docs/source/status.rst#flight-rpc
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525574841
##########
File path: csharp/test/Apache.Arrow.Flight.TestWeb/Apache.Arrow.Flight.TestWeb.csproj
##########
@@ -0,0 +1,16 @@
+<Project Sdk="Microsoft.NET.Sdk.Web">
+
+ <PropertyGroup>
+ <TargetFramework>netcoreapp3.0</TargetFramework>
+ </PropertyGroup>
+
+ <ItemGroup>
+ <PackageReference Include="Grpc" Version="2.33.1" />
Review comment:
This is just for the tests where I use some of the extension methods, such as ToListAsync() etc, Is it considered bad practice to use it even in those cases?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo edited a comment on pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo edited a comment on pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#issuecomment-732091242
@eerhardt to be honest after I went through and really thought regarding the calls, I had one final thing I was not too happy with.
The record batch stream in for get and put, I did not change it since compared to moving files around etc this felt a tiny bit bigger.
The API in itself feels quite easy to use, but I think I overspecified it for RecordBatch, and there will be trouble if we add dictionary support etc.
It also does not work well with DoExchange, and do exchange requires another implementation to work good.
The solution I can think of would reduce on ease of use thought (I think?), which would be:
* New classes, FlightData (abstract), FlightDataSchema, FlightDataRecordBatch
* Stream returns FlightData
* internal code still checks flight "correctness" (schema first message etc).
FlightData structure:
```
pubic abstract class FlightData {
public FlightDescriptor FlightDescriptor { get; }
public FlightDataType Type { get; } //Contains what type of arrow object it was
public ByteString ApplicationMetadata { get; } //app metadata can be changed from list in streamreader to correct single in FlightData.
public abstract T GetValue<T>();
public abstract void Accept(FlightDataVisitor visitor); // Maybe a visitor pattern?
}
public class FlightDataRecordBatch : FlightData {
public RecordBatch RecordBatch { get; }
public GetValue<T>() {
if(typeof(T) != typeof(RecordBatch)) { throw new Exception(...); }
return RecordBatch;
}
public Accept(...) {...};
}
enum FlightDataType {
Schema = 1,
RecordBatch = 2,
...
}
```
This stream solution could also have a FlightDataVisitor for the different types also, and I think that it allows
easier addition in the future of new arrow objects. But it of course might be a bit harder to use.
It also follows the gRPC schema much more closely, which makes DoExchange simple to implement.
If not using a visitor, the loop would not be as "pretty" but still functional with easier additions of new types (atleast in my opinion):
```
while(stream.MoveNext()) {
if(stream.Current.Type == FlightDataType.RecordBatch) {
stream.Current.GetValue<RecordBatch>();
// do stuff with record batch
}
}
```
Would love to get your input, so the flight framework starts with a good foundation.
**EDIT: Doing put operations would be a bit wierder with this though, it would require the user to have knowledge to send the schema as the first message.**
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt closed pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt closed pull request #8694:
URL: https://github.com/apache/arrow/pull/8694
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#issuecomment-732091242
@eerhardt to be honest after I went through and really thought regarding the calls, I had one final thing I was not too happy with.
The record batch stream in for get and put, I did not change it since compared to moving files around etc this felt a tiny bit bigger.
The API in itself feels quite easy to use, but I think I overspecified it for RecordBatch, and there will be trouble if we add dictionary support etc.
It also does not work well with DoExchange, and do exchange requires another implementation to work good.
The solution I can think of would reduce on ease of use thought (I think?), which would be:
* New classes, FlightData (abstract), FlightDataSchema, FlightDataRecordBatch
* Stream returns FlightData
* internal code still checks flight "correctness" (schema first message etc).
FlightData structure:
```
pubic abstract class FlightData {
public FlightDescriptor FlightDescriptor { get; }
public FlightDataType Type { get; } //Contains what type of arrow object it was
public ByteString ApplicationMetadata { get; } //app metadata can be changed from list in streamreader to correct single in FlightData.
public abstract T GetValue<T>();
public abstract void Accept(FlightDataVisitor visitor); // Maybe a visitor pattern?
}
public class FlightDataRecordBatch : FlightData {
public RecordBatch RecordBatch { get; }
public GetValue<T>() {
if(typeof(T) != typeof(RecordBatch)) { throw new Exception(...); }
return RecordBatch;
}
public Accept(...) {...};
}
enum FlightDataType {
Schema = 1,
RecordBatch = 2,
...
}
```
This stream solution could also have a FlightDataVisitor for the different types also, and I think that it allows
easier addition in the future of new arrow objects. But it of course might be a bit harder to use.
It also follows the gRPC schema much more closely, which makes DoExchange simple to implement.
If not using a visitor, the loop would not be as "pretty" but still functional with easier additions of new types (atleast in my opinion):
```
while(stream.MoveNext()) {
if(stream.Current.Type == FlightDataType.RecordBatch) {
stream.Current.GetValue<RecordBatch>();
// do stuff with record batch
}
}
```
Would love to get your input, so the flight framework starts with a good foundation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] JamesNK commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
JamesNK commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525540754
##########
File path: csharp/src/Apache.Arrow.Flight/Apache.Arrow.Flight.csproj
##########
@@ -0,0 +1,18 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+ <PropertyGroup>
+ <TargetFramework>netstandard2.1</TargetFramework>
+ <LangVersion>8.0</LangVersion>
+ </PropertyGroup>
+
+ <ItemGroup>
+ <PackageReference Include="Google.Protobuf" Version="3.14.0" />
+ <PackageReference Include="Grpc" Version="2.33.1" />
Review comment:
Actually, `Grpc` doesn't bring in `Grpc.Tools`. Should be able to remove this entirely.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#issuecomment-729017291
https://issues.apache.org/jira/browse/ARROW-10542
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r528265538
##########
File path: csharp/src/Apache.Arrow.Flight/Ticket.cs
##########
@@ -0,0 +1,60 @@
+// 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.Collections.Generic;
+using System.Text;
+using Google.Protobuf;
+
+namespace Apache.Arrow.Flight
+{
+ public class Ticket
+ {
+ private readonly Protocol.Ticket _ticket;
+ internal Ticket(Protocol.Ticket ticket)
+ {
+ _ticket = ticket;
+ }
+
+ public Ticket(ByteString ticket)
+ {
+ _ticket = new Protocol.Ticket()
+ {
+ Ticket_ = ticket
+ };
+ }
+
+ public Ticket(string ticket)
+ : this(ByteString.CopyFromUtf8(ticket))
+ {
+ }
+
+ public Ticket(byte[] bytes)
+ : this(ByteString.CopyFrom(bytes))
+ {
+ }
+
+ public string TicketString => _ticket.Ticket_.ToStringUtf8();
+
+ public ByteString TicketByteString => _ticket.Ticket_;
+
+ public byte[] TicketBytes => _ticket.Ticket_.ToByteArray();
Review comment:
Removed the 3 properties from all classes they existed in and they now only expose ByteString.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r527923215
##########
File path: csharp/src/Apache.Arrow.Flight/Ticket.cs
##########
@@ -0,0 +1,60 @@
+// 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.Collections.Generic;
+using System.Text;
+using Google.Protobuf;
+
+namespace Apache.Arrow.Flight
+{
+ public class Ticket
+ {
+ private readonly Protocol.Ticket _ticket;
+ internal Ticket(Protocol.Ticket ticket)
+ {
+ _ticket = ticket;
+ }
+
+ public Ticket(ByteString ticket)
+ {
+ _ticket = new Protocol.Ticket()
+ {
+ Ticket_ = ticket
+ };
+ }
+
+ public Ticket(string ticket)
+ : this(ByteString.CopyFromUtf8(ticket))
+ {
+ }
+
+ public Ticket(byte[] bytes)
+ : this(ByteString.CopyFrom(bytes))
+ {
+ }
+
+ public string TicketString => _ticket.Ticket_.ToStringUtf8();
+
+ public ByteString TicketByteString => _ticket.Ticket_;
+
+ public byte[] TicketBytes => _ticket.Ticket_.ToByteArray();
Review comment:
Do we really need all 3 of these as public properties?
One concern I have is that properties are supposed to be cheap, but every time you call `TicketBytes` or `TicketString`, it allocates new memory.
Can we get away with just exposing `TicketByteString` publicaly? And callers can turn it into a `byte[]` or `string` as necessary.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r527969260
##########
File path: csharp/src/Apache.Arrow.Flight/FlightInfo.cs
##########
@@ -0,0 +1,69 @@
+// 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.Collections.Generic;
+using System.Text;
+using Apache.Arrow.Flight.Writer;
+using Apache.Arrow.Ipc;
+
+namespace Apache.Arrow.Flight
+{
+ public class FlightInfo
+ {
+ private readonly Schema _schema;
+ private readonly FlightDescriptor _flightDescriptor;
+ private readonly IList<FlightEndpoint> _flightEndpoints;
+
+ internal FlightInfo(Protocol.FlightInfo flightInfo)
+ {
+ _schema = FlightMessageSerializer.DecodeSchema(flightInfo.Schema.Memory);
+ _flightDescriptor = new FlightDescriptor(flightInfo.FlightDescriptor);
+
+ var endpoints = new List<FlightEndpoint>();
+ foreach(var endpoint in flightInfo.Endpoint)
+ {
+ endpoints.Add(new FlightEndpoint(endpoint));
+ }
+ _flightEndpoints = endpoints;
+ }
+
+ public FlightInfo(Schema schema, FlightDescriptor flightDescriptor, IList<FlightEndpoint> flightEndpoints)
+ {
+ _schema = schema;
+ _flightDescriptor = flightDescriptor;
+ _flightEndpoints = flightEndpoints;
+ }
+
+ public IEnumerable<FlightEndpoint> Endpoints => _flightEndpoints;
Review comment:
This should be `IReadOnlyList<FlightEndpoint>` so people get the Count and index into it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525549849
##########
File path: csharp/src/Apache.Arrow.Flight/Generated/Flight.cs
##########
@@ -0,0 +1,3536 @@
+// <auto-generated>
Review comment:
here is the build where I saw the errors: https://github.com/apache/arrow/runs/1412215805
Error log extract:
```
2 Documents with errors:
2d1a642b553a64701a2cd2463307ee00b117363d81a8914a91a6e34b6bae0700 sha256 csharp /Users/runner/work/arrow/arrow/csharp/src/Apache.Arrow/obj/Debug/netcoreapp2.1/Format/Flight.cs
https://raw.githubusercontent.com/apache/arrow/afa5d50e94e7f75d8973aa8022502f47b93740ef/csharp/src/Apache.Arrow/obj/Debug/netcoreapp2.1/Format/Flight.cs
error: url failed NotFound: Not Found
36ececbfdcef40763e3f3effdc3fc42392fd80a6fad48bf431395d0e705d7950 sha256 csharp /Users/runner/work/arrow/arrow/csharp/src/Apache.Arrow/obj/Debug/netcoreapp2.1/Format/FlightGrpc.cs
https://raw.githubusercontent.com/apache/arrow/afa5d50e94e7f75d8973aa8022502f47b93740ef/csharp/src/Apache.Arrow/obj/Debug/netcoreapp2.1/Format/FlightGrpc.cs
error: url failed NotFound: Not Found
sourcelink test failed
```
I have not used sourcelink much myself, but it seems like it is linking it to a github file in this case, so I guessed that it is required to be checked in for it to work.
That atleast solved the issue of building (but I know that it is a very ugly fix/workaround), I searched a bit if I can change the build process is any way. But could not find anything yet.
P.S. This was before refactor of moving all the flight code into its own seperate project if the path is confusing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r528215355
##########
File path: csharp/src/Apache.Arrow.Flight/FlightDescriptor.cs
##########
@@ -0,0 +1,105 @@
+// 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.Collections.Generic;
+using System.Linq;
+using System.Text;
+using Google.Protobuf;
+
+namespace Apache.Arrow.Flight
+{
+ public class FlightDescriptor
+ {
+ private readonly Protocol.FlightDescriptor _flightDescriptor;
+
+ private FlightDescriptor(ByteString command)
+ {
+ _flightDescriptor = new Protocol.FlightDescriptor()
+ {
+ Cmd = command,
+ Type = Protocol.FlightDescriptor.Types.DescriptorType.Cmd
+ };
+ }
+
+ private FlightDescriptor(params string[] paths)
+ {
+ _flightDescriptor = new Protocol.FlightDescriptor()
+ {
+ Type = Protocol.FlightDescriptor.Types.DescriptorType.Path
+ };
+
+ foreach(var path in paths)
+ {
+ _flightDescriptor.Path.Add(path);
+ }
+ }
+
+
+ public static FlightDescriptor Command(byte[] command)
+ {
+ return new FlightDescriptor(ByteString.CopyFrom(command));
+ }
+
+ public static FlightDescriptor Command(string command)
+ {
+ return new FlightDescriptor(ByteString.CopyFromUtf8(command));
+ }
+
+ public static FlightDescriptor Path(params string[] paths)
+ {
+ return new FlightDescriptor(paths);
+ }
+
+
+ internal FlightDescriptor(Protocol.FlightDescriptor flightDescriptor)
+ {
+ if(flightDescriptor.Type != Protocol.FlightDescriptor.Types.DescriptorType.Cmd && flightDescriptor.Type != Protocol.FlightDescriptor.Types.DescriptorType.Path)
+ {
+ throw new NotSupportedException();
+ }
+ _flightDescriptor = flightDescriptor;
+ }
+
+ internal Protocol.FlightDescriptor ToProtocol()
+ {
+ return _flightDescriptor;
+ }
+
+ public bool IsCommand => _flightDescriptor.Type == Protocol.FlightDescriptor.Types.DescriptorType.Cmd;
Review comment:
This was mostly taken from the java code, I will change it to a enum instead.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo edited a comment on pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo edited a comment on pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#issuecomment-731658726
@eerhardt first of all thank you so much for your support and feedback and sorry for this wall of text. Based on your last feedback that the API is hard to change etc in the future, I really took a long sitting to go through all the classes etc, and found some issues that I really needed to address.
This resulted in a small overhaul on the structure etc. I also went through so the gRPC schema information is actually exposed to the user as well. This also resulted in some rewrites. The feedback you have given has been exactly what I needed to come to the conclusions etc on structure, so I am once again extremely thankful for all the feedback you have given.
This ofcourse increased the scope of the PR a bit again, but I hope it helps give a more real look and feel on how the framework will behave, and if the implemented design actually works.
Here comes information on the work that has been done, and also some checklists to see that all the required information is exposed etc, and why some classes are public.
# Major changes
* All exposed classes start with prefix "Flight".
* Generated code is now internal.
* New project Apache.Arrow.Flight.AspNetCore, required since generated code is internal, this project references grpc.aspnetcore.server which is netcoreapp3.1 or net5.0 only, and contains extensions only to add flight support for aspnetcore projects.
* All flight client methods now follows gRPC API more closely, allows reading response headers for all calls.
* Exposing application metadata required rewriting a bit of the logic in client and servers, read more in its chapter.
* Moved client specific classes under .Client namespace.
* Moved server specific classes under .Server namespace.
* Internal classes are now under the .Internal namespace.
* Only mutual used classes are now in the .Flight namespace.
# Minor changes
* Removed multi property exposure of bytestring, to only bytestring.
* Exposed TotalBytes and TotalRecords in FlightInfo.
* Moved public abstract reader, writer classes to internal namespace since they cant be used by the user.
* Added FlightServerRecordBatchStreamReader to expose FlightDescriptor only to the server since the client wont use it.
* Added FlightClientRecordBatchStreamReader which just extends FlightRecordBatchStreamReader which is now abstract.
* StreamWriter made internal
* PutResult does not expose ArrowBuffer (was taken from java project), instead it exposes bytestring like the other classes.
This was done so the user can choose what type of metadata to return, and also follow more closely the other classes.
* Added public static Empty on PutResult, can be useful for implementations when metadata is not used, and empty put result should be returned.
* Added a new test case to get metadata.
* Added a new test case to put metadata.
* Added a new test case for get schema.
* Added a new test case for DoAction.
* Added a new test case for ListFlights.
Test coverage is now above 80% on the non generated code.
# Application metadata addition
To enable users to write and read application metadata, the class FlightRecordBatchStreamingCall was added that
still follow gRPC look and feel, but exposes FlightRecordBatchStreamReader as response stream instead where the user can
get application metadata.
Application metadata can be read with each record batch, it is implemented as a list since hypothetically metadata can
be sent with the schema aswell. Since the packages sent follow this pattern at the moment:
Schema -> record batch -> record batch -> ... -> done
So the first message could contains metadata as well. When a user gets a new record batch the metadata is cleared similar
to how the java implementation is done.
For writing, this is done by an additional method in FlightRecordBatchStreamWriter which looks like this:
```
public Task WriteAsync(RecordBatch message, ByteString applicationMetadata);
```
# gRPC support
Here is a check on what gRPC methods and properties are exposed and can be read/used by a user of the framework, mostly to check that everything that is required to be exposed are exposed at this time.
## gRPC methods exposed or not exposed to the user
* ListFlights - Exposed
* GetFlightInfo - Exposed
* GetSchema - Exposed
* DoGet - Exposed
* DoPut - Exposed
* DoAction - Exposed
* ListActions -Exposed
* **Handshake - Not exposed**
* **DoExchange - Not exposed**
## gRPC properties exposed or not exposed to the user
### HandshakeRequest
**Handshake is not exposed at all**
* **protocol_version - not exposed**
* **payload - not exposed**
### HandshakeResponse
**Handshake is not exposed at all**
* **protocol_version - not exposed**
* **payload - not exposed**
### BasicAuth
**Basic auth is not exposed at all**
* **username - not exposed**
* **password - not exposed**
### ActionType
* Type - exposed
* Description - exposed
### Criteria
* Expression - exposed
### Action
* Type - exposed
* Body - exposed
### Result
* Body - exposed
### SchemaResult
*Schema result is not mapped to its own type, but returns a schema directly*
* Schema - exposed
### FlightDescriptor
* Type - exposed
* Path - exposed
* Cmd - exposed
### FlightInfo
* Schema - exposed
* FlightDescriptor - exposed
* endpoints - exposed
* TotalRecords - exposed
* TotalBytes - exposed
### FlightEndpoint
* Ticket - exposed
* Locations - exposed
### Location
* Uri - exposed
### Ticket
* Ticket - exposed
### FlightData
Flight data is not exposed as a class, but the data is exposed in getStream, startPut for client, and DoGet and DoPut for server.
* **FlightDescriptor**
* **client can only send flight descriptor for flight data**
* **server can only read for flight data**
* Desc: *The descriptor of the data. This is only relevant when a client is starting a new DoPut stream.*
* **Data header - used to read header of message, ex: schema, record batch message etc, not explicitly exposed and used internally only**
* Application Metadata
* Client can write metadata through *ClientRecordBatchStreamWriter*
* Client can read metadata through *FlightRecordBatchStreamReader*
* Server can write metadata through FlightServerRecordBatchStreamWriter
* Server can read metadata through *FlightRecordBatchStreamReader*
* **Data body - same as data header, exposure is done through RecordBatch**
### PutResult
* Application metadata - exposed
# Classes public or internal
Here is a list of classes and if they are public or internal, and motivation on why they are public.
* FlightRecordBatchStreamingCall - public, required since it exposes FlightRecordBatchStreamReader which is required to read metadata.
* FlightRecordBatchStreamReader - public, required to allow read on schema, flight descriptor, application metadata
* RecordBatcReaderImplementation - internal
* FlightClientRecordBatchStreamWriter - public, implements CompleteAsync for clients, extends FlightRecordBatchStreamWriter wich allows user to write application metadata.
* FlightDataStream - internal
* FlightRecordBatchDuplexStreamingCall - public, exposes FlightClientRecordBatchStreamWriter which is required to write application metadata.
* FlightRecordBatchStreamWriter - public abstract with private protected constructor, client and server implementations extend this one.
* FlightServerRecordBatchStreamWriter - public, extends FlightRecordBatchStreamWriter which allows user to write application metadata.
* SchemaWriter - internal
* FlightAction - public, allows user to read/write type, and body in Actions.
* FlightActionType - public, allows user to read/write action types with type and description from ListActions.
* FlightClient - public, allows user to call all the different endpoints.
* FlightCriteria - public, exposes Expression that servers can implement to filter result from ListFlights.
* FlightDescriptor - public, exposes DescriptorType, Paths and Cmd for user.
* FlightDescriptorType - public, contains descriptor types.
* FlightEndpoint - public, exposes flight ticket and flight locations.
* FlightInfo - public, exposes flight descriptor, schema, total bytes, total records and endpoints.
* FlightLocation - public, exposes uri to the user.
* FlightMessageSerializer - internal
* FlightPutResult - public, recieved when doing doPut, exposes application metadata.
* FlightResult - public, contains body from DoAction calls.
* FlightServerImplementation - internal, forced internal from putting generated code to internal.
* FlightTicket - public, exposes the bytestring from the ticket to the user.
* IFlightServer - public, interface to implement a flight server.
* StreamReader - internal
* StreamWriter - internal
* FlightIEndpointRouteBuilderExtensions - public, allows user to map the flight endpoint in asp net core.
* FlightIGrpcServerBuilderExtensions - public, allows the user to add their implementation of IFlightServer in a nicer way.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r527990761
##########
File path: csharp/src/Apache.Arrow.Flight/StreamWriter.cs
##########
@@ -0,0 +1,51 @@
+// 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.Collections.Generic;
+using System.Text;
+using System.Threading.Tasks;
+using Grpc.Core;
+
+namespace Apache.Arrow.Flight
+{
+ public class StreamWriter<TIn, TOut> : IAsyncStreamWriter<TIn>
Review comment:
Does this need to be public? If so, we need to give it a different name, since `StreamWriter` is a common .NET type that we wouldn't want exposed from the Arrow library.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#issuecomment-731202315
@Ulimo - how much of this is expected to work end-to-end? I've been trying to use it against an example server I found in the repo: https://github.com/apache/arrow/blob/master/python/examples/flight/server.py.
However, I am not able to successfully call anything against the server. Here is my test program:
```C#
static async Task Main(string[] args)
{
GrpcChannel channel = GrpcChannel.ForAddress("http://localhost:5005");
FlightClient flightClient = new FlightClient(channel);
var actions = flightClient.ListActions();
while (await actions.MoveNext(default))
{
Console.WriteLine(actions.Current);
}
System.Console.WriteLine("Done");
}
```
And I get this exception:
```
Unhandled exception. Grpc.Core.RpcException: Status(StatusCode="Internal", Detail="Error starting gRPC call. HttpRequestException: An error occurred while sending the request. IOException: The response ended prematurely.", DebugException="System.Net.Http.HttpRequestException: An error occurred while sending the request.
---> System.IO.IOException: The response ended prematurely.
at System.Net.Http.HttpConnection.FillAsync()
at System.Net.Http.HttpConnection.ReadNextResponseHeaderLineAsync(Boolean foldedHeadersAllowed)
at System.Net.Http.HttpConnection.SendAsyncCore(HttpRequestMessage request, CancellationToken cancellationToken)
--- End of inner exception stack trace ---
at System.Net.Http.HttpConnection.SendAsyncCore(HttpRequestMessage request, CancellationToken cancellationToken)
at System.Net.Http.HttpConnectionPool.SendWithNtConnectionAuthAsync(HttpConnection connection, HttpRequestMessage request, Boolean doRequestAuth, CancellationToken cancellationToken)
at System.Net.Http.HttpConnectionPool.SendWithRetryAsync(HttpRequestMessage request, Boolean doRequestAuth, CancellationToken cancellationToken)
at System.Net.Http.RedirectHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
at Grpc.Net.Client.Internal.GrpcCall`2.RunCall(HttpRequestMessage request, Nullable`1 timeout)")
at Grpc.Net.Client.Internal.HttpContentClientStreamReader`2.MoveNextCore(CancellationToken cancellationToken)
at Apache.Arrow.Flight.StreamReader`2.MoveNext(CancellationToken cancellationToken) in /Users/eerhardt/git/arrow/csharp/src/Apache.Arrow.Flight/StreamReader.cs:line 46
at ArrowFlightTest.Program.Main(String[] args) in /Users/eerhardt/DotNetTest/ArrowFlightTest/Program.cs:line 17
at ArrowFlightTest.Program.<Main>(String[] args)
```
One part that confuses me is the example server says the address is `grpc+tcp://localhost:5005`, but the .NET gRPC client throws with that address saying only `http` and `https` schemes are supported. So that's why I used `http://localhost:5005`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525576663
##########
File path: csharp/test/Apache.Arrow.Flight.TestWeb/Apache.Arrow.Flight.TestWeb.csproj
##########
@@ -0,0 +1,16 @@
+<Project Sdk="Microsoft.NET.Sdk.Web">
+
+ <PropertyGroup>
+ <TargetFramework>netcoreapp3.0</TargetFramework>
+ </PropertyGroup>
+
+ <ItemGroup>
+ <PackageReference Include="Grpc" Version="2.33.1" />
Review comment:
Removed it now, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r529820364
##########
File path: csharp/src/Apache.Arrow.Flight/Server/IFlightServer.cs
##########
@@ -0,0 +1,40 @@
+// 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.Collections.Generic;
+using System.Text;
+using System.Threading.Tasks;
+using Grpc.Core;
+
+namespace Apache.Arrow.Flight.Server
+{
+ public interface IFlightServer
Review comment:
What do you think about changing this to an `abstract class` instead, where all the base `virtual` methods just `throw new NotSupportedException()`?
The advantage is that:
1. If a service doesn't want to support something, they don't need to implement the method.
2. (more importantly) If/when new methods get added, we can add them to the base class without a binary breaking 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] JamesNK commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
JamesNK commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525552375
##########
File path: csharp/src/Apache.Arrow.Flight/Generated/Flight.cs
##########
@@ -0,0 +1,3536 @@
+// <auto-generated>
Review comment:
I think `<EmbedUntrackedSources>true</EmbedUntrackedSources>` in the csproj will fix 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r526101991
##########
File path: .github/workflows/csharp.yml
##########
@@ -38,8 +38,12 @@ jobs:
strategy:
fail-fast: false
matrix:
- dotnet: [2.2.103]
+ dotnet: ['3.0']
steps:
+ - name: Setup .net core 2.2
+ uses: actions/setup-dotnet@v1
+ with:
+ dotnet-version: '2.2.103'
Review comment:
This was changed in #8702 so it can be tested/approved seperately.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#issuecomment-733891188
@eerhardt I just have one question, getting this out as a preview nuget package soonish, is that possible?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#issuecomment-733176122
> but I think I overspecified it for RecordBatch, and there will be trouble if we add dictionary support etc.
I think this is OK, for now. This is just the initial impementation. The Flight API won't necessarily be "stable" in its first release - even in the C++/Python APIs it isn't stable yet, as far as I understand. There are probably many more places that will need to change to support dictionaries in the future.
Regarding what I said last week about being hard to change later - that was mostly around the concept that it is easier to hide things now, and expose them later if needed than it is to expose things now and hide them later.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r529357556
##########
File path: csharp/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs
##########
@@ -343,7 +343,7 @@ private protected void WriteRecordBatchInternal(RecordBatch recordBatch)
FinishedWritingRecordBatch(bodyLength + bodyPaddingLength, metadataLength);
}
- private Tuple<ArrowRecordBatchFlatBufferBuilder, VectorOffset> PreparingWritingRecordBatch(RecordBatch recordBatch)
+ private protected Tuple<ArrowRecordBatchFlatBufferBuilder, VectorOffset> PreparingWritingRecordBatch(RecordBatch recordBatch)
Review comment:
reverted 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r527927422
##########
File path: csharp/src/Apache.Arrow.Flight/Ticket.cs
##########
@@ -0,0 +1,60 @@
+// 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.Collections.Generic;
+using System.Text;
+using Google.Protobuf;
+
+namespace Apache.Arrow.Flight
+{
+ public class Ticket
+ {
+ private readonly Protocol.Ticket _ticket;
+ internal Ticket(Protocol.Ticket ticket)
+ {
+ _ticket = ticket;
+ }
+
+ public Ticket(ByteString ticket)
+ {
+ _ticket = new Protocol.Ticket()
+ {
+ Ticket_ = ticket
+ };
+ }
+
+ public Ticket(string ticket)
+ : this(ByteString.CopyFromUtf8(ticket))
+ {
+ }
+
+ public Ticket(byte[] bytes)
+ : this(ByteString.CopyFrom(bytes))
+ {
+ }
+
+ public string TicketString => _ticket.Ticket_.ToStringUtf8();
+
+ public ByteString TicketByteString => _ticket.Ticket_;
+
+ public byte[] TicketBytes => _ticket.Ticket_.ToByteArray();
Review comment:
Absolutely, great feedback, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo edited a comment on pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo edited a comment on pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#issuecomment-731254431
@eerhardt I tested it against that solution myself, I will check why its not working.
I would love to have a docker container of the java application and use testcontainers so we have unit tests that we are compliant with the java stuff. But that would require adding some more in the build process itself to build a container of the java stuff. Maybe in a future PR?
the dot net grpc stuff only accepts http/https thats true I think a utility class or something to change from grpc+tcp to http/https is required. I also hoped that could come in a future PR.
**EDIT**: I missed your resolved comment
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt edited a comment on pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt edited a comment on pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#issuecomment-731202315
UPDATE: I resolved the below issue with this answer: https://stackoverflow.com/a/58053460/3680432
@Ulimo - how much of this is expected to work end-to-end? I've been trying to use it against an example server I found in the repo: https://github.com/apache/arrow/blob/master/python/examples/flight/server.py.
However, I am not able to successfully call anything against the server. Here is my test program:
```C#
static async Task Main(string[] args)
{
GrpcChannel channel = GrpcChannel.ForAddress("http://localhost:5005");
FlightClient flightClient = new FlightClient(channel);
var actions = flightClient.ListActions();
while (await actions.MoveNext(default))
{
Console.WriteLine(actions.Current);
}
System.Console.WriteLine("Done");
}
```
And I get this exception:
```
Unhandled exception. Grpc.Core.RpcException: Status(StatusCode="Internal", Detail="Error starting gRPC call. HttpRequestException: An error occurred while sending the request. IOException: The response ended prematurely.", DebugException="System.Net.Http.HttpRequestException: An error occurred while sending the request.
---> System.IO.IOException: The response ended prematurely.
at System.Net.Http.HttpConnection.FillAsync()
at System.Net.Http.HttpConnection.ReadNextResponseHeaderLineAsync(Boolean foldedHeadersAllowed)
at System.Net.Http.HttpConnection.SendAsyncCore(HttpRequestMessage request, CancellationToken cancellationToken)
--- End of inner exception stack trace ---
at System.Net.Http.HttpConnection.SendAsyncCore(HttpRequestMessage request, CancellationToken cancellationToken)
at System.Net.Http.HttpConnectionPool.SendWithNtConnectionAuthAsync(HttpConnection connection, HttpRequestMessage request, Boolean doRequestAuth, CancellationToken cancellationToken)
at System.Net.Http.HttpConnectionPool.SendWithRetryAsync(HttpRequestMessage request, Boolean doRequestAuth, CancellationToken cancellationToken)
at System.Net.Http.RedirectHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
at Grpc.Net.Client.Internal.GrpcCall`2.RunCall(HttpRequestMessage request, Nullable`1 timeout)")
at Grpc.Net.Client.Internal.HttpContentClientStreamReader`2.MoveNextCore(CancellationToken cancellationToken)
at Apache.Arrow.Flight.StreamReader`2.MoveNext(CancellationToken cancellationToken) in /Users/eerhardt/git/arrow/csharp/src/Apache.Arrow.Flight/StreamReader.cs:line 46
at ArrowFlightTest.Program.Main(String[] args) in /Users/eerhardt/DotNetTest/ArrowFlightTest/Program.cs:line 17
at ArrowFlightTest.Program.<Main>(String[] args)
```
One part that confuses me is the example server says the address is `grpc+tcp://localhost:5005`, but the .NET gRPC client throws with that address saying only `http` and `https` schemes are supported. So that's why I used `http://localhost:5005`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r526016969
##########
File path: csharp/src/Apache.Arrow.Flight/Reader/RecordBatcReaderImplementation.cs
##########
@@ -0,0 +1,110 @@
+// 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.Collections.Generic;
+using System.Text;
+using System.Threading;
+using System.Threading.Tasks;
+using Apache.Arrow.Flatbuf;
+using Apache.Arrow.Ipc;
+using Grpc.Core;
+
+namespace Apache.Arrow.Flight
+{
+ internal class RecordBatcReaderImplementation : ArrowReaderImplementation
+ {
+ private readonly IAsyncStreamReader<Protocol.FlightData> _flightDataStream;
+ private FlightDescriptor _flightDescriptor;
+
+ public RecordBatcReaderImplementation(IAsyncStreamReader<Protocol.FlightData> streamReader)
+ {
+ _flightDataStream = streamReader;
+ }
+
+ public override RecordBatch ReadNextRecordBatch()
+ {
+ throw new NotImplementedException();
+ }
+
+ public async ValueTask<FlightDescriptor> ReadFlightDescriptor()
+ {
+ if (!HasReadSchema)
+ {
+ await ReadSchema();
+ }
+ return _flightDescriptor;
+ }
+
+ public async ValueTask<Schema> ReadSchema()
+ {
+ if (HasReadSchema)
+ {
+ return Schema;
+ }
+
+ var moveNextResult = await _flightDataStream.MoveNext();
+
+ if (!moveNextResult)
+ {
+ throw new Exception("No records or schema in this flight");
+ }
+
+ var header = _flightDataStream.Current.DataHeader.Memory;
+ Message message = Message.GetRootAsMessage(
+ ArrowReaderImplementation.CreateByteBuffer(header));
+
+
+ if(_flightDataStream.Current.FlightDescriptor != null)
+ {
+ _flightDescriptor = new FlightDescriptor(_flightDataStream.Current.FlightDescriptor);
+ }
+
+ switch (message.HeaderType)
+ {
+ case MessageHeader.Schema:
+ Schema = FlightMessageSerializer.DecodeSchema(message.ByteBuffer);
+ break;
+ default:
+ throw new Exception($"Expected schema as the first message, but got: {message.HeaderType.ToString()}");
+ }
+ return Schema;
+ }
+
+ public override async ValueTask<RecordBatch> ReadNextRecordBatchAsync(CancellationToken cancellationToken)
+ {
+ if (!HasReadSchema)
+ {
+ await ReadSchema();
Review comment:
I added ConfigureAwait on (hopefully) all the awaits.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r529362759
##########
File path: csharp/src/Apache.Arrow.Flight/Internal/FlightRecordBatchStreamReader.cs
##########
@@ -0,0 +1,65 @@
+// 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.Collections.Generic;
+using System.Text;
+using System.Threading;
+using System.Threading.Tasks;
+using Apache.Arrow.Flatbuf;
+using Apache.Arrow.Flight.Protocol;
+using Apache.Arrow.Ipc;
+using Google.Protobuf;
+using Grpc.Core;
+
+namespace Apache.Arrow.Flight.Internal
+{
+ /// <summary>
+ /// Stream of record batches
+ ///
+ /// Use MoveNext() and Current to iterate over the batches.
+ /// There are also gRPC helper functions such as ToListAsync() etc.
+ /// </summary>
+ public abstract class FlightRecordBatchStreamReader : IAsyncStreamReader<RecordBatch>
Review comment:
Implemented IAsyncEnumerable
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525583921
##########
File path: csharp/src/Apache.Arrow.Flight/Apache.Arrow.Flight.csproj
##########
@@ -0,0 +1,28 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+ <PropertyGroup>
+ <TargetFramework>netstandard2.1</TargetFramework>
+ <LangVersion>8.0</LangVersion>
+ <EmbedUntrackedSources>true</EmbedUntrackedSources>
+ </PropertyGroup>
+
+ <ItemGroup>
+ <None Remove="Format\Flight.proto" />
Review comment:
Removed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525549849
##########
File path: csharp/src/Apache.Arrow.Flight/Generated/Flight.cs
##########
@@ -0,0 +1,3536 @@
+// <auto-generated>
Review comment:
here is the build where I saw the errors: https://github.com/apache/arrow/runs/1412215805
Error log extract:
```
2 Documents with errors:
2d1a642b553a64701a2cd2463307ee00b117363d81a8914a91a6e34b6bae0700 sha256 csharp /Users/runner/work/arrow/arrow/csharp/src/Apache.Arrow/obj/Debug/netcoreapp2.1/Format/Flight.cs
https://raw.githubusercontent.com/apache/arrow/afa5d50e94e7f75d8973aa8022502f47b93740ef/csharp/src/Apache.Arrow/obj/Debug/netcoreapp2.1/Format/Flight.cs
error: url failed NotFound: Not Found
36ececbfdcef40763e3f3effdc3fc42392fd80a6fad48bf431395d0e705d7950 sha256 csharp /Users/runner/work/arrow/arrow/csharp/src/Apache.Arrow/obj/Debug/netcoreapp2.1/Format/FlightGrpc.cs
https://raw.githubusercontent.com/apache/arrow/afa5d50e94e7f75d8973aa8022502f47b93740ef/csharp/src/Apache.Arrow/obj/Debug/netcoreapp2.1/Format/FlightGrpc.cs
error: url failed NotFound: Not Found
sourcelink test failed
```
I have not used sourcelink much myself, but it seems like it is linking it to a github file in this case, so I guessed that it is required to be checked in for it to work.
That atleast solved the issue of building (but I know that it is a very ugly fix/workaround), I searched a bit if I can change the build process is any way. But could not find anything yet.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525941887
##########
File path: .github/workflows/csharp.yml
##########
@@ -38,8 +38,12 @@ jobs:
strategy:
fail-fast: false
matrix:
- dotnet: [2.2.103]
+ dotnet: ['3.0']
Review comment:
I added #8702 which contains net core version upgrade, C# 8.0 and embed untracked sources.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#issuecomment-767155085
FYI - @Ulimo, the `3.0.0` nuget packages have been released. You can see them at:
https://www.nuget.org/packages/Apache.Arrow.Flight/
https://www.nuget.org/packages/Apache.Arrow.Flight.AspNetCore/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r528265599
##########
File path: csharp/src/Apache.Arrow.Flight/FlightClient.cs
##########
@@ -0,0 +1,90 @@
+// 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.Collections.Generic;
+using System.Threading.Tasks;
+using Apache.Arrow.Flight.Protocol;
+using Apache.Arrow.Flight.Writer;
+using Grpc.Core;
+using Grpc.Net.Client;
+
+namespace Apache.Arrow.Flight
+{
+ public class FlightClient
+ {
+ private readonly FlightService.FlightServiceClient _client;
+ public FlightClient(GrpcChannel grpcChannel)
+ {
+ _client = new FlightService.FlightServiceClient(grpcChannel);
+ }
+
+ public IAsyncStreamReader<FlightInfo> ListFlights(Criteria criteria = null, Metadata headers = null)
+ {
+ if(criteria == null)
+ {
+ criteria = new Criteria();
Review comment:
Fixed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r527932397
##########
File path: csharp/src/Apache.Arrow.Flight/Action.cs
##########
@@ -0,0 +1,71 @@
+// 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.Collections.Generic;
+using System.Text;
+using Google.Protobuf;
+
+namespace Apache.Arrow.Flight
+{
+ public class Action
Review comment:
This is kind of an unfortunate name since it conflicts with `System.Action`. Any time someone tries using this class (or System.Action) and they have both namespaces in their `usings`, they will get:
```
Severity Code Description Project File Line Suppression State
Error CS0104 'Action' is an ambiguous reference between 'Apache.Arrow.Flight.Action' and 'System.Action' ArrowFlightTest F:\LinuxShare\ArrowFlight\Program.cs 15 Active
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r530231457
##########
File path: csharp/src/Apache.Arrow.Flight/Server/IFlightServer.cs
##########
@@ -0,0 +1,40 @@
+// 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.Collections.Generic;
+using System.Text;
+using System.Threading.Tasks;
+using Grpc.Core;
+
+namespace Apache.Arrow.Flight.Server
+{
+ public interface IFlightServer
Review comment:
Good suggestion, changed to an abstract class and virtual methods.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] JamesNK commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
JamesNK commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525546827
##########
File path: csharp/src/Apache.Arrow.Flight/Generated/Flight.cs
##########
@@ -0,0 +1,3536 @@
+// <auto-generated>
Review comment:
I haven't heard of this problem before.
Could you provide some more detail about how didn't it work?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r529298798
##########
File path: csharp/src/Apache.Arrow.Flight/FlightDescriptor.cs
##########
@@ -0,0 +1,102 @@
+// 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.Collections.Generic;
+using System.Linq;
+using System.Text;
+using Google.Protobuf;
+
+namespace Apache.Arrow.Flight
+{
+ public class FlightDescriptor
+ {
+ private readonly Protocol.FlightDescriptor _flightDescriptor;
+
+ private FlightDescriptor(ByteString command)
+ {
+ _flightDescriptor = new Protocol.FlightDescriptor()
+ {
+ Cmd = command,
+ Type = Protocol.FlightDescriptor.Types.DescriptorType.Cmd
+ };
+ }
+
+ private FlightDescriptor(params string[] paths)
+ {
+ _flightDescriptor = new Protocol.FlightDescriptor()
+ {
+ Type = Protocol.FlightDescriptor.Types.DescriptorType.Path
+ };
+
+ foreach(var path in paths)
+ {
+ _flightDescriptor.Path.Add(path);
+ }
+ }
+
+
+ public static FlightDescriptor Command(byte[] command)
+ {
+ return new FlightDescriptor(ByteString.CopyFrom(command));
+ }
+
+ public static FlightDescriptor Command(string command)
+ {
+ return new FlightDescriptor(ByteString.CopyFromUtf8(command));
+ }
+
+ public static FlightDescriptor Path(params string[] paths)
+ {
+ return new FlightDescriptor(paths);
+ }
+
+
+ internal FlightDescriptor(Protocol.FlightDescriptor flightDescriptor)
+ {
+ if(flightDescriptor.Type != Protocol.FlightDescriptor.Types.DescriptorType.Cmd && flightDescriptor.Type != Protocol.FlightDescriptor.Types.DescriptorType.Path)
+ {
+ throw new NotSupportedException();
+ }
+ _flightDescriptor = flightDescriptor;
+ }
+
+ internal Protocol.FlightDescriptor ToProtocol()
+ {
+ return _flightDescriptor;
+ }
+
+ public FlightDescriptorType Type => (FlightDescriptorType)_flightDescriptor.Type;
+
+ public IEnumerable<string> Paths => _flightDescriptor.Path;
+
+ public ByteString Cmd => _flightDescriptor.Cmd;
Review comment:
Had to rename the static create methods to CreatePathDescriptor and CreateCommandDescriptor since Command would collide with it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r528209850
##########
File path: csharp/src/Apache.Arrow.Flight/Action.cs
##########
@@ -0,0 +1,71 @@
+// 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.Collections.Generic;
+using System.Text;
+using Google.Protobuf;
+
+namespace Apache.Arrow.Flight
+{
+ public class Action
Review comment:
That makes sense 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r526102674
##########
File path: csharp/test/Apache.Arrow.Flight.TestWeb/Apache.Arrow.Flight.TestWeb.csproj
##########
@@ -0,0 +1,16 @@
+<Project Sdk="Microsoft.NET.Sdk.Web">
+
+ <PropertyGroup>
+ <TargetFramework>netcoreapp3.0</TargetFramework>
Review comment:
This was changed in #8702 so it can be tested/approved seperately.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo edited a comment on pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo edited a comment on pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#issuecomment-731658726
@eerhardt first of all thank you so much for your support and feedback and sorry for this wall of text. Based on your last feedback that the API is hard to change etc in the future, I really took a long sitting to go through all the classes etc, and found some issues that I really needed to address.
This resulted in a small overhaul on the structure etc. I also went through so the gRPC schema information is actually exposed to the user as well. This also resulted in some rewrites. The feedback you have given has been exactly what I needed to come to the conclusions etc on structure, so I am once again extremely thankful for all the feedback you have given.
This ofcourse increased the scope of the PR a bit again, but I hope it helps give a more real look and feel on how the framework will behave, and if the implemented design actually works.
Here comes information on the work that has been done, and also some checklists to see that all the required information is exposed etc, and why some classes are public.
# Major changes
* All exposed classes start with prefix "Flight".
* Generated code is now internal.
* New project Apache.Arrow.Flight.AspNetCore, required since generated code is internal, this project references grpc.aspnetcore.server which is netcoreapp3.1 or net5.0 only, and contains extensions only to add flight support for aspnetcore projects.
* All flight client methods now follows gRPC API more closely, allows reading response headers for all calls.
* Exposing application metadata required rewriting a bit of the logic in client and servers, read more in its section.
* Moved client specific classes under .Client namespace.
* Moved server specific classes under .Server namespace.
* Internal classes are now under the .Internal namespace.
* Only mutual used classes are now in the .Flight namespace.
# Minor changes
* Removed multi property exposure of bytestring, to only bytestring.
* Exposed TotalBytes and TotalRecords in FlightInfo.
* Moved public abstract reader, writer classes to internal namespace since they cant be used by the user.
* Added FlightServerRecordBatchStreamReader to expose FlightDescriptor only to the server since the client wont use it.
* Added FlightClientRecordBatchStreamReader which just extends FlightRecordBatchStreamReader which is now abstract.
* StreamWriter made internal
* PutResult does not expose ArrowBuffer (was taken from java project), instead it exposes bytestring like the other classes.
This was done so the user can choose what type of metadata to return, and also follow more closely the other classes.
* Added public static Empty on PutResult, can be useful for implementations when metadata is not used, and empty put result should be returned.
* Added a new test case to get metadata.
* Added a new test case to put metadata.
* Added a new test case for get schema.
* Added a new test case for DoAction.
* Added a new test case for ListFlights.
Test coverage is now above 80% on the non generated code.
# Application metadata addition
To enable users to write and read application metadata, the class FlightRecordBatchStreamingCall was added that
still follow gRPC look and feel, but exposes FlightRecordBatchStreamReader as response stream instead where the user can
get application metadata.
Application metadata can be read with each record batch, it is implemented as a list since hypothetically metadata can
be sent with the schema aswell. Since the packages sent follow this pattern at the moment:
Schema -> record batch -> record batch -> ... -> done
So the first message could contains metadata as well. When a user gets a new record batch the metadata is cleared similar
to how the java implementation is done.
For writing, this is done by an additional method in FlightRecordBatchStreamWriter which looks like this:
```
public Task WriteAsync(RecordBatch message, ByteString applicationMetadata);
```
# gRPC support
Here is a check on what gRPC methods and properties are exposed and can be read/used by a user of the framework, mostly to check that everything that is required to be exposed are exposed at this time.
## gRPC methods exposed or not exposed to the user
* ListFlights - Exposed
* GetFlightInfo - Exposed
* GetSchema - Exposed
* DoGet - Exposed
* DoPut - Exposed
* DoAction - Exposed
* ListActions -Exposed
* **Handshake - Not exposed**
* **DoExchange - Not exposed**
## gRPC properties exposed or not exposed to the user
### HandshakeRequest
**Handshake is not exposed at all**
* **protocol_version - not exposed**
* **payload - not exposed**
### HandshakeResponse
**Handshake is not exposed at all**
* **protocol_version - not exposed**
* **payload - not exposed**
### BasicAuth
**Basic auth is not exposed at all**
* **username - not exposed**
* **password - not exposed**
### ActionType
* Type - exposed
* Description - exposed
### Criteria
* Expression - exposed
### Action
* Type - exposed
* Body - exposed
### Result
* Body - exposed
### SchemaResult
*Schema result is not mapped to its own type, but returns a schema directly*
* Schema - exposed
### FlightDescriptor
* Type - exposed
* Path - exposed
* Cmd - exposed
### FlightInfo
* Schema - exposed
* FlightDescriptor - exposed
* endpoints - exposed
* TotalRecords - exposed
* TotalBytes - exposed
### FlightEndpoint
* Ticket - exposed
* Locations - exposed
### Location
* Uri - exposed
### Ticket
* Ticket - exposed
### FlightData
Flight data is not exposed as a class, but the data is exposed in getStream, startPut for client, and DoGet and DoPut for server.
* **FlightDescriptor**
* **client can only send flight descriptor for flight data**
* **server can only read for flight data**
* Desc: *The descriptor of the data. This is only relevant when a client is starting a new DoPut stream.*
* **Data header - used to read header of message, ex: schema, record batch message etc, not explicitly exposed and used internally only**
* Application Metadata
* Client can write metadata through *ClientRecordBatchStreamWriter*
* Client can read metadata through *FlightRecordBatchStreamReader*
* Server can write metadata through FlightServerRecordBatchStreamWriter
* Server can read metadata through *FlightRecordBatchStreamReader*
* **Data body - same as data header, exposure is done through RecordBatch**
### PutResult
* Application metadata - exposed
# Classes public or internal
Here is a list of classes and if they are public or internal, and motivation on why they are public.
* FlightRecordBatchStreamingCall - public, required since it exposes FlightRecordBatchStreamReader which is required to read metadata.
* FlightRecordBatchStreamReader - public, required to allow read on schema, flight descriptor, application metadata
* RecordBatcReaderImplementation - internal
* FlightClientRecordBatchStreamWriter - public, implements CompleteAsync for clients, extends FlightRecordBatchStreamWriter wich allows user to write application metadata.
* FlightDataStream - internal
* FlightRecordBatchDuplexStreamingCall - public, exposes FlightClientRecordBatchStreamWriter which is required to write application metadata.
* FlightRecordBatchStreamWriter - public abstract with private protected constructor, client and server implementations extend this one.
* FlightServerRecordBatchStreamWriter - public, extends FlightRecordBatchStreamWriter which allows user to write application metadata.
* SchemaWriter - internal
* FlightAction - public, allows user to read/write type, and body in Actions.
* FlightActionType - public, allows user to read/write action types with type and description from ListActions.
* FlightClient - public, allows user to call all the different endpoints.
* FlightCriteria - public, exposes Expression that servers can implement to filter result from ListFlights.
* FlightDescriptor - public, exposes DescriptorType, Paths and Cmd for user.
* FlightDescriptorType - public, contains descriptor types.
* FlightEndpoint - public, exposes flight ticket and flight locations.
* FlightInfo - public, exposes flight descriptor, schema, total bytes, total records and endpoints.
* FlightLocation - public, exposes uri to the user.
* FlightMessageSerializer - internal
* FlightPutResult - public, recieved when doing doPut, exposes application metadata.
* FlightResult - public, contains body from DoAction calls.
* FlightServerImplementation - internal, forced internal from putting generated code to internal.
* FlightTicket - public, exposes the bytestring from the ticket to the user.
* IFlightServer - public, interface to implement a flight server.
* StreamReader - internal
* StreamWriter - internal
* FlightIEndpointRouteBuilderExtensions - public, allows user to map the flight endpoint in asp net core.
* FlightIGrpcServerBuilderExtensions - public, allows the user to add their implementation of IFlightServer in a nicer way.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r527921442
##########
File path: csharp/src/Apache.Arrow.Flight/Apache.Arrow.Flight.csproj
##########
@@ -0,0 +1,21 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+ <PropertyGroup>
+ <TargetFramework>netstandard2.1</TargetFramework>
+ </PropertyGroup>
+
+ <ItemGroup>
+ <PackageReference Include="Google.Protobuf" Version="3.14.0" />
+ <PackageReference Include="Grpc.Net.Client" Version="2.33.1" />
+ <PackageReference Include="Grpc.Tools" Version="2.33.1" PrivateAssets="All" />
+ </ItemGroup>
+
+ <ItemGroup>
+ <ProjectReference Include="..\Apache.Arrow\Apache.Arrow.csproj" />
+ </ItemGroup>
+
+ <ItemGroup>
+ <Protobuf Include="..\..\..\format\Flight.proto" />
Review comment:
```suggestion
<Protobuf Include="..\..\..\format\Flight.proto" Access="internal" />
```
Let's generate the "Protocol" classes as `internal`, as we don't want public callers to use these types directly.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r528208964
##########
File path: csharp/src/Apache.Arrow.Flight/Action.cs
##########
@@ -0,0 +1,71 @@
+// 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.Collections.Generic;
+using System.Text;
+using Google.Protobuf;
+
+namespace Apache.Arrow.Flight
+{
+ public class Action
Review comment:
@eerhardt I think I will actually prefix all class names by Flight if that sounds ok to you as well. A lot of the names are quite generic such as Action, Location, Result, Criteria. There might be conflicts with other libraries for the other names as well. So I were thinking that having the same prefix gives it some kind of uniformity.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] eerhardt commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
eerhardt commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525550665
##########
File path: .github/workflows/csharp.yml
##########
@@ -38,8 +38,12 @@ jobs:
strategy:
fail-fast: false
matrix:
- dotnet: [2.2.103]
+ dotnet: ['3.0']
Review comment:
A couple thoughts:
1. It may make sense to do this in a separate change. That way we can isolate it and ensure our CI remains running correctly.
2. `3.0` is currently end of life. We should use `3.1` instead. https://dotnet.microsoft.com/platform/support/policy/dotnet-core
##########
File path: .github/workflows/csharp.yml
##########
@@ -38,8 +38,12 @@ jobs:
strategy:
fail-fast: false
matrix:
- dotnet: [2.2.103]
+ dotnet: ['3.0']
steps:
+ - name: Setup .net core 2.2
+ uses: actions/setup-dotnet@v1
+ with:
+ dotnet-version: '2.2.103'
Review comment:
Instead, we should just move all our test projects to `3.1` and no longer require this EOL version.
##########
File path: csharp/test/Apache.Arrow.Flight.TestWeb/Apache.Arrow.Flight.TestWeb.csproj
##########
@@ -0,0 +1,16 @@
+<Project Sdk="Microsoft.NET.Sdk.Web">
+
+ <PropertyGroup>
+ <TargetFramework>netcoreapp3.0</TargetFramework>
Review comment:
Let's move all the tests to `netcoreapp3.1`.
##########
File path: csharp/src/Apache.Arrow.Flight/Apache.Arrow.Flight.csproj
##########
@@ -0,0 +1,28 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+ <PropertyGroup>
+ <TargetFramework>netstandard2.1</TargetFramework>
+ <LangVersion>8.0</LangVersion>
+ <EmbedUntrackedSources>true</EmbedUntrackedSources>
+ </PropertyGroup>
+
+ <ItemGroup>
+ <None Remove="Format\Flight.proto" />
Review comment:
This shouldn't be nececessary. Just having the `<Protobuf Include="path-to\Flight.proto" />` should be enough.
##########
File path: csharp/src/Apache.Arrow.Flight/Format/Flight.proto
##########
@@ -0,0 +1,335 @@
+/*
Review comment:
This file is a duplicate of https://github.com/apache/arrow/blob/master/format/Flight.proto. We should remove this duplicate and just add the `csharp_namespace` line to the `format/Flight.proto` file.
##########
File path: csharp/src/Apache.Arrow.Flight/Apache.Arrow.Flight.csproj
##########
@@ -0,0 +1,28 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+ <PropertyGroup>
+ <TargetFramework>netstandard2.1</TargetFramework>
+ <LangVersion>8.0</LangVersion>
Review comment:
If it just works - moving this change to the Directory.Build.props would be fine with me. That way we are using the latest C# for all projects.
##########
File path: csharp/src/Apache.Arrow.Flight/Apache.Arrow.Flight.csproj
##########
@@ -0,0 +1,28 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+ <PropertyGroup>
+ <TargetFramework>netstandard2.1</TargetFramework>
+ <LangVersion>8.0</LangVersion>
+ <EmbedUntrackedSources>true</EmbedUntrackedSources>
Review comment:
I think this may make sense to move to the `csharp\Directory.Build.props` file instead.
##########
File path: csharp/src/Apache.Arrow.Flight/Reader/RecordBatcReaderImplementation.cs
##########
@@ -0,0 +1,110 @@
+// 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.Collections.Generic;
+using System.Text;
+using System.Threading;
+using System.Threading.Tasks;
+using Apache.Arrow.Flatbuf;
+using Apache.Arrow.Ipc;
+using Grpc.Core;
+
+namespace Apache.Arrow.Flight
+{
+ internal class RecordBatcReaderImplementation : ArrowReaderImplementation
+ {
+ private readonly IAsyncStreamReader<Protocol.FlightData> _flightDataStream;
+ private FlightDescriptor _flightDescriptor;
+
+ public RecordBatcReaderImplementation(IAsyncStreamReader<Protocol.FlightData> streamReader)
+ {
+ _flightDataStream = streamReader;
+ }
+
+ public override RecordBatch ReadNextRecordBatch()
+ {
+ throw new NotImplementedException();
+ }
+
+ public async ValueTask<FlightDescriptor> ReadFlightDescriptor()
+ {
+ if (!HasReadSchema)
+ {
+ await ReadSchema();
+ }
+ return _flightDescriptor;
+ }
+
+ public async ValueTask<Schema> ReadSchema()
+ {
+ if (HasReadSchema)
+ {
+ return Schema;
+ }
+
+ var moveNextResult = await _flightDataStream.MoveNext();
+
+ if (!moveNextResult)
+ {
+ throw new Exception("No records or schema in this flight");
+ }
+
+ var header = _flightDataStream.Current.DataHeader.Memory;
+ Message message = Message.GetRootAsMessage(
+ ArrowReaderImplementation.CreateByteBuffer(header));
+
+
+ if(_flightDataStream.Current.FlightDescriptor != null)
+ {
+ _flightDescriptor = new FlightDescriptor(_flightDataStream.Current.FlightDescriptor);
+ }
+
+ switch (message.HeaderType)
+ {
+ case MessageHeader.Schema:
+ Schema = FlightMessageSerializer.DecodeSchema(message.ByteBuffer);
+ break;
+ default:
+ throw new Exception($"Expected schema as the first message, but got: {message.HeaderType.ToString()}");
+ }
+ return Schema;
+ }
+
+ public override async ValueTask<RecordBatch> ReadNextRecordBatchAsync(CancellationToken cancellationToken)
+ {
+ if (!HasReadSchema)
+ {
+ await ReadSchema();
Review comment:
Since this is library code, every call to `await` should have a `.ConfigureAwait(false)` on it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r529363193
##########
File path: csharp/src/Apache.Arrow.Flight/Internal/FlightRecordBatchStreamReader.cs
##########
@@ -0,0 +1,65 @@
+// 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.Collections.Generic;
+using System.Text;
+using System.Threading;
+using System.Threading.Tasks;
+using Apache.Arrow.Flatbuf;
+using Apache.Arrow.Flight.Protocol;
+using Apache.Arrow.Ipc;
+using Google.Protobuf;
+using Grpc.Core;
+
+namespace Apache.Arrow.Flight.Internal
Review comment:
moved
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] Ulimo commented on a change in pull request #8694: ARROW-10542: [C#][Flight] Add beginning on flight code for net core
Posted by GitBox <gi...@apache.org>.
Ulimo commented on a change in pull request #8694:
URL: https://github.com/apache/arrow/pull/8694#discussion_r525583850
##########
File path: csharp/src/Apache.Arrow.Flight/Format/Flight.proto
##########
@@ -0,0 +1,335 @@
+/*
Review comment:
Removed it and added reference instead to the one you posted. Thank you for the feedback!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org