Improve error handling for partial aggregation pushdown by rschlussel · Pull Request #22011 · prestodb/presto

rschlussel · 2024-02-26T21:17:01Z

Improve error handling for partial aggregation pushdown and prevent returning wrong results when footer stats should not be relied on. This covers the following cases:

Aggregations have been pushed down but partition file format does not support aggregation pushdown (can occur if table is declared with a supported storage format, but partition has a different storage format). Previously, page source providers for some file formats had special handling for this case, but not all
Always throw an exception if aggregations have been pushed down but partition footer stats are unreliable. Previously, if filter pushdown was enabled (used OrcSelectivePageSourceFactory), we wouldn't create an AggregatedPageSource, so you would get an error somewhere on read. If it was disabled (OrcBatchPageSourceFactory), we would create an AggregatedPageSource and the query would silently give wrong results.
Unexpected state where some but not all columns are of AGGREGATED type.

Error handling is still going to be reader dependent if both the table and partition format support partial aggregation pushdown, but the partition format does not support as many types (e.g. currently supports more types for partial aggregation pushdown).

Description

Previously AggregatedPageSources (which support the execution side of partial aggregation pushdown) were created from within the selective and batch page source factories of supported file formats. Similarly error handling for any unsupported file format needed to be repeated for each PageSourceFactory of all unsupported file formats. This resulted in a fragmented implementation and some unsupported file formats that did not include proper error handling.

Additionally, partial aggregation pushdown cannot be used when footer stats are unreliable, however handling for this was only added for one of the supported file formats factories (OrcSelectivePageSourceFactory) while others (orc and parquet batch factories) could silently return wrong results. Furthermore, the handling in OrcSelectivePageSourceFactory prevented wrong results by not creating an aggregated page source but didn't produce a clear error message because it kept going by trying to create a selective page source.

This PR makes HiveAggregatedPageSourceFactories into a top-level concept similar to HiveSelectivePageSourceFactories and HiveBatchPageSourceFactories so that we can unify all the error handling and prevent bugs from creeping in as new file format page source factories are added.
The main logic of the change is in HivePageSourceProvider. A lot of the rest of it is scaffolding to support that.

Motivation and Context:

to ensure consistent error handling across different page sources even as new page formats or selective readers implementations are added.
To prevent wrong results when footer stats are unreliable regardless of file format and any other configs.

This gap was discovered as part of an audit to make sure we were not assuming that partition file formats will always match table file formats.

Impact

Fix a potential wrong results bug when footer stats are marked as unreliable and aggregation pushdown is enabled. Ensure all file formats that don't support aggregation pushdown will return a clear error to the user.

Test Plan

new unit tests for HivePageSourceProvider

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
* Fix a potential wrong results bug when footer stats are marked unreliable and partial aggregation pushdown is enabled.  Such queries will now fail with an error.

rschlussel · 2024-02-26T21:53:41Z

I'm still updating the tests. Don't review yet

rschlussel · 2024-02-27T16:21:04Z

this is ready for review (failing tests are flaky/unrelated)

abhiseksaikia

LGTM % minor nit and a question

presto-hive/src/main/java/com/facebook/presto/hive/orc/DwrfAggregatedPageSourceFactory.java

abhiseksaikia · 2024-02-29T23:00:21Z

presto-hive/src/main/java/com/facebook/presto/hive/orc/OrcAggregatedPageSourceFactory.java

Question: I noticed that some parts of the aggregated page source factory have similar logic as that of its respective non-aggregated page source factory. Does it make sense to refactor this duplicated code or is it better to leave it as is and avoid introducing more complexity/refactoring?

vivek-bharathan

Thanks for improving on the original implementation. Overall lgtm

vivek-bharathan · 2024-03-01T19:30:07Z

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

nit: this function signature and the one below

Suggested change

private static boolean shouldSkipPartition(TypeManager typeManager, HiveTableLayoutHandle hiveLayout, DateTimeZone hiveStorageTimeZone, HiveSplit hiveSplit, SplitContext

private static boolean shouldSkipPartition(TypeManager typeManager,

HiveTableLayoutHandle hiveLayout,

DateTimeZone hiveStorageTimeZone,

HiveSplit hiveSplit,

SplitContext splitContext)

vivek-bharathan · 2024-03-01T19:39:59Z

presto-hive/src/main/java/com/facebook/presto/hive/orc/OrcAggregatedPageSourceFactory.java

The test needs to drop the views at the end.

rschlussel · 2024-03-04T18:20:32Z

thanks for review @abhiseksaikia and @ClarenceThreepwood. I've addressed your comments. I also split out the commits a bit as per request from @ajaygeorge.

ajaygeorge

Consolidate error handling for ParquetPageSourceFactory a8c2a38 looks good % a nit

ajaygeorge · 2024-03-04T19:24:15Z

presto-hive/src/main/java/com/facebook/presto/hive/parquet/ParquetPageSourceFactory.java

stray space?

ajaygeorge

Remove unneeded error handling from page source factories f7fae20 looks good % some comments.

ajaygeorge · 2024-03-04T19:32:17Z

presto-hive/src/main/java/com/facebook/presto/hive/rcfile/RcFilePageSourceFactory.java

curious. where does this check move after the refactoring. I wasn't able to find it. Is it not needed any more.?

tagged you where this check is moved to. instead of adding error handling for every file format, we do it all in one place. that's why it's not needed here anymore.

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

rschlussel · 2024-03-04T20:05:17Z

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

@ajaygeorge this is where the check is moved to. If our columns are aggregated, we try to create an aggregatedPageSource by looping through all the aggregatedPageSourceFactories and returning when we get an aggregated page source (it's a weird way to do things, but it's how the selective and batch page sources work too), but if the file format doesn't support it (i.e. we finish looping through without returning), then we throw an exception.

ajaygeorge

Rest commits look good. LGTM

ajaygeorge · 2024-03-04T19:41:35Z

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

nit. arguments on separate lines for readability.

Improve error handling for partial aggregation pushdown and prevent returning wrong results when footer stats should not be relied on. This covers the following cases: 1. Aggregations have been pushed down but partition file format does not support aggregation pushdown (can occur if table is declared with a supported storage format, but partition has a different storage format). Previously, page source providers for some file formats had special handling for this case, but not all 2. Always throw an exception if aggregations have been pushed down but partition footer stats are unreliable. Previously, if filter pushdown was enabled (used OrcSelectivePageSourceFactory), we wouldn't create an AggregatedPageSource, so you would get an error somewhere on read. If it was disabled (OrcBatchPageSourceFactory), we would create an AggregatedPageSource and the query would silently give wrong results. 3. Unexpected state where some but not all columns are of AGGREGATED type. Error handling is still going to be reader dependent if both the table and partition format support partial aggregation pushdown, but the partition format does not support as many types (e.g. parquet vs. orc)

Remove error handling for aggregated columns from individual page source factories, as these errors are now handled in a consolidated place. This commit is separate from the main commit that consolidated the error handling for easier review.

create a utility method so we can share the error handling code between aggregated and batch page source factories.

ajaygeorge

LGTM

ajaygeorge

LGTM

abhiseksaikia

LGTM!

vivek-bharathan

lgtm

sdruzkin · 2024-03-08T18:11:10Z

presto-hive/src/main/java/com/facebook/presto/hive/orc/OrcAggregatedPageSourceFactory.java

+            DwrfEncryptionProvider dwrfEncryptionProvider,
+            boolean appendRowNumberEnabled)
+    {
+        OrcDataSource orcDataSource = getOrcDataSource(session, fileSplit, hdfsEnvironment, configuration, hiveFileContext, stats);


@rschlussel this is resource leak because we don't close the orcDataSource in a happy case

oh good catch. Thank you!

rschlussel requested a review from a team as a code owner February 26, 2024 21:17

rschlussel requested a review from presto-oss February 26, 2024 21:17

rschlussel force-pushed the aggregation-pushdown-error-handling branch 3 times, most recently from 4ba2d25 to 01bfe06 Compare February 27, 2024 15:14

rschlussel requested review from abhiseksaikia, ajaygeorge and vivek-bharathan February 27, 2024 16:21

abhiseksaikia previously approved these changes Feb 29, 2024

View reviewed changes

vivek-bharathan previously approved these changes Mar 1, 2024

View reviewed changes

rschlussel added 2 commits March 4, 2024 12:17

Fix default invoker view test

2fae585

The test needs to drop the views at the end.

Refactor OrcPageSourceFactories to share code

4c07688

rschlussel dismissed stale reviews from vivek-bharathan and abhiseksaikia via a8c2a38 March 4, 2024 17:52

rschlussel force-pushed the aggregation-pushdown-error-handling branch from 01bfe06 to a8c2a38 Compare March 4, 2024 17:52

rschlussel requested review from abhiseksaikia and vivek-bharathan March 4, 2024 18:18

ajaygeorge reviewed Mar 4, 2024

View reviewed changes

rschlussel commented Mar 4, 2024

View reviewed changes

ajaygeorge previously approved these changes Mar 4, 2024

View reviewed changes

rschlussel added 3 commits March 5, 2024 09:35

Consolidate error handling for ParquetPageSourceFactory

ed7bb4b

create a utility method so we can share the error handling code between aggregated and batch page source factories.

rschlussel dismissed ajaygeorge’s stale review via ed7bb4b March 5, 2024 14:35

rschlussel force-pushed the aggregation-pushdown-error-handling branch from a8c2a38 to ed7bb4b Compare March 5, 2024 14:35

ajaygeorge approved these changes Mar 6, 2024

View reviewed changes

abhiseksaikia approved these changes Mar 6, 2024

View reviewed changes

vivek-bharathan approved these changes Mar 6, 2024

View reviewed changes

rschlussel merged commit d80e49a into prestodb:master Mar 6, 2024

rschlussel mentioned this pull request Mar 7, 2024

Fix triggering of RcFilePageSourceFactory logic for non-RC tables #22066

Closed

6 tasks

sdruzkin reviewed Mar 8, 2024

View reviewed changes

rschlussel mentioned this pull request Mar 8, 2024

Fix bugs related to partial aggregation pushdown refactoring #22131

Merged

6 tasks

wanglinsong mentioned this pull request May 1, 2024

Add release notes for 0.287 #22647

Merged

48 tasks

-    private static boolean shouldSkipPartition(TypeManager typeManager, HiveTableLayoutHandle hiveLayout, DateTimeZone hiveStorageTimeZone, HiveSplit hiveSplit, SplitContext
+    private static boolean shouldSkipPartition(TypeManager typeManager,
+    HiveTableLayoutHandle hiveLayout,
+    DateTimeZone hiveStorageTimeZone,
+    HiveSplit hiveSplit,
+    SplitContext splitContext)

Conversation

rschlussel commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context:

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

rschlussel commented Feb 26, 2024

Uh oh!

rschlussel commented Feb 27, 2024

Uh oh!

abhiseksaikia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vivek-bharathan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rschlussel commented Mar 4, 2024

Uh oh!

ajaygeorge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajaygeorge left a comment

Choose a reason for hiding this comment

Uh oh!

ajaygeorge Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajaygeorge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajaygeorge left a comment

Choose a reason for hiding this comment

Uh oh!

ajaygeorge left a comment

Choose a reason for hiding this comment

Uh oh!

abhiseksaikia left a comment

Choose a reason for hiding this comment

Uh oh!

vivek-bharathan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rschlussel commented Feb 26, 2024 •

edited

Loading

ajaygeorge Mar 4, 2024 •

edited

Loading