andygrove commented on code in PR #6216:
URL: https://github.com/apache/datafusion-comet/pull/6216#discussion_r4104800969
##########
spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeWrite.scala:
##########
@@ -217,6 +218,23 @@ object CometIcebergNativeWrite extends
CometOperatorSerde[IcebergWriteExec] {
if (ctx.properties.contains(key)) Some(s"$key is set ($reason)") else
None
}
+ // The property rule above only sees providers configured through
table/write properties. A
+ // custom TableOperations can return a LocationProvider directly, while the
native writer always
+ // generates `<data location>/<partition path>/<file>`. Admit only Iceberg's
default provider;
+ // object-storage layout is already declined by the preceding property rule.
Review Comment:
I checked that the new detection test fails without this rule, since the
gate returns `Compatible`, so the gap in #6144 was real. One thing I found
while checking parity: iceberg-java only honours a provider that
`TableOperations` returns directly from Iceberg 1.11 on. Before that,
`SerializableTable` keeps the provider in a transient field and rebuilds it
from the table location and properties on each executor, so iceberg-java writes
the default layout too. I confirmed it with a probe that writes through
iceberg-java using a provider that prefixes file names. None were prefixed on
1.5.2 or 1.10.0, and all were on 1.11.0.
So on the 3.4, 3.5, 4.0 and 4.2 profiles this rule declines writes that
would have matched. I think keeping it unconditional is the right call. Could
the comment say that, so the next reader doesn't have to rediscover why a Spark
3.5 write falls back?
##########
docs/source/contributor-guide/iceberg-writes.md:
##########
@@ -133,8 +133,8 @@ The gate has these properties, and a new rule has to keep
them:
overlaid with `SparkWrite.writeProperties`, which is where iceberg-java
resolves per-write options
and `spark.sql.iceberg.*` session overrides. Reading table properties alone
misses those.
- **It checks instantiated state as well as properties.** A catalog or a
custom `TableOperations`
- can install a `FileIO` or an `EncryptionManager` without any property
changing, so
- `requireRecognizedTableFileIO` and `requirePlaintextEncryptionManager` look
at `table.io()` and
+ can install a `LocationProvider`, `FileIO`, or `EncryptionManager` without
any property changing,
+ so the corresponding rules inspect `table.locationProvider()`, `table.io()`,
and
Review Comment:
This used to name `requireRecognizedTableFileIO` and
`requirePlaintextEncryptionManager`, and the rest of this section names its
rules too, which makes it easy to go from the guide to the code. Could we keep
the names and add `requireDefaultLocationProvider`, rather than saying "the
corresponding rules"?
--
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]
