grafana / sqlds

A package that assists writing SQL-driven datasources
Apache License 2.0
17 stars 12 forks source link

Extend the Completable interface methods #46

Closed andresmgot closed 2 years ago

andresmgot commented 2 years ago

The current method for getting a table columns looks like this:

func Columns(ctx context.Context, table string) ([]string, error)

This is not complete since the columns may depend of a schema (e.g. there can be a table foo in two schemas public and bar). But some SQL data sources doesn't have the concept of schema and forcing it may not be the best option.

Also, we have an example of a data source that the table / column resolution depends on other parameters (not SQL related). This SQL data source is a database as a service, so it also depends on the database name and the region. To satisfy this for the moment, we have added a new endpoint /columns-with-details but that's just a workaround IMO.

To satisfy all the different data sources, we will need flexible argument(s) to set the different parameters. If we agree to that we have several options for the function signature:

  1. func Columns(ctx context.Context, options map[string]string) ([]string, error)
  2. func Columns(ctx context.Context, options ...string) ([]string, error)
  3. func Columns(ctx context.Context, options interface{}) ([]string, error)

I would go with the second since it better reflect the variadic nature of the function and all the parameters we foresee are strings. Opinions?

Note that this would be a breaking change.

sunker commented 2 years ago

Yeah, I guess another options would be to explicitly specify what we know func Columns(ctx context.Context, schema, table string, extra ...string) ([]string, error) But I don't have strong opinion in this matter. Options 2 looks good to me.

andresmgot commented 2 years ago

func Columns(ctx context.Context, schema, table string, extra ...string) ([]string, error)

I didn't propose that because for example Athena, and I assume others, are schema-less and also the order of the parametes may vary. For example, from wider to narrower a datasource may want to use ctx, region, database, schema, table.

kminehart commented 2 years ago

I think a struct may be an OK substitute but I think we may run into the same problem...

var ColumnsOpts struct {
    Schema string
    Table     string
}

func Columns(ctx context.Context, opts *ColumnsOpts) ([]string, error)

I don't like option 2 because it makes it quite hard to understand when implementing the function

Option 1 and 3 don't excite me because empty interfaces are just so inconvenient to work with I try to avoid them as much as possible, but it may not be avoidable in this case. This would be a good case for generics in another language.

So I'm for either 1 or 3. Probably 1 because at the very least you can define the keys as constants or something.

andresmgot commented 2 years ago

I think a struct may be an OK substitute but I think we may run into the same problem...

yes, a typed struct would only help for adding new parameters without making a breaking change but it would still changes in the library if we want to add a parameter and we wouldn't be able to add custom (non common) parameters.

So I'm for either 1 or 3. Probably 1 because at the very least you can define the keys as constants or something.

Okay, let me try with 1.