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

add WriteFile command #89

Merged
merged 13 commits into from
Apr 11, 2023
Merged

add WriteFile command #89

merged 13 commits into from
Apr 11, 2023

Conversation

leo-schick
Copy link
Member

Add a command to write a sql query result to a file in a specific format.

@leo-schick leo-schick changed the title add write file command add WriteFile command Sep 29, 2022
@leo-schick leo-schick force-pushed the add-write-file-command branch 4 times, most recently from a757ced to b8d1bae Compare February 10, 2023 11:02
@leo-schick leo-schick force-pushed the add-write-file-command branch 2 times, most recently from ad32613 to dcd67fd Compare February 26, 2023 09:51
@leo-schick leo-schick marked this pull request as ready for review February 26, 2023 10:23
@leo-schick leo-schick added this to the Version 3.4.0 milestone Feb 26, 2023
# host='DOCKER_IP' will be replaced with the ip address given from pytest-docker
# port=-1 will be replaced with the ip address given from pytest-docker

POSTGRES_DB = dbs.PostgreSQLDB(host='DOCKER_IP', port=-1, user="mara", password="mara", database="mara")
Copy link
Member

@jankatins jankatins Mar 7, 2023

Choose a reason for hiding this comment

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

Why not pick that from an env var and add a fixture to set the env var? Or just create this in the fixture, where you have all the information (and return the DB)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yee... I am not 100% sure about that. This is actually a copy from the test suite of the mara_db project. See local_config.py.example.

The main purpose that I defined it in a separate config file is that you have the option to simply activate / disable the tests for specific database engines. Especially for test against cloud services this is handy. I don't want to share the credentials for my cloud with everybody but at the same time share the option for those who want to test their changes against the cloud ;-)

Removing this and integrating it into the fixture makes sence for now, but maybe not in the future...

compression: Compression = Compression.NONE,
format: formats.Format = formats.CsvFormat()) -> None:
"""
Writes the output of a sql query to a file in a specific format. The command is executed on the shell.
Copy link
Member

Choose a reason for hiding this comment

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

... on the shell on the machine which runs mara (e.g. in the docker container!).

Copy link
Member Author

Choose a reason for hiding this comment

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

That is true! I am not happy with that everything in Mara runs through a shell command. For example, when I run mara in a Jupyter Notebook, it is pretty stupid that I have to carry out commands to make sure that the shell toolings are installed. It would be much smarter to use sqlalchemy or a DB API instead. I had already in mind to add a SqlExecutionContext which doesn't use shell but the DB API or SQLAlchemy, but ... didn't really had a usecase where I needed it...

Copy link
Member

@jankatins jankatins left a comment

Choose a reason for hiding this comment

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

See minor doc/test comments...

@leo-schick leo-schick merged commit fe87b3c into main Apr 11, 2023
@leo-schick leo-schick deleted the add-write-file-command branch April 11, 2023 14:16
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.

None yet

2 participants