Skip to content

Move timezone with timestamp plan checker to intermediate plan stage#23374

Merged
feilong-liu merged 1 commit intoprestodb:masterfrom
feilong-liu:plan_tstz
Aug 5, 2024
Merged

Move timezone with timestamp plan checker to intermediate plan stage#23374
feilong-liu merged 1 commit intoprestodb:masterfrom
feilong-liu:plan_tstz

Conversation

@feilong-liu
Copy link
Contributor

@feilong-liu feilong-liu commented Aug 3, 2024

Description

This plan check is added in #23200
However, it does not check the row expressions in table scan node (for example filters pushed down into table scan). Since we will need to check the type of table scan and add logic for all connectors if we want to solve it in the plan checker, I instead choose to move the plan check to the intermediate plan stage, which happens before filter pushdown.

Motivation and Context

Cover queries which not covered

Impact

Cover queries which not covered

Test Plan

Existing unit tests and newly added test

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Improve the plan check for timestamp with timezone type to cover filter pushdown :pr:`12345`

@feilong-liu feilong-liu requested a review from presto-oss August 3, 2024 00:02
@feilong-liu feilong-liu marked this pull request as draft August 3, 2024 00:02
@feilong-liu feilong-liu marked this pull request as ready for review August 3, 2024 06:53
@feilong-liu feilong-liu requested a review from a team as a code owner August 3, 2024 06:53
Copy link
Contributor

@amitkdutta amitkdutta left a comment

Choose a reason for hiding this comment

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

Thanks @feilong-liu

Copy link
Member

@zacw7 zacw7 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@feilong-liu feilong-liu merged commit ce85e97 into prestodb:master Aug 5, 2024
@feilong-liu feilong-liu deleted the plan_tstz branch August 5, 2024 17:39
@tdcmeehan tdcmeehan mentioned this pull request Aug 23, 2024
34 tasks
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.

5 participants