featzhang commented on code in PR #28681:
URL: https://github.com/apache/flink/pull/28681#discussion_r3541689138
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/plan/utils/RelTreeWriterImpl.scala:
##########
@@ -23,14 +23,17 @@ import
org.apache.flink.table.planner.plan.metadata.FlinkRelMetadataQuery
import org.apache.flink.table.planner.plan.nodes.physical.FlinkPhysicalRel
import org.apache.flink.table.planner.plan.nodes.physical.stream._
+import org.apache.calcite.plan.RelOptUtil
Review Comment:
sanity check: containsSubQuery walks every RexNode we print, so for plans
with many predicates this adds a visitor pass per item. probably fine given
explain is not hot, but curious if you measured any explain-time regression in
the planner tests.
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/plan/utils/RelTreeWriterImpl.scala:
##########
@@ -279,6 +240,122 @@ class RelTreeWriterImpl(
pw.println()
}
+ override def item(term: String, value: AnyRef): RelWriter = {
+ if (withQueryHint || withQueryBlockAlias) {
+ value match {
+ case rexNode: RexNode if containsSubQuery(rexNode) =>
+ return super.item(term, toHintAwareSubQueryString(rexNode))
+ case _ =>
+ }
+ }
+ super.item(term, value)
+ }
+
Review Comment:
the string-replace trick here worries me a bit: you build result from
node.toString, then look for RelOptUtil.toString(subQuery.rel) inside it via
indexOf. if the two toString calls ever render the same sub-rel slightly
differently (they can, e.g. with/without id prefix or trait diffs), start stays
-1 and the hint silently disappears from that sub-query. did you check a case
with a nested sub-query (sub-query inside a sub-query)? might be safer to
render each sub-query rel directly into the spot instead of matching on a
substring.
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/plan/utils/RelTreeWriterImpl.scala:
##########
@@ -279,6 +240,122 @@ class RelTreeWriterImpl(
pw.println()
}
+ override def item(term: String, value: AnyRef): RelWriter = {
+ if (withQueryHint || withQueryBlockAlias) {
+ value match {
+ case rexNode: RexNode if containsSubQuery(rexNode) =>
+ return super.item(term, toHintAwareSubQueryString(rexNode))
+ case _ =>
+ }
+ }
+ super.item(term, value)
+ }
+
+ private def containsSubQuery(node: RexNode): Boolean = {
+ var found = false
+ node.accept(new RexVisitorImpl[Void](true) {
+ override def visitSubQuery(subQuery: RexSubQuery): Void = {
+ found = true
+ null
+ }
+ })
+ found
+ }
+
+ private def addHintItems(rel: RelNode, addItem: (String, AnyRef) => Unit):
Unit = {
+ addQueryHintItems(rel, addItem)
+ addQueryBlockAliasHintItems(rel, addItem)
+ }
+
+ private def addQueryHintItems(rel: RelNode, addItem: (String, AnyRef) =>
Unit): Unit = {
+ if (withQueryHint) {
+ rel match {
+ case hintable: Hintable if rel.isInstanceOf[Join] ||
rel.isInstanceOf[Correlate] =>
+ val hints = hintable.getHints
+ addJoinHintItems(hints, addItem)
+ addStateTtlHintItems(hints, addItem)
+ case aggregate: Aggregate =>
+ addStateTtlHintItems(aggregate.getHints, addItem)
+ case aggregate: StreamPhysicalGroupAggregateBase =>
Review Comment:
here done calls back into item, and item can re-enter
toHintAwareSubQueryString for any RexNode value. for a normal rel node the
printed values will not be RexNodes, so no infinite loop in practice - but it
is a subtle coupling between item and this writer. a short comment on why done
delegates to item (rather than printValues.add directly) would help the next
reader.
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/plan/hints/batch/JoinHintTestBase.java:
##########
@@ -51,9 +51,6 @@
* A test base for join hint.
*
* <p>TODO add test to cover legacy table source.
- *
Review Comment:
nice that you cover sibling sub-queries with different hints. would be good
to also assert the optimized plan (not just ast) prints the hint on the
sub-query join, since the bug was about explain output and the optimized rel is
what users actually see in EXPLAIN. the xml already has the optimized plan
captured, so the assertion is mostly there.
--
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]
