-
Notifications
You must be signed in to change notification settings - Fork 24
feat: destination discover PoC and adding sync modes #527
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
base: main
Are you sure you want to change the base?
Changes from all commits
4720990
0eb0713
e3baae1
98af60c
f2c993e
ff6044c
c6c8d8f
af0640c
2ff1cd9
6c0ab9f
035acd7
18d36f5
ceac06c
6f22532
8c412b4
94209aa
fa4d3fd
0fbb159
1f552d5
f9a7ed2
0f785c1
a76f424
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,11 +1,10 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# Copyright (c) 2024 Airbyte, Inc., all rights reserved. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from abc import ABC, abstractmethod | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from copy import deepcopy | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from dataclasses import InitVar, dataclass, field | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from typing import Any, List, Mapping, MutableMapping, Optional, Union | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from typing import Any, Dict, List, Mapping, MutableMapping, Optional, Union | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import dpath | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from typing_extensions import deprecated | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -16,7 +15,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from airbyte_cdk.sources.declarative.schema.schema_loader import SchemaLoader | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from airbyte_cdk.sources.declarative.transformations import RecordTransformation | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from airbyte_cdk.sources.source import ExperimentalClassWarning | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from airbyte_cdk.sources.types import Config, StreamSlice, StreamState | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from airbyte_cdk.sources.types import Config | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
AIRBYTE_DATA_TYPES: Mapping[str, MutableMapping[str, Any]] = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"string": {"type": ["null", "string"]}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -114,6 +113,38 @@ def _update_pointer( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
@deprecated("This class is experimental. Use at your own risk.", category=ExperimentalClassWarning) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
class AdditionalPropertyFieldsInferrer(ABC): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Infers additional fields to be added to each property. For example, if this inferrer returns {"toto": "tata"}, a property that would have looked like this: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
``` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"properties": { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"Id": { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"type": ["null", "string"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<...> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
``` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
... will look like this: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
``` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"properties": { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"Id": { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"type": ["null", "string"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"toto": "tata" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<...> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
``` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
@abstractmethod | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def infer(self, property_definition: MutableMapping[str, Any]) -> MutableMapping[str, Any]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Infers additional property fields from the given property definition. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
pass | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+116
to
+146
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider making the inferrer contract read-only
- def infer(self, property_definition: MutableMapping[str, Any]) -> MutableMapping[str, Any]:
+ def infer(self, property_definition: Mapping[str, Any]) -> Mapping[str, Any]: That way implementers must return a new dict, reducing side-effects – wdyt? 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
@deprecated("This class is experimental. Use at your own risk.", category=ExperimentalClassWarning) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
@dataclass | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
class DynamicSchemaLoader(SchemaLoader): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -126,6 +157,8 @@ class DynamicSchemaLoader(SchemaLoader): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
parameters: InitVar[Mapping[str, Any]] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
schema_type_identifier: SchemaTypeIdentifier | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
schema_transformations: List[RecordTransformation] = field(default_factory=lambda: []) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
additional_property_fields_inferrer: Optional[AdditionalPropertyFieldsInferrer] = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
allow_additional_properties: bool = True | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def get_json_schema(self) -> Mapping[str, Any]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -149,22 +182,26 @@ def get_json_schema(self) -> Mapping[str, Any]: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
property_definition, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self.schema_type_identifier.type_pointer, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
value.update( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self.additional_property_fields_inferrer.infer(property_definition) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if self.additional_property_fields_inferrer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
else {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
properties[key] = value | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
transformed_properties = self._transform(properties, {}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
transformed_properties = self._transform(properties) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"$schema": "https://json-schema.org/draft-07/schema#", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"type": "object", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"additionalProperties": True, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💎 Nice. Appreciate that we're following JSON Schema standards for communicating this. 👍 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"additionalProperties": self.allow_additional_properties, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"properties": transformed_properties, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def _transform( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
properties: Mapping[str, Any], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
stream_state: StreamState, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
stream_slice: Optional[StreamSlice] = None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) -> Mapping[str, Any]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
for transformation in self.schema_transformations: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
transformation.transform( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -190,7 +227,7 @@ def _get_type( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
raw_schema: MutableMapping[str, Any], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
field_type_path: Optional[List[Union[InterpolatedString, str]]], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) -> Union[Mapping[str, Any], List[Mapping[str, Any]]]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) -> Dict[str, Any]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Determines the JSON Schema type for a field, supporting nullable and combined types. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -220,7 +257,7 @@ def _get_type( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
f"Invalid data type. Available string or two items list of string. Got {mapped_field_type}." | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def _resolve_complex_type(self, complex_type: ComplexFieldType) -> Mapping[str, Any]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def _resolve_complex_type(self, complex_type: ComplexFieldType) -> Dict[str, Any]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if not complex_type.items: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return self._get_airbyte_type(complex_type.field_type) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -255,14 +292,14 @@ def _replace_type_if_not_valid( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return field_type | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
@staticmethod | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def _get_airbyte_type(field_type: str) -> MutableMapping[str, Any]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def _get_airbyte_type(field_type: str) -> Dict[str, Any]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Maps a field type to its corresponding Airbyte type definition. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if field_type not in AIRBYTE_DATA_TYPES: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
raise ValueError(f"Invalid Airbyte data type: {field_type}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return deepcopy(AIRBYTE_DATA_TYPES[field_type]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return deepcopy(AIRBYTE_DATA_TYPES[field_type]) # type: ignore # a copy of a dict should be a dict, not a MutableMapping | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def _extract_data( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,7 +72,11 @@ def _to_mapping( | |
elif isinstance(body, bytes): | ||
return json.loads(body.decode()) # type: ignore # assumes return type of Mapping[str, Any] | ||
elif isinstance(body, str): | ||
return json.loads(body) # type: ignore # assumes return type of Mapping[str, Any] | ||
try: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without this addition, depending on the order of evaluation, test |
||
return json.loads(body) # type: ignore # assumes return type of Mapping[str, Any] | ||
except json.JSONDecodeError: | ||
# one of the body is a mapping while the other isn't so comparison should fail anyway | ||
return None | ||
return None | ||
|
||
@staticmethod | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of patterns, I think my preference here and proposed best best practice (eventually) is that we make this a class method on the base connector class. I would call it "launch" and then every connector class could be able to invoke itself. In theory, all sources and destinations could share the same implementation across connectors of the same type, and unless the connector needs something special (a code smell anyway), the "cls" input arg should be all you need in order to instantiate a connector using CLI args
I don't think we need to tackle all the scope here in this PR, but your implementation is already very close to what I think is the ideal state, so I'll mention it here as a non-blocking proposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me. I started diverging a bit from the current solution because the
run_cmd
it is not static so we need to instantiate the object before calling it but the way we instantiate the destination depends on how the method of the protocal that is called (see this piece of code for an example).So do we agree that the
launch
method would be static? If so, I think we are moving the right direction. I can start adding this in later changes. If not, I'll need more information about what you had in mind.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxi297 - Difference b/w static method and class method for this use case is just the a class method knows what class it is (static methods don't get a
cls
input), and that class method implementations can be inherited by subclasses. So, yes, agreed method is static in a sense that it doesn't need the object, but in order to not need to implement on each class, I think making a class method is slightly cleaner in the long run. Sorry if I'm not good at explaining this, and please don't block on my comment - either way, it's a step in the right direction, I think, if the class knows how to instantiate itself, without needing an external class or function in order to be instantiated. 👍There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxi297 - Here's the early implementation I wrote for S3 a while back...
This wasn't a very good implementation, but you can see the generic
cls.create()
ref.Where
cls.create()
was defined as:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another problem with our current design is that we ask for config, catalog, etc in the constructor, but then pass it again during invocation of 'check', 'discover', etc. I think it would be better to not need these in the constructor at all, in which case we greatly simplify the process of creating a connector class, and we put all code that can fail in a code path that can properly message about any failures.
Again - hopefully this is helpful for long-term thinking, but I don't think it needs to block the current PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
classmethod makes sense, yes!
And I would prefer to have these as part of the constructor personally as if we don't, each method will need to instantiate the object it needs every time is called or have some kind on intelligent accessor which would check if the field has already been instantiated and if not instantiate it. It feels complex instead of just instantiating the connector properly in
launch
. WDYT?