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

CrateDB connector #3

Open
wants to merge 3 commits into
base: cratedb
Choose a base branch
from
Open

CrateDB connector #3

wants to merge 3 commits into from

Conversation

proddata
Copy link

@proddata proddata commented Feb 7, 2024

Description

How Has This Been Tested?

  • Test A
  • Test B

Checklist

  • The PR is tagged with proper labels (bug, enhancement, feature, documentation)
  • I have performed a self-review of my own code
  • I have added unit tests that prove my fix is effective or that my feature works
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • If new documentation has been added, relative paths have been added to the appropriate section of docs/mint.json

cc:

Copy link

@amotl amotl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent stuff, thanks a stack. I've diverted the PR to be targeted towards the cratedb branch, so you can merge what works, and submit later fixes on behalf of subsequent PRs.

Other than this, I added a few quick suggestions on a few spots of particular interest. I did not run a thorough review yet, but this can also happen on a final review run before upstreaming all changes.

Comment on lines +66 to +70
'cratedb': [
'psycopg2==2.9.3',
'psycopg2-binary==2.9.3',
'sshtunnel==0.4.0',
],
Copy link

@amotl amotl Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have the choice, I'd go for psycopg3 right away (package name is psycopg[binary], IIRC), but sure it can happen on behalf of a later iteration, depending on your urgency to submit the patch to upstream.

mage_ai/io/cratedb.py Show resolved Hide resolved
mage_ai/io/cratedb.py Outdated Show resolved Hide resolved
@amotl amotl changed the base branch from master to cratedb February 7, 2024 15:23
Copy link

@amotl amotl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few more comments while quickly skimming the patch once more.

Comment on lines +22 to +25
```
cratedb-client==1.6.9
sentence-transformers==2.2.2
```
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe those are not needed at all?

Suggested change
```
cratedb-client==1.6.9
sentence-transformers==2.2.2
```

Comment on lines +209 to +215
dict(
block_type=BlockType.DATA_LOADER,
groups=[GROUP_DATABASES],
language=BlockLanguage.PYTHON,
name='CrateDB',
path='data_loaders/cratedb.py',
),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah wow. The framework allows database adapters to be written in other languages than Python?

Comment on lines +13 to +18
"""
Template for exporting data to a PostgreSQL database.
Specify your configuration settings in 'io_config.yaml'.

Docs: https://docs.mage.ai/design/data-loading#postgresql
"""
Copy link

@amotl amotl Feb 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstrings also need a few more adjustments, but sure all that can happen after merging to cratedb, just any time before submitting to upstream. It will probably take another while. I can also support on such details.

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

Successfully merging this pull request may close these issues.

2 participants