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

allow to define certain destination capabilities via configuration #964

Open
rudolfix opened this issue Feb 13, 2024 · 6 comments
Open

allow to define certain destination capabilities via configuration #964

rudolfix opened this issue Feb 13, 2024 · 6 comments
Assignees
Labels
community This issue came from slack community workspace

Comments

@rudolfix
Copy link
Collaborator

rudolfix commented Feb 13, 2024

Background
Some of the destination capabilities may be changed by the user, for example:

  • default decimal, integer, text or timestamp precision
  • naming convention

ideally we would allow user to change certain via capabilities of Destination and via configuration ie

[destination.postgres.capabilities]
decimal_precision="9,3"
naming_convention="duck_case"

Requirements

    • allow the capabilities to be set via property of Destination
    • create base capabilities that can be a part of configuration with settings for which configuration make sense
    • write tests
    • document both methods in Destination docs
@rudolfix
Copy link
Collaborator Author

@steinitzu I could see you had to subclass the destination to change default decimals. why? it seems it is in the properties? so you can create postgres() instance and then change it?

@steinitzu
Copy link
Collaborator

Hey @rudolfix , yeah you could monkey-patch the property on client class and replace the factory method.

This on both job client and sql client classes is the annoying part:

class PostgresClient(InsertValuesJobClient):
    capabilities: ClassVar[DestinationCapabilitiesContext] = capabilities()

We should pass them to __init__ from factory.

@steinitzu
Copy link
Collaborator

We could just remove the capabilities property and make it a field on DestinationClientConfiguration, no?
Should be pretty minimal change and makes it editable along with other options.

@rudolfix
Copy link
Collaborator Author

yeah that would work but:

capabilities: ClassVar[DestinationCapabilitiesContext] = capabilities()

could not be a classvar anymore but should be instantiated per instance. otherwise we'll get the initial values and those will never be modified.
I do not see we use it as classvar anywhere now. we may get rid of it finally.

Also I'd just provide a few values to be configurable in DestinationClientConfiguration not everything. some of them are callables and other should never be changed
same for sql_client.

@steinitzu
Copy link
Collaborator

steinitzu commented Feb 13, 2024

Yeah I don't see why it needs to be a classvar either. Could be instance attr on client.

Totally makes sense not to have everything configurable, the escape functions, max_query_length, etc are all fixed.
Question if the changable params should be entirely separate then.
I.e. decimal_precision, etc should not be in capabilities at all but just be regular config fields with defaults, and we keep caps as a constant.

@rudolfix
Copy link
Collaborator Author

maybe config param are base configspec from which current capabilities derive? the problem I see is that capabilities derive from context already and context is exclusively injected by context config provider so it won't be taken from toml etc. but 100% that can be solved :)

@rudolfix rudolfix moved this from Todo to Planned in dlt core library Feb 19, 2024
@rudolfix rudolfix added the community This issue came from slack community workspace label Mar 5, 2024
@rudolfix rudolfix moved this from Planned to Todo in dlt core library May 13, 2024
@rudolfix rudolfix moved this from Todo to In Progress in dlt core library May 16, 2024
@rudolfix rudolfix moved this from In Progress to Planned in dlt core library Jun 2, 2024
@rudolfix rudolfix moved this from Planned to In Progress in dlt core library Jun 16, 2024
@rudolfix rudolfix self-assigned this Jun 16, 2024
@rudolfix rudolfix changed the title allow to change destination capabilities allow to define certain destination capabilities via configuration Jun 27, 2024
@rudolfix rudolfix moved this from In Progress to Todo in dlt core library Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community This issue came from slack community workspace
Projects
Status: Todo
Development

No branches or pull requests

2 participants