enhancing compatibility with legacy ORC files #21391

ico01 · 2023-11-15T09:59:59Z

Description

This PR modifies the handling of the hive.orc.use-column-names configuration setting in Presto to allow for improved compatibility with older ORC data that may not contain column names in the file. The new strategy uses ORC file column names by default and falls back to Hive schema column names when ORC names are not present.

Motivation and Context

The ability for Presto to flexibly handle column names in ORC files is critical for accessing a wide range of data, including legacy datasets that may not contain embedded column names. The original hive.orc.use-column-names configuration setting only allowed access via column names and did not account for files without them. This approach helps mitigate issues with reading old ORC data and ensures that users can access their datasets consistently without manual intervention.

Impact

The changes to hive.orc.use-column-names represent an enhancement to the existing functionality rather than a breaking change. Users specifying this option will now benefit from an intelligent fallback mechanism. There should be no performance impact as the fallback only occurs when required, and there are no changes to the public API.

Test Plan

The changes have been tested using both new and old ORC files—both those with embedded column names and those without. The Presto cluster correctly accessed column data using the appropriate naming strategy in each case.

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 Changes
* Improves the `hive.orc.use-column-names` configuration setting to no longer fail on reading ORC files 
  without column names but falls back to using Hive's schema, enhancing compatibility with legacy ORC files.

jaystarshot · 2023-11-16T02:23:40Z

Is it possible to add unit testing? I can see some related tests at testHiveFileFormats

ico01 · 2023-11-16T03:01:50Z

Is it possible to add unit testing? I can see some related tests at testHiveFileFormats

ok, i just did't know where to test

ico01 · 2023-11-16T09:38:09Z

@jaystarshot i add unit test for it

jaystarshot

lgtm, deferring to a orc expert for any concerns

tdcmeehan · 2023-11-16T18:02:44Z

CC: @sdruzkin

ico01 · 2023-11-30T04:44:38Z

@tdcmeehan @sdruzkin @mbasmanova It's has been two weeks since last action, what should i do now

tdcmeehan

Some nits. Overall, after reading through the ORC spec, this change seems fine. Pinging @sdruzkin one last time as our ORC expert, otherwise I'm happy to merge once the nits are addressed.

tdcmeehan · 2023-11-30T17:14:02Z

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

@@ -1136,7 +1135,11 @@ public static List<HiveColumnHandle> getPhysicalHiveColumnHandles(List<HiveColum
        }

        List<String> columnNames = getColumnNames(types);
-        verifyFileHasColumnNames(columnNames, path);
+
+        boolean hasColumnNames = isFileHasColumnNames(columnNames, path);


Suggested change

boolean hasColumnNames = isFileHasColumnNames(columnNames, path);

boolean hasColumnNames = fileHasColumnNames(columnNames, path);

tdcmeehan · 2023-11-30T17:14:15Z

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

@@ -1166,13 +1169,13 @@ private static List<String> getColumnNames(List<OrcType> types)
        return types.get(0).getFieldNames();
    }

-    private static void verifyFileHasColumnNames(List<String> physicalColumnNames, Path path)
+    private static boolean isFileHasColumnNames(List<String> physicalColumnNames, Path path)


Suggested change

private static boolean isFileHasColumnNames(List<String> physicalColumnNames, Path path)

private static boolean fileHasColumnNames(List<String> physicalColumnNames, Path path)

Path is no longer used and can be removed.

Also interesting that the method name suggests that we check the file footer to get the column names, but this method does not read the file. Does this method just checks whether the columns names in the metastore have Hive style?

tdcmeehan · 2023-11-30T17:16:05Z

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

+        boolean hasColumnNames = true;
        if (!physicalColumnNames.isEmpty() && physicalColumnNames.stream().allMatch(physicalColumnName -> DEFAULT_HIVE_COLUMN_NAME_PATTERN.matcher(physicalColumnName).matches())) {
-            throw new PrestoException(
-                    HIVE_FILE_MISSING_COLUMN_NAMES,
-                    "ORC file does not contain column names in the footer: " + path);
+            hasColumnNames = false;
        }
+        return hasColumnNames;


return physicalColumnNames.isEmpty || !physicalColumnNames.stream().allMatch(physicalColumnName -> DEFAULT_HIVE_COLUMN_NAME_PATTERN.matcher(physicalColumnName).matches())

tdcmeehan · 2023-11-30T17:18:36Z

Also, please follow our guidelines for commits. Namely:

Please squash your commits
Please make sure you read through our guidelines on commit messages (although in the contributor checklist this box was checked, it does not follow our guidelines). Perhaps rename the commit message to Allow reading of legacy ORC files with default column names

sdruzkin · 2023-11-30T18:44:52Z

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

@@ -1166,13 +1169,13 @@ private static List<String> getColumnNames(List<OrcType> types)
        return types.get(0).getFieldNames();
    }

-    private static void verifyFileHasColumnNames(List<String> physicalColumnNames, Path path)
+    private static boolean isFileHasColumnNames(List<String> physicalColumnNames, Path path)


Path is no longer used and can be removed.

Also interesting that the method name suggests that we check the file footer to get the column names, but this method does not read the file. Does this method just checks whether the columns names in the metastore have Hive style?

sdruzkin · 2023-11-30T18:53:58Z

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

+    public void testOrcUseColumnNamesCompatibility(int rowCount)
+            throws Exception
+    {
+        // test hive.orc.use-column-names can fallback to use hive column names, if in orc file has no real column name


has no real column name -> has no real column names

sdruzkin · 2023-11-30T18:54:23Z

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

+            throws Exception
+    {
+        // test hive.orc.use-column-names can fallback to use hive column names, if in orc file has no real column name
+        // only have old hive style name _col1, _clo2, _clo3


_clo2, _clo3 -> _col2, _col3

sdruzkin · 2023-11-30T18:54:50Z

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

+
+    private static List<TestColumn> getHiveColumnNameColumns()
+    {
+        //Creates a new list of TestColumn objects with Hive-style column names based on their indices.


add a space after //

ico01 · 2023-12-06T03:53:08Z

thanks,I will fix these

linux-foundation-easycla · 2024-03-06T14:28:52Z

The committers listed above are authorized under a signed CLA.

✅ login: ico01 (f774aeb)

steveburnett · 2024-03-06T15:16:02Z

Suggest revising release note entry following the Order of changes in the release notes guidelines. Perhaps something like this:

== RELEASE NOTES ==

Hive Changes
* Improves the `hive.orc.use-column-names` configuration setting to no longer fail on reading ORC files 
  without column names but falls back to using Hive's schema, enhancing compatibility with legacy ORC files.

ico01 · 2024-03-09T08:43:00Z

i found it's very difficult to run all check passed, each time it will have different test not passed. even though i didn't change code

tdcmeehan · 2024-03-09T16:16:55Z

@ico01 the CI environment has become unstable lately. We are working on making it more stable again. In the meantime, maintainers will ensure tests pass and will merge.

ico01 · 2024-03-10T02:42:17Z

ok

ico01 requested a review from a team as a code owner November 15, 2023 10:00

ico01 requested a review from presto-oss November 15, 2023 10:00

ico01 changed the title ~~add compation for old data~~ enhancing compatibility with legacy ORC files Nov 15, 2023

jaystarshot previously approved these changes Nov 16, 2023

View reviewed changes

jaystarshot requested review from tdcmeehan and mbasmanova November 16, 2023 12:31

tdcmeehan requested a review from sdruzkin November 16, 2023 18:02

tdcmeehan requested changes Nov 30, 2023

View reviewed changes

sdruzkin reviewed Nov 30, 2023

View reviewed changes

ico01 dismissed jaystarshot’s stale review via 7c6e820 March 6, 2024 14:28

tdcmeehan self-assigned this Mar 6, 2024

ico01 force-pushed the fix_orc_reader_4 branch 3 times, most recently from 9358465 to a617dbf Compare March 6, 2024 15:05

ico01 force-pushed the fix_orc_reader_4 branch 7 times, most recently from feb6a24 to 43bc00d Compare March 8, 2024 07:37

ico01 force-pushed the fix_orc_reader_4 branch from 43bc00d to c30cb7c Compare March 8, 2024 12:22

tdcmeehan approved these changes Mar 8, 2024

View reviewed changes

ico01 force-pushed the fix_orc_reader_4 branch from c30cb7c to 3720bac Compare March 9, 2024 06:42

Allow reading of legacy ORC files with default column names

f774aeb

tdcmeehan force-pushed the fix_orc_reader_4 branch from 3720bac to f774aeb Compare April 16, 2024 13:49

tdcmeehan merged commit 63aa9f7 into prestodb:master Apr 16, 2024
56 checks passed

wanglinsong mentioned this pull request May 1, 2024

Add release notes for 0.287 #22647

Merged

48 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhancing compatibility with legacy ORC files #21391

enhancing compatibility with legacy ORC files #21391

ico01 commented Nov 15, 2023 •

edited

Loading

jaystarshot commented Nov 16, 2023

ico01 commented Nov 16, 2023

ico01 commented Nov 16, 2023

jaystarshot left a comment

tdcmeehan commented Nov 16, 2023

ico01 commented Nov 30, 2023

tdcmeehan left a comment

tdcmeehan Nov 30, 2023

ico01 Mar 7, 2024

tdcmeehan Nov 30, 2023

sdruzkin Nov 30, 2023

ico01 Mar 7, 2024

tdcmeehan Nov 30, 2023

ico01 Mar 7, 2024

tdcmeehan commented Nov 30, 2023 •

edited

Loading

sdruzkin Nov 30, 2023

sdruzkin Nov 30, 2023

ico01 Mar 7, 2024

sdruzkin Nov 30, 2023

ico01 Mar 7, 2024

sdruzkin Nov 30, 2023

ico01 Mar 7, 2024

ico01 commented Dec 6, 2023

linux-foundation-easycla bot commented Mar 6, 2024 •

edited

Loading

steveburnett commented Mar 6, 2024

ico01 commented Mar 9, 2024

tdcmeehan commented Mar 9, 2024

ico01 commented Mar 10, 2024

	boolean hasColumnNames = isFileHasColumnNames(columnNames, path);
	boolean hasColumnNames = fileHasColumnNames(columnNames, path);

	private static boolean isFileHasColumnNames(List<String> physicalColumnNames, Path path)
	private static boolean fileHasColumnNames(List<String> physicalColumnNames, Path path)

enhancing compatibility with legacy ORC files #21391

enhancing compatibility with legacy ORC files #21391

Conversation

ico01 commented Nov 15, 2023 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

jaystarshot commented Nov 16, 2023

ico01 commented Nov 16, 2023

ico01 commented Nov 16, 2023

jaystarshot left a comment

Choose a reason for hiding this comment

tdcmeehan commented Nov 16, 2023

ico01 commented Nov 30, 2023

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

tdcmeehan commented Nov 30, 2023 • 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

ico01 commented Dec 6, 2023

linux-foundation-easycla bot commented Mar 6, 2024 • edited Loading

steveburnett commented Mar 6, 2024

ico01 commented Mar 9, 2024

tdcmeehan commented Mar 9, 2024

ico01 commented Mar 10, 2024

ico01 commented Nov 15, 2023 •

edited

Loading

tdcmeehan commented Nov 30, 2023 •

edited

Loading

linux-foundation-easycla bot commented Mar 6, 2024 •

edited

Loading