lgbo-ustc commented on code in PR #12349:
URL: https://github.com/apache/gluten/pull/12349#discussion_r3479724351
##########
gluten-substrait/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala:
##########
@@ -336,10 +336,16 @@ object ExpressionConverter extends SQLConfHelper with
Logging {
replaceWithExpressionTransformer0(m.child, attributeSeq,
expressionsMap),
m)
case m: MapFromEntries =>
+ val mapKeyDedupPolicy =
SQLConf.get.getConf(SQLConf.MAP_KEY_DEDUP_POLICY)
BackendsApiManager.getSparkPlanExecApiInstance.genMapFromEntriesTransformer(
- substraitExprName,
+ if (mapKeyDedupPolicy.toString ==
SQLConf.MapKeyDedupPolicy.LAST_WIN.toString) {
Review Comment:
I am a bit concerned about putting the LAST_WIN function-name rewrite in the
common ExpressionConverter. This changes the Substrait function name for all
backends when `spark.sql.mapKeyDedupPolicy=LAST_WIN`, but
`map_from_entries_last_win` is currently a ClickHouse-specific function
mapping. Other backends may receive this new function name without supporting
it.
Could we keep the common converter backend-agnostic and move this
policy-to-function-name decision into
`CHSparkPlanExecApi.genMapFromEntriesTransformer` instead? For example,
`ExpressionConverter` can continue passing `substraitExprName`, and the
ClickHouse backend can rewrite it to
`ExpressionNames.MAP_FROM_ENTRIES_LAST_WIN` only when
`SQLConf.MAP_KEY_DEDUP_POLICY == SQLConf.MapKeyDedupPolicy.LAST_WIN`.
--
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]
