Copilot commented on code in PR #12818:
URL: https://github.com/apache/gluten/pull/12818#discussion_r3809737608
##########
backends-clickhouse/src/test/scala/org/apache/gluten/execution/tpch/GlutenClickHouseTPCHSaltNullParquetSuite.scala:
##########
@@ -878,6 +878,80 @@ class GlutenClickHouseTPCHSaltNullParquetSuite
compareResultsAgainstVanillaSpark(sql, true, { _ => })
}
+ test("grouping sets preserves nullable columns across union") {
+ val sql =
+ """
+ |select msg_type, os, count(*) as cnt
+ |from (
+ | select 'file' as msg_type, 'Android' as os, id from range(10)
+ | union all
+ | select 'file' as msg_type, 'iOS' as os, id from range(10)
+ |) t
+ |group by grouping sets ((msg_type), (os))
+ |having msg_type is not null
+ |order by msg_type, os, cnt
+ |""".stripMargin
+
withSparkEnvAndSQLConf(CHConfig.runtimeConfig("enable_lazy_aggregate_expand"),
"false") {
+ compareResultsAgainstVanillaSpark(
+ sql,
+ true,
+ {
+ df =>
+ val expands =
collectWithSubqueries(df.queryExecution.executedPlan) {
+ case e: ExpandExecTransformer
+ if !e.child.isInstanceOf[HashAggregateExecBaseTransformer] =>
+ e
+ }
+ assert(expands.size == 1)
+ }
+ )
+ }
+ }
+
+ test("lazy aggregate expand preserves nullable columns across union") {
+ val sql =
+ """
+ |select msg_type, os, count(*) as cnt
+ |from (
+ | select 'file' as msg_type, 'Android' as os, id from range(10)
+ | union all
+ | select 'file' as msg_type, 'iOS' as os, id from range(10)
+ |) t
+ |group by grouping sets ((msg_type), (os))
+ |having msg_type is not null
+ |order by msg_type, os, cnt
+ |""".stripMargin
+
withSparkEnvAndSQLConf(CHConfig.runtimeConfig("enable_lazy_aggregate_expand"),
"true") {
+ compareResultsAgainstVanillaSpark(
+ sql,
+ true,
+ {
+ df =>
+ val expands =
collectWithSubqueries(df.queryExecution.executedPlan) {
+ case e: ExpandExecTransformer
+ if e.child.isInstanceOf[HashAggregateExecBaseTransformer] =>
+ e
+ }
+ assert(expands.size == 1)
+ }
+ )
+ }
+ }
+
+ private def withSparkEnvAndSQLConf(key: String, value: String)(f: => Unit):
Unit = {
+ val sparkConf = SparkEnv.get.conf
+ val previousValue = sparkConf.getOption(key)
+ sparkConf.set(key, value)
+ try {
+ withSQLConf(key -> value)(f)
+ } finally {
Review Comment:
`CHBackend.enableLazyAggregateExpand()` is read from `SparkEnv.get.conf`
(not `SQLConf`), so the `withSQLConf(key -> value)` wrapper here is redundant
for the only current call sites and can be removed to reduce confusion about
which config source actually controls the behavior under test.
##########
cpp-ch/local-engine/Parser/RelParsers/ExpandRelParser.cpp:
##########
@@ -113,6 +114,12 @@ ExpandField ExpandRelParser::buildExpandField(const
DB::Block & header, const su
else if (project_expr.has_literal())
{
auto [type, field] = parseLiteral(project_expr.literal());
+ // A NULL literal is nullable even when the type carried by the
+ // Substrait literal does not explicitly encode nullability.
+ // Keep that information in the Expand output type so the
+ // generated column contains a real null map.
+ if (field.isNull() && type && !type->isNullable())
+ type = std::make_shared<DB::DataTypeNullable>(type);
Review Comment:
Instead of constructing `DataTypeNullable` directly, prefer
`DB::makeNullable(type)` for consistency with the rest of the codebase and to
let ClickHouse handle edge cases (e.g., nested wrappers) in one place.
--
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]
