-
Notifications
You must be signed in to change notification settings - Fork 625
[GLUTEN-11862][VL] Work around GMT session timezone validation failure on macOS #11869
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
Changes from 3 commits
2b78e35
2275b3f
d9af892
7b07767
cbb8085
fea81aa
1f03304
7fe431c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,14 +25,13 @@ | |
| #include "velox/exec/PlanNodeStats.h" | ||
| #ifdef GLUTEN_ENABLE_GPU | ||
| #include <cudf/io/types.hpp> | ||
| #include "cudf/GpuLock.h" | ||
| #include "velox/experimental/cudf/CudfConfig.h" | ||
| #include "velox/experimental/cudf/connectors/hive/CudfHiveConnectorSplit.h" | ||
| #include "velox/experimental/cudf/exec/ToCudf.h" | ||
| #include "cudf/GpuLock.h" | ||
| #endif | ||
| #include "operators/plannodes/RowVectorStream.h" | ||
|
|
||
|
|
||
| using namespace facebook; | ||
|
|
||
| namespace gluten { | ||
|
|
@@ -358,14 +357,15 @@ void WholeStageResultIterator::constructPartitionColumns( | |
| } | ||
|
|
||
| void WholeStageResultIterator::addIteratorSplits(const std::vector<std::shared_ptr<ResultIterator>>& inputIterators) { | ||
| GLUTEN_CHECK(!allSplitsAdded_, "Method addIteratorSplits should not be called since all splits has been added to the Velox task."); | ||
| GLUTEN_CHECK( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove unrelated changes
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @jinchengchenghh removed dist change |
||
| !allSplitsAdded_, | ||
| "Method addIteratorSplits should not be called since all splits has been added to the Velox task."); | ||
| // Create IteratorConnectorSplit for each iterator | ||
| for (size_t i = 0; i < streamIds_.size() && i < inputIterators.size(); ++i) { | ||
| if (inputIterators[i] == nullptr) { | ||
| continue; | ||
| } | ||
| auto connectorSplit = std::make_shared<IteratorConnectorSplit>( | ||
| kIteratorConnectorId, inputIterators[i]); | ||
| auto connectorSplit = std::make_shared<IteratorConnectorSplit>(kIteratorConnectorId, inputIterators[i]); | ||
| exec::Split split(folly::copy(connectorSplit), -1); | ||
| task_->addSplit(streamIds_[i], std::move(split)); | ||
| } | ||
|
|
@@ -385,7 +385,7 @@ void WholeStageResultIterator::noMoreSplits() { | |
| for (const auto& scanNodeId : scanNodeIds_) { | ||
| task_->noMoreSplits(scanNodeId); | ||
| } | ||
|
|
||
| // Mark no more splits for all stream nodes | ||
| for (const auto& streamId : streamIds_) { | ||
| task_->noMoreSplits(streamId); | ||
|
|
@@ -575,7 +575,8 @@ std::unordered_map<std::string, std::string> WholeStageResultIterator::getQueryC | |
| std::to_string(veloxCfg_->get<uint64_t>(kVeloxPreferredBatchBytes, 10L << 20)); | ||
| try { | ||
| configs[velox::core::QueryConfig::kSparkAnsiEnabled] = veloxCfg_->get<std::string>(kAnsiEnabled, "false"); | ||
| configs[velox::core::QueryConfig::kSessionTimezone] = veloxCfg_->get<std::string>(kSessionTimezone, ""); | ||
| configs[velox::core::QueryConfig::kSessionTimezone] = | ||
| normalizeSessionTimezone(veloxCfg_->get<std::string>(kSessionTimezone, "")); | ||
| // Adjust timestamp according to the above configured session timezone. | ||
| configs[velox::core::QueryConfig::kAdjustTimestampToTimezone] = "true"; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
|
|
||
| #include <unordered_map> | ||
| #include "SubstraitToVeloxPlan.h" | ||
| #include "config/GlutenConfig.h" | ||
| #include "velox/core/QueryCtx.h" | ||
|
|
||
| using namespace facebook; | ||
|
|
@@ -29,9 +30,15 @@ namespace gluten { | |
| /// a Substrait plan is supported in Velox. | ||
| class SubstraitToVeloxPlanValidator { | ||
| public: | ||
| SubstraitToVeloxPlanValidator(memory::MemoryPool* pool) { | ||
| explicit SubstraitToVeloxPlanValidator( | ||
| memory::MemoryPool* pool, | ||
| const std::unordered_map<std::string, std::string>& confMap = {}) { | ||
| const auto it = confMap.find(kSessionTimezone); | ||
| const auto sessionTimezone = | ||
| normalizeSessionTimezone(it == confMap.end() ? std::string_view("UTC") : std::string_view(it->second)); | ||
| std::unordered_map<std::string, std::string> configs{ | ||
| {velox::core::QueryConfig::kSparkPartitionId, "0"}, {velox::core::QueryConfig::kSessionTimezone, "GMT"}}; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. update the GMT to other value is enough
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, addressed. I updated |
||
| {velox::core::QueryConfig::kSparkPartitionId, "0"}, | ||
| {velox::core::QueryConfig::kSessionTimezone, sessionTimezone}}; | ||
| veloxCfg_ = std::make_shared<facebook::velox::config::ConfigBase>(std::move(configs)); | ||
| planConverter_ = std::make_unique<SubstraitToVeloxPlanConverter>( | ||
| pool, veloxCfg_.get(), std::vector<std::shared_ptr<ResultIterator>>{}, std::nullopt, std::nullopt, true); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it more appropriate to move the normalization step to Scala before sending data to native, so that the timezone received by C++ is always correct and we don’t need to apply normalization in every caller of the session timezone?