[Draft] Add support to filter out plans that use Timestamp with Timezone#23179
[Draft] Add support to filter out plans that use Timestamp with Timezone#23179kgpai wants to merge 1 commit intoprestodb:masterfrom
Conversation
| if (auto joinNode = | ||
| std::dynamic_pointer_cast<const core::AbstractJoinNode>(planNode)) { |
There was a problem hiding this comment.
Can AbstractJoinNode be used standalone?
There was a problem hiding this comment.
Yes, it would cover both HashJoin and MergeJoin.
| return true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
AggregationNode::aggregates_::call is a CallTypedExprPtr that can contain a LambdaTypedExprPtr as input. Should we check it too?
|
@gggrace14 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
kgpai
force-pushed
the
filter_out_timestamp_tz
branch
from
8ef9d99 to
1ec636e
Compare
|
@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
kgpai
force-pushed
the
filter_out_timestamp_tz
branch
from
1ec636e to
5c38d5a
Compare
|
@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
kgpai
force-pushed
the
filter_out_timestamp_tz
branch
from
5c38d5a to
064a415
Compare
|
@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@kgpai @gggrace14 @kagamiori : Am curious why the invalidation happens during Prestissimo plan conversion instead of the Presto sql parser/semantic validation of the plan ? This code is brittle and could need a lot of tweaking. Its better to reject such a query at the co-ordinator itself if we are using native engine. It would be cleaner to drop the Timestamp with Timezone type and functions from the list here for native engine There could be other approaches as well. @tdcmeehan : Would be great to hear your thoughts. |
kgpai
force-pushed
the
filter_out_timestamp_tz
branch
from
064a415 to
1a34455
Compare
|
@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@aditi-pandit We plan to fix this in velox as quickly as possible. We are putting this interim solution guarded by a query config and intend to remove it as soon as velox support is landed (which we expected to be done in a few weeks). Don't want to add any change in co-ordinator at this moment since its temporary. Ideally we want to make the velox change, but it will require a few weeks. |
kgpai
force-pushed
the
filter_out_timestamp_tz
branch
from
1a34455 to
9200982
Compare
|
@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Understood that this is a temporary measure. But I don't understand the hesitation around adding this into the coordinator? It would just be a better check--it would occur during analysis, rather than during execution (which is after resources have been allocated, after queueing, tasks have been spun up and splits have begun to be enumerated). I also think it would just be simpler to put it there. |
@tdcmeehan @aditi-pandit Yea coordinator is definitely a better place, I am trying here |
6 participants
Description
This change causes the native worker to throw if Timestamp with Timezone is encountered in the plan. This is gated on the configuration property 'fail_on_timestamp_with_timezone'.
Motivation and Context
Due to issue facebookincubator/velox#10338, there can be cases where comparisons with Timestamp with Timezone can be wrong.
Impact
Will add further details soon.
Test Plan
Working on adding tests.