Skip to content
This repository has been archived by the owner on Mar 16, 2022. It is now read-only.

Query from sql or file #6

Merged
merged 4 commits into from Oct 10, 2017
Merged

Query from sql or file #6

merged 4 commits into from Oct 10, 2017

Conversation

mathieuripert
Copy link
Contributor

see #5

@mathieuripert mathieuripert changed the title Query from Query from sql or file Oct 6, 2017
@neutralino1 neutralino1 merged commit d0118d0 into master Oct 10, 2017
@mathieuripert
Copy link
Contributor Author

it was possible to do things like

df1 = CustomersModel.query_from(sql='select id, name from zones where region_id={region_id}', region_id=1)

but now it fails with your changes
cc @neutralino1

jardin/model.py Outdated

@classmethod
def query_from(self, sql=None, filename=None, **kwargs):
kwargs['stack'] = self.stack_mark(inspect.stack())
if filename:
filename = os.path.join(os.path.dirname(inspect.stack()[1][1]), filename)
return self.db().dataframe(sql=sql, filename=filename, **kwargs)
filename = os.path.join(os.environ['PWD'], filename)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this fails for me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why do you think we need os.environ['PWD'] instead of the dir path of the file that calls the method

Copy link
Contributor

Choose a reason for hiding this comment

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

Modules get imported from the directory python is running in, even if the code importing the module is deeper in the tree. The pattern I'm seeing in airflow is also to define paths (config files, resources...) with respect to the directory where python runs. I think it makes sense to follow that pattern.
This way modules can move around independently without the whole downstream tree having to follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok that works!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, but it could lead to issues, if we have some shared code from different services?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i thought that would have been safer to define the path to sql file relative from where it is called, but i'll see when i use it in batching

@mathieuripert mathieuripert deleted the query-from branch October 10, 2017 04:32
@neutralino1
Copy link
Contributor

@mathieuripert Yes, the correct syntax according to the rest of the API is

df1 = CustomersModel.query_from(sql='select id, name from zones where region_id={region_id}', where={'region_id': 1})

@mathieuripert
Copy link
Contributor Author

:(

@mathieuripert
Copy link
Contributor Author

i dont think it is intuitive to have the where argument as it could be any type of arguments

@mathieuripert
Copy link
Contributor Author

at least params instead of where. but i liked the ability to pass the named arguments directly

@neutralino1
Copy link
Contributor

It is the same argument as when doing a select, update, delete...
We can change that but I think that's beyond the scope of this PR.
The syntax you propose would add a lot of conflicts to the rest of the API, namely what happens if the table has columns with names equal to SQL keywords (join, insert, limit...)?
Only the insert method follows the pattern you propose:

Zones.insert(name='Paris', region_id=100)

because there are no WHERE clause in the INSERT command.

@neutralino1
Copy link
Contributor

Yes we could name it params, and store it as kwargs['where'] inside the method so that the downstream API keeps working. That works for me.

@mathieuripert
Copy link
Contributor Author

vendu pour params 👍

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

Successfully merging this pull request may close these issues.

None yet

2 participants