Skip to content

Conversation

@bovlb
Copy link
Contributor

@bovlb bovlb commented Aug 3, 2025

Currently, queries like:

SELECT * FROM MyTable  WHERE flag; -- Var
SELECT * FROM MyTable  WHERE flag = TRUE; --- normalized into previous
SELECT * FROM MyTable  WHERE flag IS TRUE; -- BooleanTest

Are not passed down in quals. This change fixes that by adding support for BooleanTest and Var.

Tested in PostgreSQL 17

Copy link
Collaborator

@mfenniak mfenniak left a comment

Choose a reason for hiding this comment

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

Thanks for the proposed contribution!

I'm wondering if there's one case that could be considered here that isn't:

SELECT * FROM MyTable  WHERE NOT flag; -- inverted var

It would be great if you could add to one of the test suites all of the supported cases -- multicorn_regression_test.sql would be ideal as there's a small Test quals section there that could be expanded with the new boolean operations to ensure that they work in all environments and are maintained in the future.

A very small nit but it looks like there's a mix of tab & space indentation in the newly added code, if you don't mind standardizing it to match the source file that would be amazing.

@bovlb
Copy link
Contributor Author

bovlb commented Aug 4, 2025

I'm wondering if there's one case that could be considered here that isn't:

SELECT * FROM MyTable  WHERE NOT flag; -- inverted var

I could be wrong, but I don't think this is either a BooleanTest or a Var, but would be a BoolExpr, so that would be a distinct thing to take on.

@bovlb
Copy link
Contributor Author

bovlb commented Aug 4, 2025

A very small nit but it looks like there's a mix of tab & space indentation in the newly added code, if you don't mind standardizing it to match the source file that would be amazing.

I think I have fixed it.

@bovlb
Copy link
Contributor Author

bovlb commented Aug 4, 2025

It would be great if you could add to one of the test suites all of the supported cases -- multicorn_regression_test.sql would be ideal as there's a small Test quals section there that could be expanded with the new boolean operations to ensure that they work in all environments and are maintained in the future.

I would love to add tests, but I have not yet succeeded in running the existing test suite.

@mfenniak
Copy link
Collaborator

mfenniak commented Aug 4, 2025

I would love to add tests, but I have not yet succeeded in running the existing test suite.

I run a little unusual dev environment (running NixOS) so I can't advise a whole lot on this -- in NixOS, or if Nix is installed, it's documented in the README (https://github.com/pgsql-io/multicorn2?tab=readme-ov-file#integration-tests). But outside of that environment... theoretically it should just require make easycheck?

If you have troubles, we could merge this in and I can add a test... but I took a quick shot at it and ran into a concern you might want to review first. I added these test statements near the end of test-3.9/sql/multicorn_regression_test.sql...

...
SELECT * from testmulticorn where test3 = 12::money;
SELECT * from testmulticorn where test1 = '12 €';

-- Test boolean operation pushdown
ALTER FOREIGN TABLE testmulticorn alter test1 type bool;
select * from testmulticorn where test1;
select * from testmulticorn where test1 = TRUE;
select * from testmulticorn where test1 = FALSE;
select * from testmulticorn where test1 IS TRUE;
select * from testmulticorn where test1 IS FALSE;
select * from testmulticorn where test1 IS UNKNOWN;
select * from testmulticorn where test1 IS NOT UNKNOWN;

DROP USER MAPPING FOR current_user SERVER multicorn_srv;
DROP EXTENSION multicorn cascade;
...

Resulted in a warning and no quals being pushed down, in the test1 = FALSE case:

select * from testmulticorn where test1 = FALSE;
WARNING:  unsupported expression for extractClauseFrom
DETAIL:  {BOOLEXPR :boolop not :args ({VAR :varno 1 :varattno 1 :vartype 16 :vartypmod -1 :varcollid 0 :varlevelsup 0 :varnosyn 1 :varattnosyn 1 :location 76}) :location -1}
WARNING:  unsupported expression for extractClauseFrom
DETAIL:  {BOOLEXPR :boolop not :args ({VAR :varno 1 :varattno 1 :vartype 16 :vartypmod -1 :varcollid 0 :varlevelsup 0 :varnosyn 1 :varattnosyn 1 :location 76}) :location -1}

@bovlb
Copy link
Contributor Author

bovlb commented Aug 4, 2025

I'm wondering if there's one case that could be considered here that isn't:

SELECT * FROM MyTable  WHERE NOT flag; -- inverted var

I could be wrong, but I don't think this is either a BooleanTest or a Var, but would be a BoolExpr, so that would be a distinct thing to take on.

Done.

@bovlb bovlb requested a review from mfenniak August 4, 2025 03:58
@bovlb
Copy link
Contributor Author

bovlb commented Aug 4, 2025

Resulted in a warning and no quals being pushed down, in the test1 = FALSE case:

Using EXPLAIN, it appears the test1 = FALSE is normalized to NOT test1, which is covered by my recent update.

@bovlb
Copy link
Contributor Author

bovlb commented Aug 4, 2025

run a little unusual dev environment (running NixOS) so I can't advise a whole lot on this -- in NixOS, or if Nix is installed, it's documented in the README (https://github.com/pgsql-io/multicorn2?tab=readme-ov-file#integration-tests). But outside of that environment... theoretically it should just require make easycheck?

I wasn't ready to cut a PR for this, but I have playing with adding a dev.container.
https://github.com/pgsql-io/multicorn2/compare/main...bovlb:multicorn2:dev-container?expand=1
In this environment, I can neither run the nix commands suggested in the readme, nor make easycheck (which I already found).

@mfenniak mfenniak merged commit aca6514 into pgsql-io:main Aug 4, 2025
1 check passed
@mfenniak
Copy link
Collaborator

mfenniak commented Aug 4, 2025

@bovlb Sorry for the inconvenience on testing -- I'll try to write up some useful dev testing instructions in the future. In the meantime I popped in my tests after your NOT column addition and merged. Thanks for the contribution! 👍

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