Add ignore_missing_files Hive config #8615

zhli1142015 · 2024-01-31T10:36:35Z

Spark provides support for the data source configuration
spark.sql.files.ignoreMissingFiles, to ignore missing files while reading data
from files.
https://spark.apache.org/docs/latest/sql-data-sources-generic-options.html#ignore-missing-files

Introduce Hive config ignore_missing_files to match this behavior.

netlify · 2024-01-31T10:36:40Z

✅ Deploy Preview for meta-velox ready!

Name	Link
🔨 Latest commit	`d42a35f`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/65bcda10b264550008d41aa7
😎 Deploy Preview	https://deploy-preview-8615--meta-velox.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

zhli1142015 · 2024-02-01T02:58:51Z

@rui-mo and @mbasmanova , could you help review this PR?
Thanks.

rui-mo · 2024-02-01T08:08:39Z

velox/connectors/hive/SplitReader.cpp

+  std::shared_ptr<FileHandle> fileHandle = nullptr;
+  try {
+    fileHandle = fileHandleFactory_->generate(hiveSplit_->filePath).second;
+  } catch (VeloxRuntimeError& e) {


I have seen some discussions about the poor performance of try-catch, and am not sure if there's better way to achieve this. Maybe Status is an option. Thanks.

@rui-mo Throwing is problematic when it happens per-row. Here, throwing should be fine.

rui-mo · 2024-02-01T08:09:21Z

velox/common/file/File.cpp

-      folly::errnoStr(errno));
+  if (fd_ < 0) {
+    if (errno == ENOENT) {
+      VELOX_FILE_NOT_FOUND_ERROR("file {} does not exist", path);


nit: messages ends with dot .

mbasmanova

@zhli1142015 Looks good overall. Some comments.

mbasmanova · 2024-02-01T11:19:30Z

velox/common/file/tests/FileTest.cpp

+  auto tempFolder = ::exec::test::TempDirectoryPath::create();
+  auto path = fmt::format("{}/file", tempFolder->path);
+  auto localFs = filesystems::getFileSystem(path, nullptr);
+  {


nit: remove this scope

mbasmanova · 2024-02-01T11:19:44Z

velox/common/file/tests/FileTest.cpp

+  {
+    try {
+      LocalReadFile readFile(path);
+      ASSERT_FALSE(true) << "Function should throw.";


mbasmanova · 2024-02-01T11:21:23Z

velox/connectors/hive/HiveConfig.cpp

@@ -152,6 +152,14 @@ bool HiveConfig::isPartitionPathAsLowerCase(const Config* session) const {
  return session->get<bool>(kPartitionPathAsLowerCaseSession, true);
 }

+bool HiveConfig::ignoreMissingFiles(const Config* session) const {
+  if (session->isValueExists(kIgnoreMissingFilesSession)) {


This pattern of double checking (exists + get) is an anti-pattern. Let's change to session->get<T>(name, defaultValue). Would you make a separate PR for that? CC: @kewang1024

Updated, thanks.

#8633, thanks.

mbasmanova · 2024-02-01T11:22:54Z

velox/connectors/hive/SplitReader.cpp

+  std::shared_ptr<FileHandle> fileHandle = nullptr;
+  try {
+    fileHandle = fileHandleFactory_->generate(hiveSplit_->filePath).second;
+  } catch (VeloxRuntimeError& e) {


@rui-mo Throwing is problematic when it happens per-row. Here, throwing should be fine.

mbasmanova · 2024-02-01T11:23:08Z

velox/connectors/hive/SplitReader.cpp

@@ -91,7 +92,19 @@ void SplitReader::prepareSplit(
  VELOX_CHECK_NE(
      baseReaderOpts_.getFileFormat(), dwio::common::FileFormat::UNKNOWN);

-  auto fileHandle = fileHandleFactory_->generate(hiveSplit_->filePath).second;
+  std::shared_ptr<FileHandle> fileHandle = nullptr;


drop = nullptr

Updated, thanks.

mbasmanova · 2024-02-01T11:26:08Z

velox/exec/tests/TableScanTest.cpp

+              connector::hive::HiveConfig::kIgnoreMissingFilesSession,
+              std::to_string(ignoreMissingFile))
+          .splits({split})
+          .assertResults("");


use assertEmptyResults

Updated, thanks.

mbasmanova · 2024-02-01T11:26:16Z

velox/exec/tests/TableScanTest.cpp

+              kHiveConnectorId,
+              connector::hive::HiveConfig::kIgnoreMissingFilesSession,
+              std::to_string(ignoreMissingFile))
+          .splits({split})


use split(split)

Updated, thanks.

mbasmanova · 2024-02-01T11:26:44Z

velox/exec/tests/TableScanTest.cpp

-  EXPECT_THROW(cursor->moveNext(), VeloxRuntimeError);
+  auto split = HiveConnectorSplitBuilder("/path/to/nowhere.orc").build();
+  for (bool ignoreMissingFiles : {true, false}) {
+    auto assertQueryForMissingFile = [&](bool ignoreMissingFile) {


auto assertMissingFile = .

Updated, thanks.

mbasmanova · 2024-02-01T11:27:07Z

velox/exec/tests/TableScanTest.cpp

+      assertQueryForMissingFile(ignoreMissingFiles);
+    } else {
+      try {
+        assertQueryForMissingFile(ignoreMissingFiles);


use VELOX_ASSERT_THROW

Added, thanks.

mbasmanova · 2024-02-01T11:27:42Z

velox/common/file/tests/FileTest.cpp

+      LocalReadFile readFile(path);
+      ASSERT_FALSE(true) << "Function should throw.";
+    } catch (VeloxRuntimeError& e) {
+      ASSERT_EQ(e.errorCode(), error_code::kFileNotFound);


Let's add a macro similar to VELOX_ASSERT_THROW that can check error code as well.

Added, thanks.

mbasmanova

Looks great % a couple nits.

mbasmanova · 2024-02-01T17:15:59Z

velox/common/base/tests/GTestUtils.h

@@ -63,6 +63,16 @@
      << "Expected error message to contain '" << (_errorMessage)          \
      << "', but received '" << status.message() << "'."

+#define VELOX_ASSERT_ERROR_CODE(_expression, _type, _errorCode)               \


nit: perhaps, VELOX_ASSERT_RUNTIME_THROW_CODE(_expression, _errorCode) to match VELOX_ASSERT_RUNTIME_THROW

Added, thanks.

mbasmanova · 2024-02-01T17:17:44Z

velox/exec/tests/TableScanTest.cpp

-  cursor->task()->addSplit("0", makeHiveSplit("/path/to/nowhere.orc"));
-  EXPECT_THROW(cursor->moveNext(), VeloxRuntimeError);
+  auto split = HiveConnectorSplitBuilder("/path/to/nowhere.orc").build();
+  for (bool ignoreMissingFiles : {true, false}) {


nit: perhaps, it would be more readable as

assertMissingFile(true); VELOX_ASSERT_RUNTIME_CODE_THROW( assertMissingFile(false), error_code::kFileNotFound);

without the loop

Updated, thanks.

mbasmanova

Thanks.

facebook-github-bot · 2024-02-02T00:42:02Z

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

mbasmanova · 2024-02-02T00:42:21Z

velox/connectors/hive/SplitReader.cpp

-  auto fileHandle = fileHandleFactory_->generate(hiveSplit_->filePath).second;
+  std::shared_ptr<FileHandle> fileHandle;
+  try {
+    fileHandle = fileHandleFactory_->generate(hiveSplit_->filePath).second;


Would you also document 'generate' API to explain that it should throw a specific error code to indicate file-not-found?

I plan to add doc for this in the follow up pr, sth like: c195d90#diff-b2b8b019f80c44db000432c6c90bfffeb215c772b15908246f736ddd30011d1eR84. Please let me know if other docs are needed, thanks.

mbasmanova

@zhli1142015 This change is breaking Prestissimo tests.

[ RUN      ] BroadcastTest.invalidBroadcastFilePath
fbcode/github/presto-trunk/presto-native-execution/presto_cpp/main/operators/tests/BroadcastTest.cpp:339: Failure
Value of: e.message().find("No such file or directory") != std::string::npos
  Actual: false
Expected: true
Expected error message to contain 'No such file or directory', but received 'file /tmp/this-should-not-exist/velox--missing-broadcast-file.bin does not exist.'.

Maybe change

VELOX_FILE_NOT_FOUND_ERROR("file {} does not exist.", path);

to

VELOX_FILE_NOT_FOUND_ERROR("No such file or directory: {}", path);

zhli1142015 · 2024-02-02T12:04:31Z

@zhli1142015 This change is breaking Prestissimo tests.

[ RUN      ] BroadcastTest.invalidBroadcastFilePath
fbcode/github/presto-trunk/presto-native-execution/presto_cpp/main/operators/tests/BroadcastTest.cpp:339: Failure
Value of: e.message().find("No such file or directory") != std::string::npos
  Actual: false
Expected: true
Expected error message to contain 'No such file or directory', but received 'file /tmp/this-should-not-exist/velox--missing-broadcast-file.bin does not exist.'.

Maybe change

VELOX_FILE_NOT_FOUND_ERROR("file {} does not exist.", path);

to

VELOX_FILE_NOT_FOUND_ERROR("No such file or directory: {}", path);

Updated, thanks.

facebook-github-bot · 2024-02-02T12:34:14Z

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

facebook-github-bot · 2024-02-02T18:46:03Z

@mbasmanova merged this pull request in 3c2b74a.

conbench-facebook · 2024-02-02T19:26:41Z

Conbench analyzed the 1 benchmark run on commit 3c2b74a7.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Summary: Spark provides support for the data source configuration `spark.sql.files.ignoreMissingFiles`, to ignore missing files while reading data from files. https://spark.apache.org/docs/latest/sql-data-sources-generic-options.html#ignore-missing-files Introduce Hive config ignore_missing_files to match this behavior. Pull Request resolved: facebookincubator#8615 Reviewed By: xiaoxmeng Differential Revision: D53335663 Pulled By: mbasmanova fbshipit-source-id: 328e9abb20dba7a9b1c724e372967aa86b67c90f

Add hive config 'ignore_missing_files'

95b2ad6

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 31, 2024

minor change keep same runtime error

7cfc2ab

rui-mo reviewed Feb 1, 2024

View reviewed changes

mbasmanova requested review from Yuhta and majetideepak February 1, 2024 11:17

mbasmanova reviewed Feb 1, 2024

View reviewed changes

address comments

72cba94

zhli1142015 requested a review from mbasmanova February 1, 2024 15:51

mbasmanova changed the title ~~Add hive config 'ignore_missing_files'~~ Add Hive config 'ignore_missing_files' Feb 1, 2024

mbasmanova changed the title ~~Add Hive config 'ignore_missing_files'~~ Add ignore_missing_files Hive config Feb 1, 2024

mbasmanova reviewed Feb 1, 2024

View reviewed changes

address comments

e4fb08f

mbasmanova approved these changes Feb 2, 2024

View reviewed changes

mbasmanova reviewed Feb 2, 2024

View reviewed changes

fix build

d42a35f

facebook-github-bot closed this in 3c2b74a Feb 2, 2024

facebook-github-bot added the Merged label Feb 2, 2024

zhli1142015 mentioned this pull request Feb 5, 2024

Throw kFileNotFound in all storage adapters #8662

Closed

zhli1142015 mentioned this pull request Feb 21, 2024

[Gluten-4585][VL] Support spark.sql.files.ignoreMissingFiles=true apache/incubator-gluten#4725

Merged

Add ignore_missing_files Hive config #8615

Add ignore_missing_files Hive config #8615

Conversation

zhli1142015 commented Jan 31, 2024 • edited by mbasmanova Loading

netlify bot commented Jan 31, 2024 • edited Loading

✅ Deploy Preview for meta-velox ready!

zhli1142015 commented Feb 1, 2024

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

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

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

mbasmanova left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 2, 2024

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

zhli1142015 commented Feb 2, 2024

facebook-github-bot commented Feb 2, 2024

facebook-github-bot commented Feb 2, 2024

conbench-facebook bot commented Feb 2, 2024

zhli1142015 commented Jan 31, 2024 •

edited by mbasmanova

Loading

netlify bot commented Jan 31, 2024 •

edited

Loading