anuragmantri commented on PR #16154:
URL: https://github.com/apache/iceberg/pull/16154#issuecomment-4786006519
I just took a look at the PR and all the historical context. The motivation
is clear to me. I echo @RussellSpitzer and @wombatu-kun's comments.
The global session conf is the right starting point. Every existing Iceberg
session conf (`vectorization.enabled`, `data-planning-mode`,
`advisory-partition-size`, `aggregate-push-down.enabled`) is a global key. This
follows that pattern exactly and is easy to reason about.
I'd also suggest removing the table-scoped mechanism from this PR. A few
reasons:
1. Spark itself has no per-table session config pattern anywhere. The
closest analog is spark.sql.catalog.<name>.* for catalog plugin, and that uses
prefix-based extraction (iterate all configs, regex-match the prefix, strip
it). There is no suffix approach.
2. Every `sessionConf()` caller in SparkConfParser now silently supports
table-scoping. Which is a larger behavior change that needs more discussion as
the earlier reviewers pointed out.
3. Table.name() format varies across catalog implementations. The user
must know exactly what string their catalog returns to construct the session
key, which is fragile.
I know this design was rejected earlier citing table ambiguity, but since
then Spark has added SQL options in SELECT
(https://github.com/apache/spark/pull/46707) and INSERT
(https://github.com/apache/spark/pull/47591). I think we should add the support
for MERGE, UPDATE and DELETE statements as well in Spark. I can attempt to make
these changes but I believe that will be the cleanest way to do this.
Also, I'm not a GPU programmer but I'm curious, isn't having a uniform split
size for all the tables involved in a session be better for GPU bandwidth
utilization?
--
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]
