Table writer 6: create directory if not exists #3414

gggrace14 · 2022-12-05T02:58:51Z

If the parent directory of the file that a LocalFileSink writes to
does not exist, create the directory first when creating
the LocalFileSink.

Rename FileSink to LocalFileSink, to be more clear.
LocalFileSink is one subclass of the interface
dwio::common::DataSink that implements a local file
system based data sink for a dwio writer.

This is needed by a partitioned table writer, which calls on
the LocalFileSink to write to files under partition subdirectories
that are created during query execution.

netlify · 2022-12-05T02:58:58Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`7fa3d5d`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/639aa3afba23900009328859

facebook-github-bot · 2022-12-05T03:01:14Z

@gggrace14 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-12-05T03:29:04Z

@gggrace14 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

xiaoxmeng · 2022-12-06T03:29:05Z

velox/dwio/common/DataSink.cpp

@@ -28,6 +30,10 @@ FileSink::FileSink(
    const MetricsLogPtr& metricLogger,
    IoStatistics* stats)
    : DataSink{name, metricLogger, stats} {
+  auto dir = fs::path(name).parent_path();


Shall we use Velox filesystem here?

Hi @xiaoxmeng , I find DataSink abstraction and Velox FileSystem abstraction are kept separate now, ex., we register DataSink independently,

velox/velox/dwio/common/DataSink.h

Line 200 in d38a666

#define VELOX_STATIC_REGISTER_DATA_SINK(function) \

.
Current Velox has separate implementation of local FileSink and WarmStorageBlockSink, https://fburl.com/code/usn9qvb8. Each calls their corresponding "file system" APIs for file ops. I think we should follow this pattern.

As you might know, the reason we call Velox FileSystem interface is to auto dispatch to the right "file system" APIs based on the file path prefix. Here we're already with the local file DataSink, so we don't need the FileSystem interface for the Polymorphism

Shall we use the same set of interface to access file system. My understanding is that local FileSink and WarmStorageBlockSink could be a layer on top of file system abstraction. Ideally, we could have one DataSink object built on top of different file system interfaces?

@gggrace14 Can we use Velox filesystem interface to access local file system instead of directly calling posix APIs. The same for ws based data sink? Thanks!

mbasmanova

@gggrace14 Would you add a test and update PR description to provide some more details about the change?

mbasmanova

@gggrace14 Is FileSink a test-only class? If so, it would be cleaner to move it into test namespace and document accordingly.

gggrace14 · 2022-12-07T21:20:39Z

@gggrace14 Is FileSink a test-only class? If so, it would be cleaner to move it into test namespace and document accordingly.

Hi @mbasmanova , nope, FileSink is prod class and being called by table writing on a local Presto server.

mbasmanova · 2022-12-07T21:26:46Z

@gggrace14 Ge, thank you for clarifying. I see that FileSink is an implementation of DataSink interface for the local file system. I guess it would be clearer if it was called LocalFileSink :-)

facebook-github-bot · 2022-12-08T07:15:14Z

@gggrace14 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

gggrace14 · 2022-12-08T07:16:18Z

@gggrace14 Ge, thank you for clarifying. I see that FileSink is an implementation of DataSink interface for the local file system. I guess it would be clearer if it was called LocalFileSink :-)

@mbasmanova Hi Masha, good suggestion. I refactored to rename FileSink to LocalFileSink, to make the name more clear (and consistent w/ the class LocalFileSystem that we have).

It touches several files, so will wait for the CicleCi tests to be all green.

There're many deps on FileSink in fbcode. Also adapting them

facebook-github-bot · 2022-12-13T09:35:38Z

@gggrace14 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-12-13T09:36:40Z

@gggrace14 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-12-13T09:42:31Z

@gggrace14 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

mbasmanova

@gggrace14 Looks good to me % some questions below.

mbasmanova · 2022-12-13T20:54:09Z

velox/dwio/common/tests/DataSinkTest.cpp

+
+namespace facebook::velox::dwio::common {
+
+TEST(LocalFileSinkTests, create) {


naming: LocalFileSinkTests -> LocalFileSinkTest and the file name should be LocalFileSinkTest.cpp

mbasmanova · 2022-12-13T20:55:19Z

velox/dwio/common/tests/DataSinkTest.cpp

+  auto subdir = fs::path();
+  for (auto i = 0; i < 4; i++) {
+    subdir =
+        subdir / boost::uuids::to_string(boost::uuids::random_generator()());


The test would be more readable if only top-level directory name is auto-generated, but the rest of the directories and the file name are hard-coded.

I see. Revised.

mbasmanova · 2022-12-13T20:55:39Z

velox/dwio/common/tests/DataSinkTest.cpp

+  }
+  auto fileName = boost::uuids::to_string(boost::uuids::random_generator()());
+
+  auto filePath = root / subdir / fileName;


nit: perhaps, add a check that root / subdir doesn't exist.

mbasmanova · 2022-12-13T20:56:20Z

velox/dwio/common/tests/DataSinkTest.cpp

+      DataSink::create(fmt::format("file:{}", filePath.string()));
+  localFileSink->close();
+
+  EXPECT_TRUE(fs::exists(filePath.string()));


Can we make sure the file is deleted at the end of the test?

The root is created by TempDirectoryPath::create(), and I see the destructor of TempDirectoryPath is like

velox/velox/exec/tests/utils/TempDirectoryPath.cpp

Lines 30 to 32 in 05ad3d0

TempDirectoryPath::~TempDirectoryPath() {

boost::filesystem::remove_all(path.c_str());

}

Also TempDirectoryPath::create() calls mkdtemp() underlying

velox/velox/exec/tests/utils/TempDirectoryPath.h

Lines 41 to 43 in 05ad3d0

static std::string createTempDirectory() {

char path[] = "/tmp/velox_test_XXXXXX";

const char* tempDirectoryPath = mkdtemp(path);

So my understanding is the root directory would be deleted when the variable leaves its scope.

auto root = fs::path(TempDirectoryPath::create()->path);

This code creates directory at 'root' path and deletes it (because the result of TempDirectoryPath::create() is not stored in a variable and goes out or scope after the call.). After this call, I expect root to not exist. Hence, it is not clear which code will take care of deleting the directory at the end of the test. Would you double check that the directory is indeed deleted?

Revised, and checked the directory gets deleted after the unit test finishes.

I see what you mean, and you're right. As I didn't store in a variable TempDirectoryPath::create() but stored ->path, the destructor ~TempDirectoryPath() is called right away after this line. So no directory deletion will be called after the test finishes.

mbasmanova · 2022-12-13T20:57:13Z

velox/connectors/hive/HiveConnector.h

@@ -270,12 +270,12 @@ class HiveConnectorFactory : public ConnectorFactory {
      "hive-hadoop2";

  HiveConnectorFactory() : ConnectorFactory(kHiveConnectorName) {
-    dwio::common::FileSink::registerFactory();


I wonder why do we need to register local file sink in the Hive Connector? Shouldn't the application using Hive connector register whatever file system it is going to use?

My best guess is it just tries to make sure at least one DataSink, the LocalFileSink, is registered, as Velox itself also needs it for, ex., unit tests.

It feels like it is not HiveConnector's job to setup things for unit tests. It would be nice to remove this code in a follow-up PR.

Will follow up with a separate PR and move it to the right place.

mbasmanova

@gggrace14 Looks good % a couple of nits and a question about cleaning up test directory after the test.

mbasmanova · 2022-12-14T10:34:20Z

velox/dwio/common/tests/LocalFileSinkTest.cpp

+  LocalFileSink::registerFactory();
+
+  auto root = fs::path(TempDirectoryPath::create()->path);
+  auto filePath = root / "test_dir_xxx/test_dir_yyy/test_dir_zzz/test_file";


nit: "xxx/yyy/zzz/test_file.ext" might be easier to read

mbasmanova · 2022-12-14T10:34:44Z

velox/dwio/common/tests/LocalFileSinkTest.cpp

+  auto root = fs::path(TempDirectoryPath::create()->path);
+  auto filePath = root / "test_dir_xxx/test_dir_yyy/test_dir_zzz/test_file";
+
+  ASSERT_FALSE(fs::exists((filePath.string())));


nit: redundant parentheses around filePath.string()

mbasmanova · 2022-12-14T10:35:54Z

velox/connectors/hive/HiveConnector.h

@@ -270,12 +270,12 @@ class HiveConnectorFactory : public ConnectorFactory {
      "hive-hadoop2";

  HiveConnectorFactory() : ConnectorFactory(kHiveConnectorName) {
-    dwio::common::FileSink::registerFactory();


It feels like it is not HiveConnector's job to setup things for unit tests. It would be nice to remove this code in a follow-up PR.

mbasmanova · 2022-12-14T10:39:14Z

velox/dwio/common/tests/DataSinkTest.cpp

+      DataSink::create(fmt::format("file:{}", filePath.string()));
+  localFileSink->close();
+
+  EXPECT_TRUE(fs::exists(filePath.string()));


auto root = fs::path(TempDirectoryPath::create()->path);

This code creates directory at 'root' path and deletes it (because the result of TempDirectoryPath::create() is not stored in a variable and goes out or scope after the call.). After this call, I expect root to not exist. Hence, it is not clear which code will take care of deleting the directory at the end of the test. Would you double check that the directory is indeed deleted?

Summary: If the parent directory of the file that a LocalFileSink writes to does not exist, create the directory first when creating the LocalFileSink. Rename FileSink to LocalFileSink, to be more clear. LocalFileSink is one subclass of the interface dwio::common::DataSink that implements a local file system based data sink for a dwio writer. This is needed by a partitioned table writer, which calls on the LocalFileSink to write to files under partition subdirectories that are created during query execution. Pull Request resolved: facebookincubator#3414 Test Plan: Imported from GitHub, without a `Test Plan:` line. buck build //fb_presto_cpp:main buck test velox/dwio/common/tests: Reviewed By: mbasmanova Differential Revision: D41723673 Pulled By: gggrace14 fbshipit-source-id: 6aab5e9504f6b4c284f26c85eb664dd415d5db7c

facebook-github-bot · 2022-12-15T04:33:53Z

This pull request was exported from Phabricator. Differential Revision: D41723673

facebook-github-bot · 2022-12-15T05:56:47Z

@gggrace14 merged this pull request in 8127e86.

gggrace14 requested review from mbasmanova and oerling December 5, 2022 02:58

facebook-github-bot added the CLA Signed label Dec 5, 2022

gggrace14 force-pushed the mkdir branch from a497ad3 to 606da5f Compare December 5, 2022 03:27

xiaoxmeng reviewed Dec 6, 2022

View reviewed changes

mbasmanova reviewed Dec 7, 2022

View reviewed changes

gggrace14 force-pushed the mkdir branch from 606da5f to b330da5 Compare December 8, 2022 07:14

gggrace14 closed this Dec 8, 2022

gggrace14 reopened this Dec 8, 2022

gggrace14 force-pushed the mkdir branch from b330da5 to 8bc5907 Compare December 13, 2022 09:35

gggrace14 force-pushed the mkdir branch from 8bc5907 to c6c1133 Compare December 13, 2022 09:41

gggrace14 force-pushed the mkdir branch from c6c1133 to 22068cf Compare December 13, 2022 10:01

mbasmanova reviewed Dec 13, 2022

View reviewed changes

gggrace14 force-pushed the mkdir branch from 22068cf to 2a05d19 Compare December 14, 2022 08:02

mbasmanova approved these changes Dec 14, 2022

View reviewed changes

gggrace14 force-pushed the mkdir branch from 2a05d19 to aee51a0 Compare December 15, 2022 02:41

gggrace14 force-pushed the mkdir branch from aee51a0 to 7fa3d5d Compare December 15, 2022 04:33

xiaoxmeng approved these changes Dec 15, 2022

View reviewed changes

facebook-github-bot closed this in 8127e86 Dec 15, 2022

gggrace14 deleted the mkdir branch December 15, 2022 05:56

facebook-github-bot added the Merged label Dec 15, 2022


		namespace facebook::velox::dwio::common {

		TEST(LocalFileSinkTests, create) {

	TempDirectoryPath::~TempDirectoryPath() {
	boost::filesystem::remove_all(path.c_str());
	}

	static std::string createTempDirectory() {
	char path[] = "/tmp/velox_test_XXXXXX";
	const char* tempDirectoryPath = mkdtemp(path);

Table writer 6: create directory if not exists #3414

Table writer 6: create directory if not exists #3414

Conversation

gggrace14 commented Dec 5, 2022 • edited Loading

netlify bot commented Dec 5, 2022 • edited Loading

✅ Deploy Preview for meta-velox canceled.

facebook-github-bot commented Dec 5, 2022

facebook-github-bot commented Dec 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaoxmeng Dec 9, 2022 • edited Loading

Choose a reason for hiding this comment

mbasmanova left a comment

Choose a reason for hiding this comment

mbasmanova left a comment

Choose a reason for hiding this comment

gggrace14 commented Dec 7, 2022

mbasmanova commented Dec 7, 2022

facebook-github-bot commented Dec 8, 2022

gggrace14 commented Dec 8, 2022 • edited Loading

facebook-github-bot commented Dec 13, 2022

facebook-github-bot commented Dec 13, 2022

facebook-github-bot commented Dec 13, 2022

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gggrace14 Dec 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 15, 2022

facebook-github-bot commented Dec 15, 2022

gggrace14 commented Dec 5, 2022 •

edited

Loading

netlify bot commented Dec 5, 2022 •

edited

Loading

xiaoxmeng Dec 9, 2022 •

edited

Loading

gggrace14 commented Dec 8, 2022 •

edited

Loading

gggrace14 Dec 14, 2022 •

edited

Loading