Skip to content

Conversation

@pashandor789
Copy link
Contributor

@pashandor789 pashandor789 commented Oct 25, 2025

This pull request fixes a logical error in the Expr::sameOrEqual method in QueryGraph.cpp. The change corrects the condition for comparing arguments in function call expressions.

  • Fixed the argument comparison logic in Expr::sameOrEqual so that it returns false if any argument is not the same or equal, rather than if any argument is the same or equal.

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Meta Open Source bot. label Oct 25, 2025
@pashandor789 pashandor789 changed the title fix: sameOrEqual computing bug fix: Make sameOrEqual work properly Oct 25, 2025
@pashandor789
Copy link
Contributor Author

@mbasmanova hey! have found a bug and fixed it

Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

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

@pashandor789 Thank you for the fix. Would you add a test?

@mbasmanova
Copy link
Contributor

CI is red.

@pashandor789 pashandor789 changed the title fix: Make sameOrEqual work properly fix: Make sameOrEqual work properly. Oct 25, 2025
@pashandor789 pashandor789 changed the title fix: Make sameOrEqual work properly. fix: Make sameOrEqual work properly Oct 25, 2025
@pashandor789 pashandor789 changed the title fix: Make sameOrEqual work properly fix: Make sameOrEqual work properly Oct 25, 2025
@MBkkt
Copy link
Collaborator

MBkkt commented Oct 26, 2025

@mbasmanova CI is green now

@pashandor789
Copy link
Contributor Author

pashandor789 commented Oct 27, 2025

@mbasmanova i'm really not sure how to test it. It's used in the depths of the optimizer. Is it possible to merge without test because the fix is really obviuos, WDYT? 🤔

@mbasmanova
Copy link
Contributor

i'm really not sure how to test it. It's used in the depths of the optimizer.

This logic is unit testable, no?

@MBkkt
Copy link
Collaborator

MBkkt commented Nov 3, 2025

@mbasmanova Yes, I even did this, but not sure it's helpful.

I think this code actually couldn't be called while we don't have data sources with partitionkeys/orderkeys that isn't just columns. I think it's possible in theory but in practice we don't have such usage right now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants