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

Extend the Completable interface methods #46

Closed
andresmgot opened this issue Oct 5, 2021 · 4 comments · Fixed by #47
Closed

Extend the Completable interface methods #46

andresmgot opened this issue Oct 5, 2021 · 4 comments · Fixed by #47

Comments

@andresmgot
Copy link
Collaborator

andresmgot commented Oct 5, 2021

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
Copy link
Collaborator

sunker commented Oct 6, 2021

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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.

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

Successfully merging a pull request may close this issue.

3 participants