Jackie-Jiang commented on code in PR #19003:
URL: https://github.com/apache/pinot/pull/19003#discussion_r3632882372
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotImplicitTableHintRule.java:
##########
@@ -57,8 +57,11 @@ public static PinotImplicitTableHintRule
withWorkerManager(WorkerManager workerM
public boolean matches(RelOptRuleCall call) {
TableScan tableScan = call.rel(0);
- // we don't want to apply this rule if the explicit hint is complete
- return !isHintComplete(getTableOptionHint(tableScan));
+ // we don't want to apply this rule if the explicit hint is complete, or
if the table is hinted as replicated
+ // across all workers (each worker scans all the segments, so partition
options are irrelevant)
+ @Nullable
Review Comment:
(nit) Not introduced new, but we don't usually put `@Nullable` here
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotImplicitTableHintRule.java:
##########
@@ -57,8 +57,11 @@ public static PinotImplicitTableHintRule
withWorkerManager(WorkerManager workerM
public boolean matches(RelOptRuleCall call) {
TableScan tableScan = call.rel(0);
- // we don't want to apply this rule if the explicit hint is complete
- return !isHintComplete(getTableOptionHint(tableScan));
+ // we don't want to apply this rule if the explicit hint is complete, or
if the table is hinted as replicated
+ // across all workers (each worker scans all the segments, so partition
options are irrelevant)
+ @Nullable
+ RelHint explicitHint = getTableOptionHint(tableScan);
Review Comment:
We should return non-null only if rewrite is required, and the new hint is
valid
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotImplicitTableHintRule.java:
##########
@@ -57,8 +57,11 @@ public static PinotImplicitTableHintRule
withWorkerManager(WorkerManager workerM
public boolean matches(RelOptRuleCall call) {
TableScan tableScan = call.rel(0);
- // we don't want to apply this rule if the explicit hint is complete
- return !isHintComplete(getTableOptionHint(tableScan));
+ // we don't want to apply this rule if the explicit hint is complete, or
if the table is hinted as replicated
+ // across all workers (each worker scans all the segments, so partition
options are irrelevant)
+ @Nullable
+ RelHint explicitHint = getTableOptionHint(tableScan);
Review Comment:
I feel it will be cleaner if we perform the validation within
`getTableOptionHint()` and return `null` when the hint is not valid. It is more
self contained that way, and we don't need to repeat the `null` checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
