Skip to content

Translate arithmetic to connector expression#11511

Merged
findepi merged 2 commits intotrinodb:masterfrom
findepi:findepi/translate-arithmetic-to-connector-expression-f74ff0
Mar 21, 2022
Merged

Translate arithmetic to connector expression#11511
findepi merged 2 commits intotrinodb:masterfrom
findepi:findepi/translate-arithmetic-to-connector-expression-f74ff0

Conversation

@findepi
Copy link
Copy Markdown
Member

@findepi findepi commented Mar 16, 2022

Extracted from #11510

Copy link
Copy Markdown
Contributor

@wendigo wendigo left a comment

Choose a reason for hiding this comment

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

LGTM % comment

Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM % possible improvement to test.

@findepi findepi force-pushed the findepi/translate-arithmetic-to-connector-expression-f74ff0 branch from 39547d2 to 7652a39 Compare March 17, 2022 13:51
@findepi findepi force-pushed the findepi/translate-arithmetic-to-connector-expression-f74ff0 branch 2 times, most recently from ab3bbfe to 25fcbf4 Compare March 18, 2022 09:45
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Mar 18, 2022

When working on this PR discovered #11559 and #11560.
Rebased on top of #11561

@findepi findepi force-pushed the findepi/translate-arithmetic-to-connector-expression-f74ff0 branch from 25fcbf4 to ee1edf9 Compare March 18, 2022 13:12
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Mar 18, 2022

rebased on top of #11567

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.

how much time does it take?

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.

3 minutes, this is the default optimizer timeout. The test is fixed, however, within the PR. (#11567)

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.

btw i didn't lower optimizer timeout in tests, because this needs to account for fetching metadata from remote systems, so could lead to test flakiness .

@findepi findepi force-pushed the findepi/translate-arithmetic-to-connector-expression-f74ff0 branch from ee1edf9 to 0f1a658 Compare March 18, 2022 16:41
@findepi findepi force-pushed the findepi/translate-arithmetic-to-connector-expression-f74ff0 branch from 0f1a658 to 8d04de9 Compare March 21, 2022 09:43
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Mar 21, 2022

Rebased after #11567 merged.

List.of(symbolReference1, dereferenceExpression1));

assertPartialTranslation(
new CoalesceExpression(
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.

i replaced + with coalesce as an example of an expression that cannot be translated.
This is OK for now, but we need a better testing strategy, see eg #11535
cc @ebyhr

@findepi findepi merged commit 08e4146 into trinodb:master Mar 21, 2022
@findepi findepi deleted the findepi/translate-arithmetic-to-connector-expression-f74ff0 branch March 21, 2022 12:50
@findepi findepi added enhancement New feature or request no-release-notes This pull request does not require release notes entry labels Mar 21, 2022
@github-actions github-actions bot added this to the 375 milestone Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed enhancement New feature or request no-release-notes This pull request does not require release notes entry

Development

Successfully merging this pull request may close these issues.

4 participants