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

Add native_op_trace_directory_create_config session property and setDirectoryCB in native #24156

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

Conversation

yuandagits
Copy link

@yuandagits yuandagits commented Nov 26, 2024

Description

In native engine, add ability to set a create directory callback and add the corresponding config as a property. We also expose native_op_trace_directory_create_config as a session property.

Motivation and Context

See facebookincubator/velox#11631 for operator directory option support and API to set create directory facebookincubator/velox#11572

Impact

Test Plan

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • 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
* ... :pr:`12345`
* ... :pr:`12345`

Hive Connector Changes
* ... :pr:`12345`
* ... :pr:`12345`

If release note is NOT required, use:

== NO RELEASE NOTE ==

Copy link

linux-foundation-easycla bot commented Nov 26, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: yuandagits / name: Yuanda (Yenda) Li (7cf709d)

@yuandagits yuandagits marked this pull request as ready for review November 26, 2024 20:57
xiaoxmeng
xiaoxmeng previously approved these changes Nov 27, 2024
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@yuandagits LGTM % minors. Thanks!

execTask.queryCtx()->queryId(),
execTask.taskId(),
includeNodeInSpillPath);
const auto [taskSpillDirPath, taskDateStrPath] =
Copy link
Contributor

Choose a reason for hiding this comment

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

s/taskDateStrPath/dateSpillDirPath/?

@@ -402,8 +424,12 @@ std::unique_ptr<TaskInfo> TaskManager::createOrUpdateErrorTask(
if (includeNodeInSpillPath) {
folly::toAppend(fmt::format("{}_{}/", nodeIp, nodeId), &path);
}

std::string dateStringPath = path;
Copy link
Contributor

Choose a reason for hiding this comment

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

s/dateStringPath/dateSpillDirPath/

folly::toAppend(fmt::format("{}/{}/{}/", dateString, queryId, taskId), &path);
return path;
return std::make_tuple(std::move(path), std::move(dateStringPath));
Copy link
Contributor

Choose a reason for hiding this comment

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

s/path/taskSpillDirPath/

@@ -154,8 +154,9 @@ class TaskManager {
std::vector<velox::exec::Task::OpCallInfo>& stuckOpCalls) const;

/// Build directory path for spilling for the given task.
/// Always returns non-empty string.
static std::string buildTaskSpillDirectoryPath(
/// Always returns tuple of non-empty string containing the spill directory
Copy link
Contributor

Choose a reason for hiding this comment

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

the task spill directory and date spill directory which is parent directory of task spill directory.

Not sure if we want to return the two as the last two output parameters of buildTaskSpillDirectoryPath?

std::string& taskSpillDirPath,
std::string& dateSpillDirPath);

@yuandagits yuandagits marked this pull request as draft November 27, 2024 22:16
@yuandagits yuandagits force-pushed the directoryTtlSupport branch 11 times, most recently from 12d8cca to d5fe7d7 Compare November 30, 2024 03:41
@yuandagits yuandagits marked this pull request as ready for review November 30, 2024 04:16
@yuandagits yuandagits force-pushed the directoryTtlSupport branch 2 times, most recently from dcc1631 to bd10f61 Compare November 30, 2024 18:10
@yuandagits yuandagits marked this pull request as draft November 30, 2024 18:11
@yuandagits
Copy link
Author

yuandagits commented Nov 30, 2024

Pushed a wrong commit by mistake after rebase. After fixing, re-pushing https://github.com/yuandagits/presto/tree/directoryTtlSupport is now stuck on processing update

Might abandon and recreate PR if the processing does not complete

Edit: it finally finished processing...

In native engine, add ability to set a create directory callback and add the corresponding config as a property. We also expose native_op_trace_directory_create_config as a session property.

[Description]
In native engine, add ability to set a create directory callback and add the corresponding config as a property. We also expose native_op_trace_directory_create_config as a session property.
@yuandagits yuandagits marked this pull request as ready for review November 30, 2024 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants