-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Fix 'Multiple entries with same key' error with dynamic filters #20917
Fix 'Multiple entries with same key' error with dynamic filters #20917
Conversation
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Li Xiangqun.
|
caa3ed7
to
223d557
Compare
@@ -440,19 +441,22 @@ private TupleDomain<ColumnHandle> translateSummaryToTupleDomain( | |||
{ | |||
Collection<DynamicFilters.Descriptor> descriptors = descriptorMultimap.get(filterId); | |||
return TupleDomain.withColumnDomains(descriptors.stream() | |||
.collect(toImmutableMap( | |||
.collect(Collectors.groupingBy( | |||
descriptor -> { | |||
Symbol probeSymbol = Symbol.from(descriptor.getInput()); | |||
return requireNonNull(columnHandles.get(probeSymbol), () -> format("Missing probe column for %s", probeSymbol)); | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test in BaseDynamicPartitionPruningTest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted will do.
core/trino-main/src/main/java/io/trino/server/DynamicFilterService.java
Outdated
Show resolved
Hide resolved
lgtm % @raunaqmorarka comments.
that query really simplifies to:
and inequality join is pretty inefficient. Is there a better example where it fails? |
@@ -71,6 +71,7 @@ | |||
import java.util.concurrent.atomic.AtomicLong; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: commit message too long
This is just a simplified query to reproduce the issue. |
223d557
to
ccb0192
Compare
ccb0192
to
9492455
Compare
I understand. However, the issue that this PR fixes is rather basic, but wasn't discovered before. Hence I wonder what practical query could trigger it, because queries like @lxqfy What kind of production query did hit the issue? |
The actual query spans 200 lines and is overly complex. Generally, The query owner creates a CTE |
@lxqfy but does that prod query simplify to The reason I'm asking is maybe we should have a rule that transforms |
Description
Trino will throw Exceptions "Multiple entries with same key", when the same dynamic filter applies to the same partitions key column multiple times under different conditions. E.g.
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text: