Skip to content

HIVE-28549: Limit the maximum number of operators merged by SharedWorkOptimizer #5492

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

Merged
merged 7 commits into from
Apr 1, 2025

Conversation

okumin
Copy link
Contributor

@okumin okumin commented Oct 8, 2024

What changes were proposed in this pull request?

This PR would limit the maximum number of table scan operators which SWO tries to merge.

https://issues.apache.org/jira/browse/HIVE-28549

Why are the changes needed?

We observed SWO makes a negative impact when it merges too many, e.g. 50, operators. If operators are memory intensive, they might throw OOM or might slow down.

I believe we can resolve OOM with the following patch, but we still want an upper limit so that we can tune concurrency or RAM per operator reasonably.
#5478

Does this PR introduce any user-facing change?

No.

Is the change a dependency upgrade?

No.

How was this patch tested?

I added a qtest

Copy link

sonarqubecloud bot commented Oct 8, 2024

@okumin okumin changed the title [WIP] HIVE-28549: Limit the maximum number of operators merged by SharedWorkOptimizer HIVE-28549: Limit the maximum number of operators merged by SharedWorkOptimizer Oct 16, 2024
@okumin okumin marked this pull request as ready for review October 16, 2024 01:25
ArrayListMultimap<String, TableScanOperator> tableNameToOps, int batchSize) {
if (batchSize == -1) {
return Collections.singletonList(sortedTables.stream().map(Entry::getKey)
.flatMap(tableName -> tableNameToOps.get(tableName).stream()).collect(Collectors.toList()));
Copy link
Contributor

Choose a reason for hiding this comment

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

This puts all TS ops into a single List, regardless of their source. Maybe you intend the following code?

    if (batchSize == -1) {
      return sortedTables.stream()
          .map(entry -> tableNameToOps.get(entry.getKey()))
          .collect(Collectors.toList());
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I was just confused when I wrote the line. I modified it and ran some qtests which failed
e574161

Copy link
Contributor

@ngsg ngsg left a comment

Choose a reason for hiding this comment

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

LGTM, +1
(The failed test seems just a flaky one. In my local environment, the test finished successfully.)

@okumin
Copy link
Contributor Author

okumin commented Nov 18, 2024

Retriggered CI just in case

@okumin
Copy link
Contributor Author

okumin commented Nov 20, 2024

I am checking why partition_explain_ddl always fails. In my impression, this PR is unrelated.
P.S. I found it failed on the latest master branch

@deniskuzZ
Copy link
Member

hi @okumin, should we rebase?

@okumin
Copy link
Contributor Author

okumin commented Mar 24, 2025

@deniskuzZ
Hi, I've rebased the branch on the master branch

Copy link
Member

@deniskuzZ deniskuzZ left a comment

Choose a reason for hiding this comment

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

LGTM, minor comment

@deniskuzZ
Copy link
Member

hi @okumin, not sure if you noticed the above comment, do you think it's legit?

@okumin
Copy link
Contributor Author

okumin commented Mar 26, 2025

@deniskuzZ Yes, I do. I'm using my machine power to review and test another PR. I will likely update this one tomorrow.

Copy link

@deniskuzZ deniskuzZ merged commit 1b4defa into apache:master Apr 1, 2025
4 checks passed
@okumin okumin deleted the HIVE-28549-limit-swo branch April 1, 2025 08:53
@okumin
Copy link
Contributor Author

okumin commented Apr 1, 2025

Thanks

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