Skip to content

Add more tests for modulo pushdown in PostgreSQL#12104

Merged
hashhar merged 1 commit intotrinodb:masterfrom
hashhar:hashhar/postgres-modulo-rewrite-test
Oct 14, 2022
Merged

Add more tests for modulo pushdown in PostgreSQL#12104
hashhar merged 1 commit intotrinodb:masterfrom
hashhar:hashhar/postgres-modulo-rewrite-test

Conversation

@hashhar
Copy link
Copy Markdown
Member

@hashhar hashhar commented Apr 22, 2022

Some databases return remainder instead of modulus when one of the
values is negative. Since Postgres tests are used as template for other
tests let's make sure we have a test covering this case even if Postgres
itself has the correct behaviour. This is to ensure that any databases
which calculate remainder instead can be caught in tests.

@hashhar hashhar added test no-release-notes This pull request does not require release notes entry labels Apr 22, 2022
@cla-bot cla-bot bot added the cla-signed label Apr 22, 2022
@hashhar hashhar requested review from ebyhr and findepi April 22, 2022 13:58
Comment on lines 724 to 722
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That could be BCT.

Also, what if we have % (regionkey-1) (a variable on the RHS) ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point, I'll add a test.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added, but kept in TestPostgreSqlConnectorTest for now since most connectors don't support connector expression pushdown yet.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can have correctness-only coverage in BCT.
or guard this with SUPPORTS_PREDICATE_EXPRESSION_PUSHDOWN

@hashhar hashhar force-pushed the hashhar/postgres-modulo-rewrite-test branch from cc5d7a5 to e1fd849 Compare April 25, 2022 05:56
@hashhar
Copy link
Copy Markdown
Member Author

hashhar commented Apr 25, 2022

AC @findepi PTAL.

@findepi
Copy link
Copy Markdown
Member

findepi commented Oct 12, 2022

can you please rebase?

@hashhar
Copy link
Copy Markdown
Member Author

hashhar commented Oct 12, 2022

I'd prefer if @vlad-lyutenko takes over this in #14570 since that PR will be more useful to have same test run in two connectors and hence come with more general test.

@hashhar
Copy link
Copy Markdown
Member Author

hashhar commented Oct 12, 2022

Rebased either way. But did not move test to BCT. Movement to BCT/BJCT should be done anyway in #14570.

@hashhar hashhar force-pushed the hashhar/postgres-modulo-rewrite-test branch from e1fd849 to 228e1db Compare October 12, 2022 12:59
Some databases return remainder instead of modulus when one of the
values is negative. Since Postgres tests are used as template for other
tests let's make sure we have a test covering this case even if Postgres
itself has the correct behaviour. This is to ensure that any databases
which calculate remainder instead can be caught in tests.
@hashhar hashhar force-pushed the hashhar/postgres-modulo-rewrite-test branch from a43dc57 to b14e309 Compare October 14, 2022 08:39
@hashhar hashhar merged commit 9b59007 into trinodb:master Oct 14, 2022
@hashhar hashhar deleted the hashhar/postgres-modulo-rewrite-test branch October 14, 2022 13:08
@github-actions github-actions bot added this to the 401 milestone Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed no-release-notes This pull request does not require release notes entry test

Development

Successfully merging this pull request may close these issues.

2 participants