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

[native] Disable plan validation failure for timestamp with timezone and ipaddress types for native execution. #24040

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

amitkdutta
Copy link
Contributor

@amitkdutta amitkdutta commented Nov 14, 2024

Description

As native engine now supports timestamp with timezone and ipaddress types, no need to make queries fail with plan validation by default. This change disables such validation failures by default.

Motivation and Context

Keeping timestamp and ipaddress by default enabled like java engine.

Impact

No new impact. If a production system explicitly set these configs, now they can remove those as default is enabled.

Test Plan

Updated existing tests.

== NO RELEASE NOTE ==

@prestodb-ci prestodb-ci added the from:Meta PR from Meta label Nov 14, 2024
@amitkdutta amitkdutta marked this pull request as draft November 14, 2024 01:28
@amitkdutta amitkdutta changed the title [native] Disable plan validation failure for timestamp with timezone … [native] Disable plan validation failure for timestamp with timezone and ipaddress types for native execution. Nov 14, 2024
@amitkdutta amitkdutta marked this pull request as ready for review November 14, 2024 04:49
@tdcmeehan
Copy link
Contributor

Should we also delete the code CheckUnsupportedPrestissimoTypes?

feilong-liu
feilong-liu previously approved these changes Nov 14, 2024
@aditi-pandit
Copy link
Contributor

Should we also delete the code CheckUnsupportedPrestissimoTypes?

Agree. We should remove the code as well. It doesn't provide any value anymore. We can re-add it if we want to bring the config back.

Copy link
Contributor

@pramodsatya pramodsatya left a comment

Choose a reason for hiding this comment

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

Thanks @amitkdutta! The failing e2e tests with IpAddress and TimestampWithTimeZone types in presto-native-tests are running successfully with this fix.

@amitkdutta
Copy link
Contributor Author

@tdcmeehan @aditi-pandit Yes, will delete CheckUnsupportedPrestissimoTypes. We just launched timestamp and ipaddress support a week ago, and as the features are new, it could be possible to hit issues in next few weeks. If anything observed, we will have the opportunity to disable timestamp but keep the build with other fixes as each release contains many new correctness fixes with velox. Henece, I want to keep the check alive but disable by default. Once we pass through a few releases, we will be more confident to delete it completely.

@amitkdutta amitkdutta force-pushed the enable_1 branch 5 times, most recently from bc36e26 to 54a4fac Compare November 23, 2024 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from:Meta PR from Meta
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants