Add property to skip empty files in Hive Connector #22727

cvarelad-denodo · 2024-05-13T12:08:46Z

Ignore empty files when hive.skip_empty_files is set to true (false by default). Otherwise, it produces an error when iterating through these files.

Description

This support allows to configure a hive connector property to decide if Presto should ignore these empty files or it should fail (default behavior). The new connector property is called hive.skip_empty_files. The following diagram shows the flow of the property to the HiveFileIterator:

Motivation and Context

Currently, if Presto reads a Parquet folder where one of the files is an empty file the query fails. This is a problem in some environments.

Solves issue: 14759

Impact

No impact

Test Plan

TestHiveSkipEmptyFiles contains two tests. Both reads a directory that contains an empty file. One has the property set to true and the query execution finish with no errors skipping the empty file. The other one has the property set to false and the execution fails as before these changes

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
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 ==

Hive Connector Changes
* Add support to skip empty files using configuration property ``hive.skip_empty_files`` :pr:`22727`

Ignore empty files when hive.skip_empty_files is set to true. Otherwise, it produces an error when iterating through these files.

linux-foundation-easycla · 2024-05-13T12:08:50Z

The committers listed above are authorized under a signed CLA.

✅ login: cvarelad-denodo / name: cvarelad-denodo (ca18e6f, e566d79, e50656b, 6b1d9c6, 5d28022, 0d87129)

…StatsProvider

elharo · 2024-05-13T15:04:17Z

presto-docs/src/main/sphinx/connector/hive.rst

@@ -195,6 +195,9 @@ Property Name                                      Description
                                                   S3SelectPushdown.

 ``hive.metastore.load-balancing-enabled``          Enable load balancing between multiple Metastore instances
+
+``hive.skip-empty-files``                          Enable skipping empty files. Otherwise, it will produce an   ``false``


a false
explicitly specify the default value of this property
What is a "false error"?

screenshot of local docs build - the doc when built may answer all of these issues

Ah, it's a table.

elharo · 2024-05-13T15:05:56Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveClientConfig.java

@@ -1816,4 +1818,17 @@ public HiveClientConfig setAffinitySchedulingFileSectionSize(DataSize affinitySc
        this.affinitySchedulingFileSectionSize = affinitySchedulingFileSectionSize;
        return this;
    }
+
+    @Config("hive.skip-empty-files")
+    @ConfigDescription("Enables skip of parquet empty files avoiding output error")


Skip empty files rather than reporting an error
What about ORC files?

elharo · 2024-05-13T15:06:11Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveClientConfig.java

+
+    @Config("hive.skip-empty-files")
+    @ConfigDescription("Enables skip of parquet empty files avoiding output error")
+    public HiveClientConfig setSkipEmptyFilesEnabled(boolean parallelParsingOfPartitionValuesEnabled)


change argument name

elharo · 2024-05-13T15:07:02Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveSkipEmptyFiles.java

+    private void createQueryFailRunner()
+            throws Exception
+    {
+        logger.info("Creating 'QueryFailRunner'");


no logs in tests please.

elharo · 2024-05-13T15:07:48Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveSkipEmptyFiles.java

+    protected QueryRunner createQueryRunner()
+            throws Exception
+    {
+        logger.info("Creating 'QueryRunner'");


no logs in tests please, only failure messages. Our logs are already too full of random messages to efficiently locate the actual failures.

elharo · 2024-05-13T15:08:47Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveSkipEmptyFiles.java

+     */
+    private static void deleteMetadata(File temporaryDirectory)
+    {
+        File[] data = temporaryDirectory.listFiles();


data --> files or metadataFiles or something like that

elharo · 2024-05-13T15:09:30Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveSkipEmptyFiles.java

+        boolean isDirectory = file.isDirectory();
+        if (file.delete()) {
+            if (isDirectory) {
+                logger.info("   deleted temporary directory: " + filePath);


elharo · 2024-05-13T15:09:55Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveSkipEmptyFiles.java

+        deleteAndLog(temporaryDirectory);
+    }
+
+    private static void deleteAndLog(File file)


I know JUNit has easier ways to manage temp files these days. Does TestNG?

I could use method FileUtils.deleteDirectory from commons-io but that module does not currently have that dependency and it's probably not worth it to add it just for this.

In JUnit you can just annotate a field with @TempFile and the framework manages creating and disposing it. Again, I don't know if TestNG has that yet.

I haven't found anything similar in TestNG

elharo · 2024-05-13T15:10:15Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveSkipEmptyFiles.java

+        if (resourceUrl == null) {
+            throw new RuntimeException("Cannot find resource path for table name: " + tableName);
+        }
+        logger.info("resource url: " + resourceUrl);


elharo · 2024-05-13T15:10:27Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveSkipEmptyFiles.java

+     */
+    private void executeCreationTestAndDropCycle(DistributedQueryRunner queryRunner, String tableName, String externalLocation, boolean shouldFail, @Language("RegExp") String errorMessage)
+    {
+        logger.info("Executing Create - Test - Drop for: " + tableName);


steveburnett

Thanks, local build of doc looks good! One very minor nit of a suggestion to consider.

steveburnett · 2024-05-13T16:04:29Z

presto-docs/src/main/sphinx/connector/hive.rst

@@ -195,6 +195,9 @@ Property Name                                      Description
                                                   S3SelectPushdown.

 ``hive.metastore.load-balancing-enabled``          Enable load balancing between multiple Metastore instances
+
+``hive.skip-empty-files``                          Enable skipping empty files. Otherwise, it will produce an   ``false``


screenshot of local docs build - the doc when built may answer all of these issues

presto-docs/src/main/sphinx/connector/hive.rst

steveburnett

LGTM! (docs)
Pull updated branch, local docs build, looks good. Thanks!

steveburnett · 2024-05-15T14:52:40Z

Nit suggestion for the release note entry:

== RELEASE NOTES ==

Hive Connector Changes
* Add support to skip empty files using configuration property ``hive.skip_empty_files`` :pr:`22727`

tdcmeehan

I think this looks good. Could you look into adding a test when bucketed execution is enabled? I just want to make sure this continues to work when there's a definitive order required of the underlying files.

tdcmeehan · 2024-05-13T17:37:16Z

presto-hive/src/main/java/com/facebook/presto/hive/util/HiveFileIterator.java


    private Iterator<HiveFileInfo> remoteIterator = Collections.emptyIterator();

    public HiveFileIterator(
            Path path,
            ListDirectoryOperation listDirectoryOperation,
            NamenodeStats namenodeStats,
-            NestedDirectoryPolicy nestedDirectoryPolicy)
+            NestedDirectoryPolicy nestedDirectoryPolicy,
+            boolean skipEmptyFiles)// Tipo Boolean


Suggested change

boolean skipEmptyFiles)// Tipo Boolean

boolean skipEmptyFiles)

Bucketed execution is enabled by default. You mean add a test when it is disabled?

rereviewing

elharo · 2024-05-23T11:15:34Z

presto-docs/src/main/sphinx/connector/hive.rst

@@ -195,6 +195,9 @@ Property Name                                      Description
                                                   S3SelectPushdown.

 ``hive.metastore.load-balancing-enabled``          Enable load balancing between multiple Metastore instances
+
+``hive.skip-empty-files``                          Enable skipping empty files. Otherwise, it will produce an   ``false``


Ah, it's a table.

elharo · 2024-05-23T11:19:11Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveSkipEmptyFiles.java

+     * @param shouldFail {@code true} if the table creation should fail, {@code false} otherwise
+     * @param errorMessage a {@link String} containing the expected error message. Will be checked if {@code shouldFail} is {@code true}
+     */
+    private void checkBucketedResult(DistributedQueryRunner queryRunner, String tableName, boolean shouldFail, @Language("RegExp") String errorMessage)


boolean parameters to change behavior are an antipattern that increases cyclomatic complexity. Use two methods, one for things that should fail and one for things that should pass.

elharo · 2024-05-23T11:21:41Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveSkipEmptyFiles.java

+            throws Exception
+    {
+        // obtains the root resource directory in order to create temporary tables
+        URL url = TestHiveSkipEmptyFiles.class.getClassLoader().getResource(".");


Temporary files should be located with either the Java temp fiels API or TestNG APIs, not the classloader.

elharo · 2024-05-23T11:22:06Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveSkipEmptyFiles.java

+        // obtains the root resource directory in order to create temporary tables
+        URL url = TestHiveSkipEmptyFiles.class.getClassLoader().getResource(".");
+        if (url == null) {
+            throw new RuntimeException("Could not obtain resource URL");


This must be more specific. Never throw a raw RuntimeException.

elharo · 2024-05-23T11:23:56Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveSkipEmptyFiles.java

+        if (!created) {
+            throw new RuntimeException("Could not create resource directory: " + temporaryDirectory.getPath());
+        }
+        File firstParquetFile = new File(temporaryDirectory, randomUUID().toString());


You're reinventing a lot of temp file logic that already exists in the JDK, and probably in TestNG too.

elharo · 2024-05-23T11:24:39Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveSkipEmptyFiles.java

+     * @param temporaryDirectory a {@link File} pointing to the directory to delete
+     * @param hasPattern a {@code true} if it is necessary to delete only one directory file, {@code false} otherwise
+     */
+    private static void deleteMetadata(File temporaryDirectory, boolean hasPattern)


You've done far more work here than you needed, I'm afraid.

elharo · 2024-05-23T11:25:24Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveSkipEmptyFiles.java

+        String tableName = "skip_empty_files_success";
+        File resourcesLocation = generateMetadata(tableName);
+        executeCreationTestAndDropCycle(queryRunner, tableName, getResourceUrl(tableName), false, null);
+        deleteMetadata(resourcesLocation, false);


deletion in tests should be handled in an after test method to ensure cleanup in the face of exceptions

Add property to skip empty files in Hive Connector

6b1d9c6

Ignore empty files when hive.skip_empty_files is set to true. Otherwise, it produces an error when iterating through these files.

cvarelad-denodo requested review from steveburnett, a team, jaystarshot and feilong-liu as code owners May 13, 2024 12:08

cvarelad-denodo requested a review from presto-oss May 13, 2024 12:08

tdcmeehan self-assigned this May 13, 2024

cvarelad-denodo added 2 commits May 13, 2024 15:41

Fix code style in TestHiveSkipEmptyFiles

ca18e6f

Add property hive.skip_empty_files to session properties in TestQuick…

e50656b

…StatsProvider

elharo previously requested changes May 13, 2024

View reviewed changes

steveburnett reviewed May 13, 2024

View reviewed changes

Fixes in TesHiveSkipEmptyFiles and HiveClientConfig

e566d79

steveburnett previously approved these changes May 15, 2024

View reviewed changes

tdcmeehan reviewed May 15, 2024

View reviewed changes

Add tests for bucketed tables in TestHiveSkipEmptyFiles

5d28022

cvarelad-denodo dismissed steveburnett’s stale review via 5d28022 May 23, 2024 08:24

elharo requested changes May 23, 2024

View reviewed changes

Fix TestHiveSkipEmptyFiles temporal files and directories creation

0d87129

	boolean skipEmptyFiles)// Tipo Boolean
	boolean skipEmptyFiles)

Add property to skip empty files in Hive Connector #22727

Are you sure you want to change the base?

Add property to skip empty files in Hive Connector #22727

Conversation

cvarelad-denodo commented May 13, 2024 • edited

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

linux-foundation-easycla bot commented May 13, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett commented May 15, 2024

tdcmeehan 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

cvarelad-denodo commented May 13, 2024 •

edited

linux-foundation-easycla bot commented May 13, 2024 •

edited