Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

GH-44800: [C#] Implement Flight SQL Client #44783

Open
wants to merge 79 commits into
base: main
Choose a base branch
from

Conversation

HackPoint
Copy link

@HackPoint HackPoint commented Nov 18, 2024

GH-44800: [C#] Implement Flight SQL Client

Rationale for this Change

This pull request introduces a new implementation of FlightSqlClient and PreparedStatement in C#. Previously, there was no C# client for Flight SQL, leaving a significant gap for .NET developers who wished to interact with Flight SQL servers.

The implementation aligns with the existing C++ Flight SQL client API, ensuring consistent and familiar behavior across languages and providing a robust client for the Apache Arrow ecosystem in .NET.


What's Included in this PR?

Key Features

  1. FlightSqlClient:

    • Provides query execution (ExecuteAsync, ExecuteUpdateAsync) and schema retrieval (GetCatalogsAsync, GetDbSchemasAsync, etc.).
    • Implements metadata operations for catalogs, schemas, tables, and more.
    • Fully integrated with gRPC and Apache Arrow ecosystems.
    • Supports extensibility for advanced features like transactions.
  2. PreparedStatement:

    • Implements parameterized query execution (SetParameters, ExecuteAsync, and ExecuteUpdateAsync).
    • Supports lifecycle management (CloseAsync) for effective resource handling.
    • Aligns with the prepared statement design in the C++ client.

API Consistency

This implementation mirrors the C++ Flight SQL client to ensure API alignment across supported languages:

  • Consistent naming conventions and parameter semantics.
  • Ensures .NET developers can work seamlessly with existing Flight SQL servers.

Are These Changes Tested?

Testing Overview

  1. Unit Tests:

    • Added tests for query execution and parameter binding in PreparedStatement.
    • Verified schema retrieval methods like GetCatalogsAsync and GetDbSchemasAsync.
  2. Integration Tests:

    • Tested against a live Flight SQL server to validate query execution, schema retrieval, and metadata operations.
  3. End-to-End Tests:

    • Covered real-world scenarios for parameterized updates and queries, ensuring robustness.

Example Test Cases

  • Verify that parameterized queries return correct results with valid input.
  • Ensure schema retrieval throws appropriate exceptions for invalid descriptors.
  • Validate row counts after ExecuteUpdateAsync.

Are There Any Breaking Changes?

This PR introduces new functionality and does not affect any existing features. There are no breaking changes.


Are There Any User-Facing Changes?

New Capabilities

  1. FlightSqlClient:

    • Query execution and schema retrieval for SQL queries on Flight SQL servers.
    • Metadata retrieval for catalogs, schemas, tables, and more.
  2. PreparedStatement:

    • Supports parameterized queries with proper parameter binding.
    • Provides robust lifecycle management and execution.

API Consistency

  • Aligns with the C++ Flight SQL client for interoperability and familiar API design.

Additional Notes

  • This PR does not include transaction support at this stage, as it requires additional server-side capabilities.
  • All methods follow idiomatic C# practices, including async/await for non-blocking operations.
  • Extensible for future enhancements, including advanced features like savepoints.

Resources


Feedback and Suggestions

Thank you for reviewing this contribution! Suggestions and feedback are welcome to ensure the implementation meets the project's standards and requirements.

Genady Shmunik and others added 30 commits August 5, 2024 19:14
**GetExportedKeys**
    - Retrieves a description about the foreign key columns that reference the primary key columns of the given table.
- **GetExportedKeysSchema**
    - Get the exported keys schema from the server.
- **GetImportedKeys**
    - Retrieves the foreign key columns for the given table.
- **GetImportedKeysSchema**
    - Get the imported keys schema from the server.
- **GetCrossReference**
    - Retrieves a description of the foreign key columns in the given foreign key table that reference the primary key or the columns representing a unique constraint of the parent table.
- **GetCrossReferenceSchema**
    - Get the cross reference schema from the server.
- **GetTableTypes**
    - Request a list of table types.
- **GetTableTypesSchema**
    - Get the table types schema from the server.
- **GetXdbcTypeInfo**
    - Request the information about all the data types supported.
- **GetXdbcTypeInfo (with data_type parameter)**
    - Request the information about a specific data type supported.
- **GetXdbcTypeInfoSchema**
    - Get the type info schema from the server.
- **GetSqlInfo**
    - Request a list of SQL information.
- **GetSqlInfoSchema**
# Conflicts:
#	csharp/src/Apache.Arrow.Flight/Client/FlightClient.cs
#	csharp/test/Apache.Arrow.Flight.Tests/FlightTests.cs
…htSqlServer.cs

csharp/Apache.Arrow.Flight.Sql.TestWeb/TestFlightSqlServer.cs
            var batches = flightHolder.GetRecordBatches();
            foreach (var batch in batches)
            {
                await responseStream.WriteAsync(batch.RecordBatch, batch.Metadata);
Add ConfigureAwait(false)
- PreparedStatement implementation
- SqlActions adding command requests
  -- Commit
  -- Rollback
  -- BeginTransaction
  -- CancelFlightInfo
@HackPoint HackPoint changed the title Feat/flight sql client GH-44800: [C#] Implement Flight SQL Client Nov 20, 2024
Copy link

⚠️ GitHub issue #44800 has been automatically assigned in GitHub to PR creator.

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Nov 20, 2024
@assignUser
Copy link
Member

I approved the workflows.

I don't work on C# sharp but wanted to thank you @HackPoint for such a big first-time contribution!

@HackPoint
Copy link
Author

I approved the workflows.

I don't work on C# sharp but wanted to thank you @HackPoint for such a big first-time contribution!

Can you please review the code and allow me to close the PR please?

@CurtHagenlocher
Copy link
Contributor

Can you please review the code and allow me to close the PR please?

I will review this tonight.

@kou
Copy link
Member

kou commented Nov 26, 2024

Could you revert all cpp/ changes?

@HackPoint
Copy link
Author

HackPoint commented Nov 26, 2024 via email

@HackPoint
Copy link
Author

Could you revert all cpp/ changes?

seems that it's done

HackPoint added 2 commits December 23, 2024 13:23
# Conflicts:
#	csharp/src/Apache.Arrow.Flight.Sql/Apache.Arrow.Flight.Sql.csproj
@jeremyosterhoudt
Copy link
Contributor

Any updates on this? We are also looking to use .NET with a Flight SQL client and are eager to get this functionality.

@CurtHagenlocher
Copy link
Contributor

Any updates on this? We are also looking to use .NET with a Flight SQL client and are eager to get this functionality.

I'm sorry about the delay; I was very much wanting to get the review done before today's fork but first work, then vacation, then a lengthy internet outage at home got in the way. I will definitely make time for a review later this week.

@HackPoint
Copy link
Author

Any updates on this? We are also looking to use .NET with a Flight SQL client and are eager to get this functionality.

I'm sorry about the delay; I was very much wanting to get the review done before today's fork but first work, then vacation, then a lengthy internet outage at home got in the way. I will definitely make time for a review later this week.

Hi Curt,
Is it possible to review and if approved merge my PR, it's already waiting for more than 2 months.

@CurtHagenlocher CurtHagenlocher self-assigned this Feb 8, 2025
Copy link
Contributor

@CurtHagenlocher CurtHagenlocher left a comment

Choose a reason for hiding this comment

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

Thanks again for such a comprehensive change! I'm very sorry it took me this long to get to it. I've left a bunch of feedback of various degrees of seriousness. You'll also likely need to rebase given the amount of time that has passed (and the presence of a merge conflict).

@@ -7,6 +7,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "FluentBuilderExample", "Flu
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Apache.Arrow", "..\src\Apache.Arrow\Apache.Arrow.csproj", "{1FE1DE95-FF6E-4895-82E7-909713C53524}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "FlightClientExample", "FlightClientExample\FlightClientExample.csproj", "{CBB46C39-530D-465A-9367-4E771595209A}"
EndProject
Copy link
Contributor

Choose a reason for hiding this comment

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

FlightClientExample was added to Examples.sln but its files are not part of this pull request.

@@ -0,0 +1,38 @@
using System;
Copy link
Contributor

Choose a reason for hiding this comment

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

Added files must have the Apache header. It's missing from this file as well as FlightInfoCancelResult.cs.

public class TableRef
{
public string? Catalog { get; set; }
public string DbSchema { get; set; } = null!;
Copy link
Contributor

Choose a reason for hiding this comment

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

The null! seems dodgy. As these are required to be set, consider using a pair of constructors TableRef(string, string) and TableRef(string, string, string) instead.

_transactionId = ByteString.CopyFromUtf8(transactionId);
}

public bool IsValid() => TransactionId.Length > 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a property?

}

public bool IsValid() => TransactionId.Length > 0;
public void ResetTransaction()
Copy link
Contributor

Choose a reason for hiding this comment

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

ResetTransaction doesnt appear to be used.

new StringArray.Builder().Append("John Doe").Build()
}, 1);


Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra blank line

Assert.IsType<FlightInfo>(flightInfo);
}


Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra blank line

Assert.Equal(expectedFlightInfo.Endpoints.Count, flightInfo.Endpoints.Count);
}


Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra blank line

}, 3);
}


Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra blank line

return batchBuilder.Build();
}


Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra blank line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants