⭐ Improve parameterized query support - fixes #793

[mvanderlee]

Fixes #793
The implemented tests speak for themselves, but here is an extract:

subquery = (
    Query.from_(self.table_efg)
    .select(self.table_efg.fiz, self.table_efg.buz)
    .where(self.table_efg.buz == 'buz')
)

q = (
    Query.from_(self.table_abc)
    .join(subquery)
    .on(self.table_abc.bar == subquery.buz)
    .select(self.table_abc.foo, subquery.fiz)
    .where(self.table_abc.bar == 'bar')
)

parameter = NamedParameter()
sql = q.get_sql(parameter=parameter)
self.assertEqual(
    'SELECT "abc"."foo","sq0"."fiz" FROM "abc" JOIN (SELECT "fiz","buz" FROM "efg" WHERE "buz"=:param1)'
    ' "sq0" ON "abc"."bar"="sq0"."buz" WHERE "abc"."bar"=:param2',
    sql,
)
self.assertEqual({'param1': 'buz', 'param2': 'bar'}, parameter.get_parameters())

This would allow the user to easily plug into SQLAlchemy for example:

parameter = NamedParameter()
session.execute(text(q.get_sql(parameter=parameter)), **parameter.get_parameters())

[wd60622]

how does this work on None?

[mvanderlee]

callable Returns False

But I presume you mean how will the self_placeholder(None) work with None.
Good point, it wouldn't atm with the default generators.

I will fix it now.

[wd60622]

Thanks for the PR! Will take a deeper look later

[coveralls]

coverage: 98.392% (+0.002%) from 98.39%
when pulling 2ae30df on mvanderlee:mvanderlee/parametized_query
into 8841520 on kayak:master.

[mvanderlee]

Thanks for the PR! Will take a deeper look later

@wd60622 Do you think you'll have time this week?

[wd60622]

Looks really good.
Just some comments on type hints, raising errors, and combining logic

[wd60622]

Type hint here doesn't include callable? Is that correct still?

[mvanderlee]

Correct, only ListParameter and DictParameter (and classes that inherit from them) support callable.
The base parameter does not.

[wd60622]

Might be good to use a setter here. With that, implementing a check for the correct type can happen on initialization if put in the setter

[mvanderlee]

I'm not sure I follow how to do this pythonically? Could you share some pseudo code to show what you mean?

[danielenricocahall]

@mvanderlee I believe @wd60622 was referring to the following:

@property
def placeholder(self):
      return self._placeholder

@placeholder.setter
def placeholder(self, placeholder):
     # can add checks here if needed
     self._placeholder = placeholder

[mvanderlee]

Right, which adds no value and isn't pythonic. This isn't Java/C#.

It would also break parameters. After a parameter has been created and used, any changes to the placeholder will break queries. Which is why I've made it private with a getter only.

I haven't changed the type, so any type checks would have to be part of a separate PR.

[danielenricocahall]

I would disagree with you on it not being Pythonic, as it’s a native language feature.

Also, I was trying to be helpful, but given your receptiveness to discussion and tone, I won’t proceed.

Good luck.

[mvanderlee]

Sorry for my tone @danielenricocahall it'd been a rough week and I lashed out.

As for pythonic. This video does a good job at explaining language feature vs pythonic.
Raymond Hettinger - Beyond PEP 8 -- Best practices for beautiful intelligible code - https://www.youtube.com/watch?v=wf-BqAjZb8M

[wd60622]

Do you have any thoughts as well? @AzisK

[mvanderlee]

@wd60622 @AzisK any update on this?
It should go hand in hand with #792

[AzisK]

Could we also have this functionality described in pypika docs?

[AzisK]

The tests look really nice. What about adding PyformatParameter in the tests as well?

[reasv]

I just wanted to say that I'm going to fork your branch because this feature is really useful, and I'm using it in my own project to help me build a custom high level query language: https://github.com/reasv/panoptikon/blob/master/src/panoptikon/db/pql/build_query.py

I'm resorting to making my own fork of PyPika and merging a bunch of these PRs because it doesn't seem PyPika is getting much merged lately and there are open bugs that otherwise block development: d7a4054

[larsschwegmann]

Any progress on getting this merged? :)

[AzisK]

Hello, I asked @mvanderlee for a test for PyformatParameter and I believe it would be good to have it. Otherwise I could merge this PR

[larsschwegmann]

I forked @mvanderlee 's fork and added a test for PyformatParameter: Commit

[AzisK]

@larsschwegmann could you make a PR? Is it better for me to push this change to this commit? Probably

[larsschwegmann]

If it's possible, I guess it would be best if you could just add the commit to this PR, just for the sake of documentation. Then you'll still have all the discussion in the PR that was merged 😄
If not, I can create a PR. Let me know what works best for you.

[mvanderlee]

Thank you @larsschwegmann I've added it to the branch!

[AzisK]

I could merge but linting fails. It's not a big deal but it would be good to lint before. Also unit tests for other Python versions fail but it's probably a bug of Github actions which is super annoying. It seems that they fail due to a race condition

[mvanderlee]

@AzisK I'll run the linter, but it's an issue in the master branch. The linter is failing on lines that were not altered by this PR. I see the same errors with other PRs.

[mvanderlee]

The affected code wasn't in this fork yet, so I rebased in order to fix it.