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.

Sign up for GitHub

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

Jump to bottom

Catch exception at joinCompiler lookup source #12117

Closed
wants to merge 1 commit into from
Closed

Catch exception at joinCompiler lookup source #12117

wants to merge 1 commit into from

Conversation

miniway
Copy link
Contributor

@miniway miniway commented Apr 25, 2022

Description

This could fix when a join query failed to compile byte codes at Join query and use interpreter at the compilation failure by various reasons ex, too many involved columns.

We got the following error when hundreds of columns are used at the output columns in a JOIN query in presto 350.

com.google.common.util.concurrent.UncheckedExecutionException: io.airlift.bytecode.CompilationException: Error compiling class: io/prestosql/$gen/PagesHashStrategy_20220421_050326_462152
    at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2051)
    at com.google.common.cache.LocalCache.get(LocalCache.java:3951)
    at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:3974)
    at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:4958)
    at com.google.common.cache.LocalCache$LocalLoadingCache.getUnchecked(LocalCache.java:4964)
    at io.prestosql.sql.gen.JoinCompiler.compileLookupSourceFactory(JoinCompiler.java:131)
    at io.prestosql.operator.PagesIndex.createLookupSourceSupplier(PagesIndex.java:504)
    at io.prestosql.operator.HashBuilderOperator.buildLookupSource(HashBuilderOperator.java:589)
    at io.prestosql.operator.HashBuilderOperator.finishInput(HashBuilderOperator.java:486)
    at io.prestosql.operator.HashBuilderOperator.finish(HashBuilderOperator.java:442)
    at io.prestosql.operator.Driver.processInternal(Driver.java:397)
    at io.prestosql.operator.Driver.lambda$processFor$8(Driver.java:283)
    at io.prestosql.operator.Driver.tryWithLock(Driver.java:675)
    at io.prestosql.operator.Driver.processFor(Driver.java:276)
    at io.prestosql.execution.SqlTaskExecution$DriverSplitRunner.processFor(SqlTaskExecution.java:1076)
    at io.prestosql.execution.executor.PrioritizedSplitRunner.process(PrioritizedSplitRunner.java:163)
    at io.prestosql.execution.executor.TaskExecutor$TaskRunner.run(TaskExecutor.java:484)
    at io.prestosql.$gen.Presto_350____20220418_042654_2.run(Unknown Source)
    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
    at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: io.airlift.bytecode.CompilationException: Error compiling class: io/prestosql/$gen/PagesHashStrategy_20220421_050326_462152
    at io.airlift.bytecode.ClassGenerator.defineClasses(ClassGenerator.java:143)
    at io.airlift.bytecode.ClassGenerator.defineClass(ClassGenerator.java:117)
    at io.prestosql.util.CompilerUtils.defineClass(CompilerUtils.java:63)
    at io.prestosql.util.CompilerUtils.defineClass(CompilerUtils.java:57)
    at io.prestosql.sql.gen.JoinCompiler.internalCompileHashStrategy(JoinCompiler.java:234)
    at io.prestosql.sql.gen.JoinCompiler.internalCompileLookupSourceFactory(JoinCompiler.java:165)
    at io.prestosql.sql.gen.JoinCompiler.lambda$new$0(JoinCompiler.java:101)
    at com.google.common.cache.CacheLoader$FunctionToCacheLoader.load(CacheLoader.java:165)
    at com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3529)
    at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2278)
    at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2155)
    at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2045)
    ... 20 more
Caused by: org.objectweb.asm.MethodTooLargeException: Method too large: io/prestosql/$gen/PagesHashStrategy_20220421_050326_462152.<init> (Ljava/util/List;Ljava/util/OptionalInt;)V
    at org.objectweb.asm.MethodWriter.computeMethodInfoSize(MethodWriter.java:2080)
    at org.objectweb.asm.ClassWriter.toByteArray(ClassWriter.java:459)
    at io.airlift.bytecode.ClassGenerator.defineClasses(ClassGenerator.java:140)

PagesIndex.createPagesHashStrategy catches compilation error at joinCompiler.compilePagesHashStrategyFactory and uses interpreter's one but not for joinCompiler.compileLookupSourceFactory

Related issues, pull requests, and links

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Apr 25, 2022
@findepi findepi requested review from electrum and sopel39 April 25, 2022 11:13
sopel39
sopel39 reviewed Apr 26, 2022
View reviewed changes
core/trino-main/src/main/java/io/trino/operator/PagesIndex.java
}
catch (Exception e) {
log.error(e, "Lookup source compile failed for types=%s error=%s", types, e);
}
}

PagesHashStrategy hashStrategy = new SimplePagesHashStrategy(
Copy link
Member

Choose a reason for hiding this comment

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

We should remove fallback instead (see: 1232d39). Why do you want to restore it? Is there a compilation failure? In case compilation failed user will get a perf hit without even knowing that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By this

trino/core/trino-main/src/main/java/io/trino/operator/PagesIndex.java

Line 451 in 6a192cf

try {
I thought slow performance might be allowed than a failure.

A session parameter to allow fallback would be nice so that user can decide.

Copy link
Member

Choose a reason for hiding this comment

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

That is a different code path for PagesHashStrategy. We should remove it too (need to check before if we compile all cases).

A session parameter to allow fallback would be nice so that user can decide.

I'm not sure. It's really technical, so unless we see actual issues around that, I would not add a parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, if I understand correctly, Trino would not allow queries causing compilation failure so that users could notice it and rewrite their failed queries.

Copy link
Member

Choose a reason for hiding this comment

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

Compilation failures are not for users to fix really.

@colebow
Copy link
Member

colebow commented Mar 30, 2023

👋 @miniway - this PR has become inactive. If you're still interested in working on it, please let us know, and we can try to get reviewers to help with that.

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.

@weiatwork
Copy link
Contributor

+1 Slowness is better than query failure

@sopel39
Copy link
Member

sopel39 commented Apr 28, 2023

@weiatwork did you see any failures due to missing fallback?

@weiatwork
Copy link
Contributor

Yes, similar to OP, when there are a lot of columns in the query. With the fallback it can succeed

@colebow
Copy link
Member

colebow commented May 2, 2023

I think it sounds like we may be interested in this PR, but because the author hasn't chimed in for a year, I'm going to go ahead and close this out. Feel free to re-open at any point, and if someone else wants to pick up the work, go for it.

@colebow colebow closed this May 2, 2023
@miniway
Copy link
Contributor Author

miniway commented May 3, 2023

Thanks, I'll reopen it or create another PR with an enhancement. Personally a session property to allow it might be better than catching all compiler-errors.

@takezoe takezoe mentioned this pull request Oct 6, 2024
Closed
@takezoe
Copy link
Member

takezoe commented Oct 9, 2024

#23720 would solve (or mitigate) this issue.

@takezoe takezoe mentioned this pull request Nov 13, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sopel39 sopel39 sopel39 left review comments

@electrum electrum Awaiting requested review from electrum

Assignees
No one assigned
Labels
cla-signed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@miniway @colebow @weiatwork @sopel39 @takezoe