Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HIVE-27484: Limit pushdown with offset generate wrong results #4471

Merged
merged 3 commits into from
Jul 21, 2023

Conversation

okumin
Copy link
Contributor

@okumin okumin commented Jul 6, 2023

What changes were proposed in this pull request?

Fix an incorrect push-down of LIMIT + OFFSET.
https://issues.apache.org/jira/browse/HIVE-27484

I know SemanticAnalyzer also doesn't handle LIMIT + OFFSET correctly when LIMIT + OFFSET is pushed down to parallel tasks. I will be working on it in HIVE-27480 after HIVE-27484.

Why are the changes needed?

To prevent wrong results.

Does this PR introduce any user-facing change?

Bug fix.

Is the change a dependency upgrade?

No

How was this patch tested?

Unit tests

@@ -1339,6 +1341,7 @@ limit 1 offset 1
POSTHOOK: type: QUERY
POSTHOOK: Input: default@src
#### A masked pattern was here ####
86 val_86 86 val_86
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result was originally empty because it pushes LIMIT 1 OFFSET 1 twice.

@okumin okumin changed the title [WIP] HIVE-27484: Limit pushdown with offset generate wrong results HIVE-27484: Limit pushdown with offset generate wrong results Jul 7, 2023
Copy link

@aturoczy aturoczy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and this is an important fix for Hive 4.0! Nice job!

@aturoczy
Copy link

aturoczy commented Jul 7, 2023

@zabetak @kasakrisz Could you please check this change?

@okumin
Copy link
Contributor Author

okumin commented Jul 13, 2023

@zabetak @kasakrisz Could you please take a look when you have a chance? Thanks in advance!

Copy link
Contributor

@kasakrisz kasakrisz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left minor comments

Copy link
Contributor

@kasakrisz kasakrisz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 pending tests

@sonarcloud
Copy link

sonarcloud bot commented Jul 20, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug B 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@kasakrisz kasakrisz merged commit 9e4a0e7 into apache:master Jul 21, 2023
@okumin okumin deleted the HIVE-27484-limit-pushdown branch July 21, 2023 09:06
@okumin
Copy link
Contributor Author

okumin commented Jul 21, 2023

Thanks for your reviews 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants