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

a simple example for C# #51

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

Conversation

tgavin-dremio
Copy link

Adding this C# example based on a few requests for this in stackoverflow. This is just an example to show connecting to Dremio and run a query.

using Apache.Arrow.Flight;
using Apache.Arrow;

namespace FlightClientExample
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, is there a reason why a namespace is needed here?

{
public static async Task Main(string[] args)
{
string host = "localhost";
Copy link
Contributor

Choose a reason for hiding this comment

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

Could these magic numbers be placed outside of the Main function like below?
const string DEFAULT_HOST = "localhost"
This would make them more easy to find and change.

Would a class also be a good way to modularise this information? There would only be one argument when passing these arguments into methods.

{
public class Program
{
public static async Task Main(string[] args)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe splitting main into many smaller methods that are all called in main could lead to more modularity and better readability.

string query = "SELECT city, loc[0] AS longitude, loc[1] AS latitude, pop, state, _id FROM Samples.\"samples.dremio.com\".\"zips.json\" LIMIT 100";
string protocol = "http";

// Parse command-line arguments to override the defaults when set
Copy link
Contributor

Choose a reason for hiding this comment

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

Parsing the command line arguments should fall into a static private method.

// Parse command-line arguments to override the defaults when set
for (int i = 0; i < args.Length-1; i++)
{
if (i % 2 == 0 && args[i].StartsWith("-"))
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic assumes that the user will use the command flags correctly. I think a more robust algorithm would be to:

  1. Have the method for parsing the command line arguments return a Dictionary <string, string> that has the keys being the command line flags and the values being the value of the flag.
  2. When creating the Dictionary, have the initial values be the default values of the flags
  3. Loop through the arguments list by each argument
  4. Have a null string lastFlag to keep track of the previous command flag
  5. Go through the following control structure for each argument:
// Skip if is null or white space
if (string.IsNullOrWhiteSpace(argument))
{
    continue;
}
// Add argument to lastFlag if starts with -
if (argument.StartsWith("-", StringComparison.Ordinal))
{
    lastFlag = argument;
}
// If lastFlag is not null, save the 
else if (lastFlag != null && optionsDic.ContainsKey(lastFlag))
{
    commandLineArguments.Add(lastFlag, argument);
    lastFlag = null;
}
  1. Return the dictionary

}
}

// Basic auth using username and password
Copy link
Contributor

Choose a reason for hiding this comment

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

Creating a separate method for this would modularise and make the code more legible.

string encoded = System.Convert.ToBase64String(System.Text.Encoding.GetEncoding("ISO-8859-1").GetBytes(user + ":" + pass));
Console.WriteLine($"The encoded credentials: {encoded}");

// Create client
Copy link
Contributor

Choose a reason for hiding this comment

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

Creating a separate method for this would modularise and make the code more legible.

Console.WriteLine($"Connecting to: {address}");

var handler = new HttpClientHandler();
// For localhost https (TLS) endpoint testing, uncomment the following to avoid a cert error
Copy link
Contributor

Choose a reason for hiding this comment

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

Creating a separate option to disable certificate validation would be more user friendly - the control structure for the arguments list would have to be modified slightly to accept a flag with no value.


FlightClient client = new FlightClient(channel);

// Pass the query text as the Command Descriptor
Copy link
Contributor

Choose a reason for hiding this comment

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

Creating a separate method for this would modularise and make the code more legible.

@tgavin-dremio
Copy link
Author

Thanks for reviewing @ArgusLi -- I will plan to make this more than just a simple example then :-)

@tgavin-dremio
Copy link
Author

Created a number of functions and added a generic command-line args parser

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

Successfully merging this pull request may close these issues.

2 participants