-
Notifications
You must be signed in to change notification settings - Fork 346
chore: Codegen fallback explain for plan #4891
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
Changes from all commits
b344dea
f45e486
e0889c5
125e07b
18121eb
1dfc5df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,6 +131,72 @@ class CometCodegenSuite | |
| } | ||
| } | ||
|
|
||
| test("explainCodegen.enabled surfaces routed expressions in COMET-INFO") { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One of the reasons cited for making this configurable was to avoid the annotation bleeding into golden files. Would it be worth adding a companion assertion in this test that flips Separately, the four new fallback-reason prefixes in |
||
| // With the opt-in flag on, `hypot` and `levenshtein` (both `CometCodegenDispatch`) roll up | ||
| // into one `[COMET-INFO: JVM codegen dispatcher: hypot, levenshtein]` line on the | ||
| // `CometProject`. With the flag off (default), no such line appears. | ||
| withTable("t") { | ||
| sql("CREATE TABLE t (a DOUBLE, b DOUBLE, s1 STRING, s2 STRING) USING parquet") | ||
| sql("INSERT INTO t VALUES (3.0, 4.0, 'kitten', 'sitting')") | ||
|
|
||
| withSQLConf( | ||
| CometConf.COMET_SCALA_UDF_CODEGEN_ENABLED.key -> "true", | ||
| CometConf.COMET_EXPLAIN_CODEGEN_ENABLED.key -> "true", | ||
| CometConf.COMET_EXEC_PROJECT_ENABLED.key -> "true", | ||
| CometConf.COMET_EXTENDED_EXPLAIN_FORMAT.key -> | ||
| CometConf.COMET_EXTENDED_EXPLAIN_FORMAT_VERBOSE) { | ||
| val df = sql("SELECT hypot(a, b), levenshtein(s1, s2) FROM t") | ||
| checkSparkAnswerAndOperator(df) | ||
| val explain = | ||
| new ExtendedExplainInfo().generateExtendedInfo(df.queryExecution.executedPlan) | ||
| assert( | ||
| explain.contains("[COMET-INFO:"), | ||
| s"expected a [COMET-INFO: segment, got:\n$explain") | ||
| // Names appear alphabetically via `.distinct.sorted` in rollUpInfoMessages. | ||
| assert( | ||
| explain.contains("JVM codegen dispatcher: hypot, levenshtein"), | ||
| s"expected combined codegen-dispatch info, got:\n$explain") | ||
| } | ||
|
|
||
| withSQLConf( | ||
| CometConf.COMET_SCALA_UDF_CODEGEN_ENABLED.key -> "true", | ||
| CometConf.COMET_EXPLAIN_CODEGEN_ENABLED.key -> "false", | ||
| CometConf.COMET_EXEC_PROJECT_ENABLED.key -> "true", | ||
| CometConf.COMET_EXTENDED_EXPLAIN_FORMAT.key -> | ||
| CometConf.COMET_EXTENDED_EXPLAIN_FORMAT_VERBOSE) { | ||
| val df = sql("SELECT hypot(a, b), levenshtein(s1, s2) FROM t") | ||
| checkSparkAnswerAndOperator(df) | ||
| val explain = | ||
| new ExtendedExplainInfo().generateExtendedInfo(df.queryExecution.executedPlan) | ||
| assert( | ||
| !explain.contains("JVM codegen dispatcher"), | ||
| s"expected NO codegen-dispatch info with the flag off, got:\n$explain") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| test("codegen dispatch fallback reasons name the expression") { | ||
| // Flag-off short-circuit tags the expression `<name>: <reason>` so distinct expressions | ||
| // don't collapse in the `Set[String]` roll-up. | ||
| withTable("t") { | ||
| sql("CREATE TABLE t (a DOUBLE, b DOUBLE) USING parquet") | ||
| sql("INSERT INTO t VALUES (3.0, 4.0)") | ||
| withSQLConf( | ||
| CometConf.COMET_SCALA_UDF_CODEGEN_ENABLED.key -> "false", | ||
| CometConf.COMET_EXTENDED_EXPLAIN_FORMAT.key -> | ||
| CometConf.COMET_EXTENDED_EXPLAIN_FORMAT_VERBOSE) { | ||
| val df = sql("SELECT hypot(a, b) FROM t") | ||
| checkSparkAnswer(df) | ||
| val explain = | ||
| new ExtendedExplainInfo().generateExtendedInfo(df.queryExecution.executedPlan) | ||
| assert( | ||
| explain.contains("hypot:") && | ||
| explain.contains(CometConf.COMET_SCALA_UDF_CODEGEN_ENABLED.key + "=false"), | ||
| s"expected 'hypot:' prefix and disabled-flag reason, got:\n$explain") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| test("dispatcher caches the compiled kernel across batches of one query") { | ||
| // Within a single query, the dispatcher compiles a kernel for the (expression, schema) pair | ||
| // once and reuses it across every subsequent batch of the same shape. Force multiple batches | ||
|
|
||
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.
The prose here already explains the dispatcher path well. One small suggestion: could you add a sentence explicitly noting the annotation only appears for expressions that lack a native DataFusion implementation? A reader could infer this from the surrounding paragraph, but making it explicit avoids anyone treating the tag as a general "this expression ran here" marker.