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

feat(optimizer): simplify_equality #2281

Merged
merged 2 commits into from
Sep 21, 2023
Merged

feat(optimizer): simplify_equality #2281

merged 2 commits into from
Sep 21, 2023

Conversation

barakalon
Copy link
Collaborator

We have expressions like this:

DATE_ADD(
  DATE_ADD(
    DATE_TRUNC(
      'week', 
      DATE_SUB(
        x, 
        1, 
        DAY
      )
    ), 
    1, 
    DAY
  ), 
  1, 
  YEAR
) < CAST('2021-01-08' AS DATE);

This optimization, plus the previous operations, gives:

x < CAST('2020-01-07' AS DATE);

1 + x + 1 = 3 + 1;
x = 2;

x - INTERVAL 1 DAY = CAST('2021-01-01' AS DATE);
Copy link
Owner

Choose a reason for hiding this comment

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

what would happen if it's

x - interval 1 day = cast(y as date)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nothing. The right side must be a date literal.

return expression

return expression.__class__(
this=a.copy(), expression=INVERSE_OPS[l.__class__](this=r.copy(), expression=b.copy())
Copy link
Owner

Choose a reason for hiding this comment

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

are these copies needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah shit sorry i hit merge.
I'm not sure.
In general, I copy when creating a new expression using child nodes from an existing tree. When do you?

Copy link
Owner

@tobymao tobymao Sep 21, 2023

Choose a reason for hiding this comment

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

in the optimizer, i only copy when i have to, because it's mutation heavy and expensive,

in generator, where the contract is idempotency and no mutations, i copy

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@georgesittas georgesittas Sep 21, 2023

Choose a reason for hiding this comment

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

Agree on copy being a bit redundant here: It seems like we're dropping the expression arg in this return statement anyway & a and b are not associated with r's AST, so this completely replaces the AST corresponding to the input expression.

@barakalon barakalon merged commit 69a2c67 into main Sep 21, 2023
@barakalon barakalon deleted the barak/simplify-equality branch September 21, 2023 16:32
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.

3 participants