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

Fix queries filtering for the same condition with both an IN and EQUALS to not return empty results #16597

Merged
merged 10 commits into from
Jul 9, 2024

Conversation

kgyrtkirk
Copy link
Member

@kgyrtkirk kgyrtkirk commented Jun 13, 2024

  • temp fix until CALCITE-6435 gets fixed (released&upgraded to)
  • added a custom rule (FixIncorrectInExpansionTypes) to fix-up types of the affected literals
  • added a testcase which will alert on upgrade

Release note

Queries filtering for the same string value with both an IN expression and an = filter may have been incorrectly returned an empty resultset.

This PR has:

  • been self-reviewed.
  • a release note entry in the PR description.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.

{
public FixIncorrectInExpansionTypes()
{
super(operand(RelNode.class, any()));

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note

Invoking
RelOptRule.operand
should be avoided because it has been deprecated.
{
public FixIncorrectInExpansionTypes()
{
super(operand(RelNode.class, any()));

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note

Invoking
RelOptRule.any
should be avoided because it has been deprecated.
@kgyrtkirk kgyrtkirk closed this Jun 25, 2024
@kgyrtkirk kgyrtkirk reopened this Jun 25, 2024
Copy link
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

I suspect the PR description is not accurate; it says:

current approach is to run the ReduceExpressionRule early to get rid of the problematic issue

but that does not seem to be what is happening. Could you please clarify what the bug is and how we're fixing it?

public RexNode visitCall(RexCall call)
{
RexNode newNode = super.visitCall(call);
if (newNode.getKind() == SqlKind.EQUALS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this really only needed for EQUALS? what about NOT_EQUALS, GREATER_THAN, etc? what about other kinds of operations on strings?

Copy link
Member Author

Choose a reason for hiding this comment

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

to raise the current error:

  • a literal generated by that part in SqlToRel for an IN expression or NOT IN
  • a comparision with equals should happen in RexSimplify

I've tried a few queries with not in ; and they were not able to trigger the issue.
but I've added NOT_EQUALS as there might be some twisted case just in case...

@kgyrtkirk
Copy link
Member Author

current approach is to run the ReduceExpressionRule early to get rid of the problematic issue

but that does not seem to be what is happening. Could you please clarify what the bug is and how we're fixing it?

thank you for noticing this! that would have been the easiest to fix it - however it had some adverse effects; so I've decided to write a custom rule instead. (updated the desc)

Copy link
Contributor

@sreemanamala sreemanamala left a comment

Choose a reason for hiding this comment

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

Looks good. Quick qsn - Can we have the sqlToRelWorkaroundProgram to have multiple temporary rules in case if we need similar rules rather than a singleton list or we need to have a new Program?

@kgyrtkirk
Copy link
Member Author

thank you @sreemanamala for taking a look; I expect this sqlToRelWorkaroundProgram to be removed when we upgrade to 1.38.0 - I don't think we will need to add more such rules...or at least hope :D
but it can be re-organized as needed if we have to add anything beyond this

Copy link
Contributor

@abhishekagarwal87 abhishekagarwal87 left a comment

Choose a reason for hiding this comment

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

Tried a few queries. All passed. PR lgtm.

@abhishekagarwal87 abhishekagarwal87 merged commit a9bd0ee into apache:master Jul 9, 2024
88 checks passed
sreemanamala pushed a commit to sreemanamala/druid that referenced this pull request Aug 6, 2024
…LS to not return empty results (apache#16597)

temp fix until CALCITE-6435 gets fixed (released&upgraded to)
added a custom rule (FixIncorrectInExpansionTypes) to fix-up types of the affected literals
added a testcase which will alert on upgrade
@kfaraz kfaraz added this to the 31.0.0 milestone Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants