-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Join context hints #17541
base: master
Are you sure you want to change the base?
Join context hints #17541
Conversation
...i-stage-query/src/test/quidem/org.apache.druid.msq.quidem.MSQQuidemTest/msqNestedJoinHint.iq
Outdated
Show resolved
Hide resolved
...i-stage-query/src/test/quidem/org.apache.druid.msq.quidem.MSQQuidemTest/msqNestedJoinHint.iq
Outdated
Show resolved
Hide resolved
private static HintStrategyTable createHintStrategies() | ||
{ | ||
return HintStrategyTable.builder() | ||
.hintStrategy("broadcast", HintPredicates.JOIN) |
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: is it really needed to burn in the "broadcast"
literal here?
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.
note: I think this class should be moved in the DruidHints
or something class you rename/restructure the JoinHint
class to
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.
Changed to the enum name instead.
if (closestHint == null || hint.inheritPath.size() < closestHint.inheritPath.size()) { | ||
closestHint = hint; | ||
} |
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.
this assumes that all hints are joinhints - which may not be true in the future
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.
Added a check here
|
||
import java.util.Arrays; | ||
|
||
public enum JoinHint |
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.
I think it would be better to not have this as an enum - and you could probably use as(JoinAlgorithm.class)
or asJoinAlgorithm()
to obtain the current value
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.
There's an issue with that, Calcite only seems to accept certain keywords as hints, and this is sort_merge
. Trying to replace the hint name with sortMerge
seems to make it not read by calcite.
@JsonCreator | ||
public static JoinHint fromString(final String id) | ||
{ |
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.
I don't think this should be serializable by jackson; if it gets on the wire it should be a JoinAlgorithm
already
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.
I think it might have been to deserializer it for the planner context at some point
sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java
Outdated
Show resolved
Hide resolved
processing/src/main/java/org/apache/druid/query/JoinDataSource.java
Outdated
Show resolved
Hide resolved
processing/src/main/java/org/apache/druid/query/planning/PreJoinableClause.java
Outdated
Show resolved
Hide resolved
@@ -15585,7 +15587,8 @@ public void testOrderByAlongWithInternalScanQueryNoDistinct() | |||
JoinType.INNER, | |||
null, | |||
ExprMacroTable.nil(), | |||
CalciteTests.createJoinableFactoryWrapper() | |||
CalciteTests.createJoinableFactoryWrapper(), | |||
JoinAlgorithm.BROADCAST |
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.
I don't think there is meaning of this in this context as this engine doesn't support joinalgorithms ; so this change should be absent here or null
?
maybe use JsonInclude.Include.NON_NULL
and make the class behave like null
means BROADCAST
?
Rebased and addressed some issues of #17406
Currently, using the query context, all joins in a query can be hinted to either broadcast or sort merge join. However, this does not allow this to be set at a join level.
Add support for hints to queries, which can dictate the join type.
Syntax
Note
Calcite does not expose if the hint is at a particular join level, and allows inheritance of hints from parent nodes. This causes join hints to to affect inner queries. Additional join hints can be added to inner queries as a workaround.
This PR has: