Add plan validator for timestamp with timezone check for prestissimo#23200
Conversation
0ae802d to
beb771f
Compare
|
@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. |
beb771f to
3118100
Compare
3118100 to
c7189a7
Compare
Added one more feature config for it |
b418e6e to
9855049
Compare
9855049 to
2eb533b
Compare
amitkdutta
left a comment
There was a problem hiding this comment.
Looks good from my end.
@aditi-pandit @tdcmeehan See if you have any comments.
| { | ||
| 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.*"); |
There was a problem hiding this comment.
Nice! Thanks for validating the behavior in e2e tests.
| return this.nativeExecutionEnabled; | ||
| } | ||
|
|
||
| @Config("disable-timestamp-with-timezone-for-native-execution") |
There was a problem hiding this comment.
@gggrace14 @kgpai @kagamiori For testing the timestamp comparison features, we will need to turn this on in deployment configs.
|
+1. LGTM |
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:
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.