-
Notifications
You must be signed in to change notification settings - Fork 881
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
fixes #12591: add BigTable #15122
fixes #12591: add BigTable #15122
Conversation
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
1. docstrings 2. tests 3. created a Row BaseModel 4. implemented a ClassConverter
The Java checkstyle failed. Please run You can install the pre-commit hooks with |
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
- removed TODO - changed headers' year to 2024 for new files - fixed typos
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
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.
Awesome @sushi30! Thanks for the great job. Left just a few comments if you have the time
|
||
# ths instances and tables are cached to avoid making redundant requests to the API. | ||
self.instances: Dict[str, Tuple[Instance]] = {} | ||
self.tables: Dict[str, Dict[str, Tuple[Table]]] = {} |
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.
nit - since this is quite a hard typing, would it make sense to have a custom pydantic model?
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.
I changed the types to be aliased so it is clear how the dictionaries are formed. If you still think a pydantic Model is better suited I am open to modifying but I need an example of how to model it either in the project or some draft.
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.
thanks, works with the aliases 👍
return cls(config, metadata) | ||
|
||
def get_configured_database(self) -> Optional[str]: | ||
return None |
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.
maybe I'm just out of context on the NoSQL common class, but a comment/docstring explaining why this is None would be great 🙏
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.
added a docstring. this is an edge case for a MultiDB source without a default DB.
project_id = self.context.database | ||
try: | ||
# the first element is a list of instances | ||
# the second element is another collection (seems empty) and I do not know what is its purpose |
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.
fair enough 😅 sometimes these client have weird things
try: | ||
instance = [ | ||
i for i in self.instances[project_id] if i.instance_id == schema_name | ||
][0] |
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.
oh this feels a bit magic
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.
abstracted it to a method. should be clearer now.
"type": "service_account", | ||
"projectId": "my-gcp-project", | ||
"privateKeyId": "private_key_id", | ||
"privateKey": "-----BEGIN RSA PRIVATE KEY-----\nMIIEpQIBAAKCAQEAw3vHG9fDIkcYB0xi2Mv4fS2gUzKR9ZRrcVNeKkqGFTT71AVB\nOzgIqYVe8b2aWODuNye6sipcrqTqOt05Esj+sxhk5McM9bE2RlxXC5QH/Bp9zxMP\n/Yksv9Ov7fdDt/loUk7sTXvI+7LDJfmRYU6MtVjyyLs7KpQIB2xBWEToU1xZY+v0\ndRC1NA+YWc+FjXbAiFAf9d4gXkYO8VmU5meixVh4C8nsjokEXk0T/HEItpZCxadk\ndZ7LKUE/HDmWCO2oNG6sCf4ET2crjSdYIfXuREopX1aQwnk7KbI4/YIdlRz1I369\nAz3+Hxlf9lLJVH3+itN4GXrR9yWWKWKDnwDPbQIDAQABAoIBAQC3X5QuTR7SN8iV\niBUtc2D84+ECSmza5shG/UJW/6N5n0Mf53ICgBS4GNEwiYCRISa0/ILIgK6CcVb7\nsuvH8F3kWNzEMui4TO0x4YsR5GH9HkioCCS224frxkLBQnL20HIIy9ok8Rpe6Zjg\nNZUnp4yczPyqSeA9l7FUbTt69uDM2Cx61m8REOpFukpnYLyZGbmNPYmikEO+rq9r\nwNID5dkSeVuQYo4MQdRavOGFUWvUYXzkEQ0A6vPyraVBfolESX8WaLNVjic7nIa3\nujdSNojnJqGJ3gslntcmN1d4JOfydc4bja4/NdNlcOHpWDGLzY1QnaDe0Koxn8sx\nLT9MVD2NAoGBAPy7r726bKVGWcwqTzUuq1OWh5c9CAc4N2zWBBldSJyUdllUq52L\nWTyva6GRoRzCcYa/dKLLSM/k4eLf9tpxeIIfTOMsvzGtbAdm257ndMXNvfYpxCfU\nK/gUFfAUGHZ3MucTHRY6DTkJg763Sf6PubA2fqv3HhVZDK/1HGDtHlTPAoGBAMYC\npdV7O7lAyXS/d9X4PQZ4BM+P8MbXEdGBbPPlzJ2YIb53TEmYfSj3z41u9+BNnhGP\n4uzUyAR/E4sxrA2+Ll1lPSCn+KY14WWiVGfWmC5j1ftdpkbrXstLN8NpNYzrKZwx\njdR0ZkwvZ8B5+kJ1hK96giwWS+SJxJR3TohcQ18DAoGAJSfmv2r//BBqtURnHrd8\nwq43wvlbC8ytAVg5hA0d1r9Q4vM6w8+vz+cuWLOTTyobDKdrG1/tlXrd5r/sh9L0\n15SIdkGm3kPTxQbPNP5sQYRs8BrV1tEvoao6S3B45DnEBwrdVN42AXOvpcNGoqE4\nuHpahyeuiY7s+ZV8lZdmxSsCgYEAolr5bpmk1rjwdfGoaKEqKGuwRiBX5DHkQkxE\n8Zayt2VOBcX7nzyRI05NuEIMrLX3rZ61CktN1aH8fF02He6aRaoE/Qm9L0tujM8V\nNi8WiLMDeR/Ifs3u4/HAv1E8v1byv0dCa7klR8J257McJ/ID4X4pzcxaXgE4ViOd\nGOHNu9ECgYEApq1zkZthEQymTUxs+lSFcubQpaXyf5ZC61cJewpWkqGDtSC+8DxE\nF/jydybWuoNHXymnvY6QywxuIooivbuib6AlgpEJeybmnWlDOZklFOD0abNZ+aNO\ndUk7XVGffCakXQ0jp1kmZA4lGsYK1h5dEU5DgXqu4UYJ88Vttax2W+Y=\n-----END RSA PRIVATE KEY-----\n", |
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.
I hope this is a made-up key 😬
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.
it's a real key. but it doesnt open aynthing... it has to be real otherwise the model fails on validation.
1. added missing docstrings. 2. abstracted the _find_instance method. 3. aliased the IDs used in the BigTable connection
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
Quality Gate passed for 'open-metadata-ui'Issues Measures |
Quality Gate passed for 'open-metadata-ingestion'Issues Measures |
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.
Thanks @sushi30, congrats on the first connector! LGTM
Describe your changes:
Fixes #12591. Implements the BigTable metadata connector.
Documentation will be submitted in a separate PR.
Summary
metadata.py
,connection.py
,models.py`.bytes
type support fordatalake_utils
.Type of change:
Checklist:
Fixes <issue-number>: <short explanation>