Add plan validator for timestamp with timezone check for prestissimo #23200

feilong-liu · 2024-07-12T21:01:22Z

Description

Detail in discussion of #23191

Add a plan checker for Prestissimo, so that queries which have timestamp with timezone type will fail.

Motivation and Context

Fix facebookincubator/velox#10338

Impact

Only affect Prestissimo

Test Plan

existing unit tests and
end to end test with queries in the issue:

presto:sf1> select cast(x as timestamp with time zone), count(1) from unnest(array['2024-04-10 07:00 America/Los_Angeles', '2024-04-10 10:00 America/New_York']) as t(x) group by 1;
Query 20240712_205745_00033_5bggp failed: Timestamp with Timezone type is not supported in Prestissimo
java.lang.IllegalStateException: Timestamp with Timezone type is not supported in Prestissimo
	at com.google.common.base.Preconditions.checkState(Preconditions.java:507)
	at com.facebook.presto.sql.planner.sanity.CheckNoTimestampWithNoTimezoneType$Visitor.visitPlan(CheckNoTimestampWithNoTimezoneType.java:63)
	at com.facebook.presto.sql.planner.sanity.CheckNoTimestampWithNoTimezoneType$Visitor.visitPlan(CheckNoTimestampWithNoTimezoneType.java:50)
	at com.facebook.presto.spi.plan.PlanVisitor.visitOutput(PlanVisitor.java:25)
	at com.facebook.presto.spi.plan.OutputNode.accept(OutputNode.java:98)
	at com.facebook.presto.sql.planner.sanity.CheckNoTimestampWithNoTimezoneType.validate(CheckNoTimestampWithNoTimezoneType.java:47)
	at com.facebook.presto.sql.planner.sanity.PlanChecker.lambda$validateFinalPlan$0(PlanChecker.java:81)
	at com.google.common.collect.ImmutableList.forEach(ImmutableList.java:407)
	at com.facebook.presto.sql.planner.sanity.PlanChecker.validateFinalPlan(PlanChecker.java:81)
	at com.facebook.presto.sql.Optimizer.validateAndOptimizePlan(Optimizer.java:131)
	at com.facebook.presto.execution.SqlQueryExecution.lambda$createLogicalPlanAndOptimize$3(SqlQueryExecution.java:557)
	at com.facebook.presto.common.RuntimeStats.profileNanos(RuntimeStats.java:136)
	at com.facebook.presto.execution.SqlQueryExecution.createLogicalPlanAndOptimize(SqlQueryExecution.java:555)
	at com.facebook.presto.execution.SqlQueryExecution.start(SqlQueryExecution.java:463)
	at com.facebook.presto.$gen.Presto_null__testversion____20240712_205726_4.run(Unknown Source)
	at com.facebook.presto.execution.SqlQueryManager.createQuery(SqlQueryManager.java:319)
	at com.facebook.presto.dispatcher.LocalDispatchQuery.lambda$startExecution$8(LocalDispatchQuery.java:213)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:829)

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
* Add a plan checker so that queries having timestamp with timezone type will fail in prestissimo :pr:`23200`

aditi-pandit · 2024-07-12T21:19:56Z

@feilong-liu : Thanks for this code. MIght be great to put this behind a co-ordinator config so that we can enable/disable in our clusters easily as the underlying issue is getting fixed in Velox.

feilong-liu · 2024-07-12T22:42:39Z

@feilong-liu : Thanks for this code. MIght be great to put this behind a co-ordinator config so that we can enable/disable in our clusters easily as the underlying issue is getting fixed in Velox.

Added one more feature config for it

amitkdutta

Looks good from my end.
@aditi-pandit @tdcmeehan See if you have any comments.

amitkdutta · 2024-07-13T01:42:12Z

...ecution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeGeneralQueries.java

@@ -1211,22 +1211,22 @@ public void testArithmetic()
    @Test
    public void testTimestampWithTimeZone()
    {
-        assertQuery("SELECT from_unixtime(orderkey, '+01:00'), from_unixtime(orderkey, '-05:00'), from_unixtime(orderkey, 'Europe/Moscow') FROM orders");
-        assertQuery("SELECT from_unixtime(orderkey, '+01:00'), count(1) FROM orders GROUP BY 1");
+        assertQueryFails("SELECT from_unixtime(orderkey, '+01:00'), from_unixtime(orderkey, '-05:00'), from_unixtime(orderkey, 'Europe/Moscow') FROM orders", ".*Timestamp with Timezone type is not supported in Prestissimo.*");


Nice! Thanks for validating the behavior in e2e tests.

amitkdutta · 2024-07-13T01:43:26Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java

@@ -2601,6 +2602,19 @@ public boolean isNativeExecutionEnabled()
        return this.nativeExecutionEnabled;
    }

+    @Config("disable-timestamp-with-timezone-for-native-execution")


@gggrace14 @kgpai @kagamiori For testing the timestamp comparison features, we will need to turn this on in deployment configs.

aditi-pandit · 2024-07-13T17:21:54Z

+1. LGTM

feilong-liu requested review from jaystarshot, ClarenceThreepwood and a team as code owners July 12, 2024 21:01

feilong-liu requested a review from presto-oss July 12, 2024 21:01

feilong-liu force-pushed the timestamp_timezone_validate branch from 0ae802d to beb771f Compare July 12, 2024 21:07

feilong-liu requested a review from amitkdutta July 12, 2024 21:07

feilong-liu force-pushed the timestamp_timezone_validate branch from beb771f to 3118100 Compare July 12, 2024 22:33

feilong-liu requested a review from a team as a code owner July 12, 2024 22:33

feilong-liu force-pushed the timestamp_timezone_validate branch from 3118100 to c7189a7 Compare July 12, 2024 22:42

feilong-liu force-pushed the timestamp_timezone_validate branch 2 times, most recently from b418e6e to 9855049 Compare July 13, 2024 00:11

Add plan validator for timestamp with timezone check for prestissimo

2eb533b

feilong-liu force-pushed the timestamp_timezone_validate branch from 9855049 to 2eb533b Compare July 13, 2024 00:44

amitkdutta approved these changes Jul 13, 2024

View reviewed changes

amitkdutta reviewed Jul 13, 2024

View reviewed changes

feilong-liu requested a review from tdcmeehan July 13, 2024 01:51

NikhilCollooru approved these changes Jul 13, 2024

View reviewed changes

feilong-liu merged commit b693975 into prestodb:master Jul 13, 2024
59 checks passed

feilong-liu deleted the timestamp_timezone_validate branch July 13, 2024 05:51

amitkdutta mentioned this pull request Jul 13, 2024

Disable timestamp #23191

Closed

feilong-liu mentioned this pull request Aug 3, 2024

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

Merged

6 tasks

amitkdutta mentioned this pull request Aug 9, 2024

Add IPAddress Presto type facebookincubator/velox#10596

Closed

mohsaka mentioned this pull request Aug 13, 2024

[native] Disable IPAddress type on presto native #23436

Merged

6 tasks

tdcmeehan mentioned this pull request Aug 23, 2024

Add release notes for 0.289 #23513

Merged

34 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add plan validator for timestamp with timezone check for prestissimo #23200

Add plan validator for timestamp with timezone check for prestissimo #23200

feilong-liu commented Jul 12, 2024 •

edited

Loading

aditi-pandit commented Jul 12, 2024

feilong-liu commented Jul 12, 2024

amitkdutta left a comment

amitkdutta Jul 13, 2024

amitkdutta Jul 13, 2024

aditi-pandit commented Jul 13, 2024

Add plan validator for timestamp with timezone check for prestissimo #23200

Add plan validator for timestamp with timezone check for prestissimo #23200

Conversation

feilong-liu commented Jul 12, 2024 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

aditi-pandit commented Jul 12, 2024

feilong-liu commented Jul 12, 2024

amitkdutta left a comment

Choose a reason for hiding this comment

amitkdutta Jul 13, 2024

Choose a reason for hiding this comment

amitkdutta Jul 13, 2024

Choose a reason for hiding this comment

aditi-pandit commented Jul 13, 2024

feilong-liu commented Jul 12, 2024 •

edited

Loading