mbutrovich commented on code in PR #6168:
URL: https://github.com/apache/datafusion-comet/pull/6168#discussion_r4095218998
##########
docs/source/user-guide/latest/iceberg.md:
##########
@@ -170,24 +172,31 @@ For a custom S3-compatible endpoint, configure the
catalog with the endpoint, pa
--conf spark.sql.catalog.s3_cat.s3.secret-access-key=...
```
-These `s3.*` storage properties are not specific to the Hive catalog shown
here. When `s3.access-key-id` / `s3.secret-access-key` are omitted, credentials
come from the standard AWS chain (environment variables, instance profiles, and
so on). `client.region` is auto-detected for AWS but should be set for non-AWS
endpoints. If your REST catalog vends temporary credentials, the native reader
does not consume them automatically, and wiring that requires the credential
provider bridge. See Iceberg's [S3
FileIO](https://iceberg.apache.org/docs/latest/aws/#s3-fileio) docs for the
full property list, and [S3 Credential Providers](s3-credential-providers.md)
for vended or per-request credentials.
+These `s3.*` storage properties are not specific to the Hive catalog shown
here. When `s3.access-key-id` / `s3.secret-access-key` are omitted, credentials
come from the standard AWS chain (environment variables, instance profiles, and
so on). `client.region` is not auto-detected: when neither it nor `AWS_REGION`
is set, Comet uses `us-east-1`, so set it for AWS buckets in any other region.
If your REST catalog vends temporary credentials, the native reader does not
consume them automatically, and wiring that requires the credential provider
bridge. See Iceberg's [S3
FileIO](https://iceberg.apache.org/docs/latest/aws/#s3-fileio) docs for the
full property list, and [S3 Credential Providers](s3-credential-providers.md)
for vended or per-request credentials.
Review Comment:
This describes the region default as depending only on `client.region` and
`AWS_REGION`. The check in
[`iceberg_common.rs#L139-L145`](https://github.com/apache/datafusion-comet/blob/c473dbb2adc37fcd77592ff1227a233c08577b78/native/core/src/execution/operators/iceberg_common.rs#L139-L145)
also accepts `s3.region` and `AWS_DEFAULT_REGION`. A reader who has set
`s3.region` could take this sentence to mean Comet ignores it.
`s3-credential-providers.md` in this PR already lists all four sources. How
about matching that wording here?
```suggestion
These `s3.*` storage properties are not specific to the Hive catalog shown
here. When `s3.access-key-id` / `s3.secret-access-key` are omitted, credentials
come from the standard AWS chain (environment variables, instance profiles, and
so on). The region is not auto-detected: when neither the catalog
(`client.region` or `s3.region`) nor the executor environment (`AWS_REGION` or
`AWS_DEFAULT_REGION`) supplies one, Comet uses `us-east-1`, so set it for AWS
buckets in any other region. If your REST catalog vends temporary credentials,
the native reader does not consume them automatically, and wiring that requires
the credential provider bridge. See Iceberg's [S3
FileIO](https://iceberg.apache.org/docs/latest/aws/#s3-fileio) docs for the
full property list, and [S3 Credential Providers](s3-credential-providers.md)
for vended or per-request credentials.
```
##########
docs/source/user-guide/latest/compatibility/index.md:
##########
@@ -111,12 +112,9 @@ Code that catches `SparkException` and only asserts on
message substrings is una
inspects the exception class, `getCondition()`, or the parameterised error
class will observe
divergence:
-- Byte / Short `Add`, `Subtract`, and `Multiply` overflow raises
`ARITHMETIC_OVERFLOW` where Spark
- 4.1 raises `BINARY_ARITHMETIC_OVERFLOW`, and Long overflow surfaces as
`"integer overflow"`
- rather than `"long overflow"`. `Abs` uses Rust type names (`Int8`, `Int64`,
...) in the message
- instead of Spark's SQL type names, and the scalar path of `UnaryMinus` on
Byte / Short emits a
- malformed message. The `try_` suggestion is omitted from all of these
- ([#5071](https://github.com/apache/datafusion-comet/issues/5071)).
+- Byte / Short `Add`, `Subtract`, and `Multiply` overflow raises
`ARITHMETIC_OVERFLOW` (for
+ example `byte overflow`) where Spark raises `BINARY_ARITHMETIC_OVERFLOW`,
and integral
+ `ARITHMETIC_OVERFLOW` messages omit Spark's `try_` suggestion.
Review Comment:
This is now the only bullet in the section without an issue link, since
#5071 was closed by #5162 with these two items still open. #6169 already asks
for a follow-up issue so this entry can link to it. Could you file that issue
now and add the link in this PR? The text ships with 1.1.0, and a reader who
hits the divergence has nowhere to follow it otherwise.
##########
docs/source/user-guide/latest/compatibility/operators.md:
##########
@@ -32,6 +32,27 @@ operator restrictions and aggregate buffer compatibility
checks still apply.
Parquet writes whose input plans contain an empty relation use Spark's writer
to preserve
readable empty output files and their schema metadata.
+## In-Memory Cache
+
+Comet can store cached relations (`df.cache()`, `CACHE TABLE`) in Arrow format
and scan them
+natively. This is experimental and disabled by default. Enable it with
+`spark.comet.exec.inMemoryCache.enabled=true` before the application starts:
the value at startup
+decides whether Comet sets `spark.sql.cache.serializer` to its Arrow cache
serializer, and
+because that is a static config the cache format is fixed for the application.
Comet does not
+replace a `spark.sql.cache.serializer` that the application has already set.
Disabling the
+setting later only sends cached scans back to Spark's execution path.
+
+Relations whose schema Comet's Arrow writer does not support are cached in
Spark's default
+format, and their scans fall back to Spark. Each cached column is stored as
its own compressed
+Arrow IPC stream, so a scan decodes only the columns it projects. Reads that
feed Spark operators
+rather than Comet operators still pay a row conversion that Spark's default
format avoids, and
+can be slower than Spark's cache.
+
+With `spark.kryo.registrationRequired=true`, also set
+`spark.kryo.registrator=org.apache.comet.CometKryoRegistrator` before creating
the
+`SparkContext`. Otherwise caching fails as soon as a block is serialized,
including the disk
+half of the default `MEMORY_AND_DISK` storage level.
Review Comment:
The Kryo requirement also applies to native broadcast, which is on by
default, so it isn't specific to the in-memory cache. `CometKryoRegistrator`
registers `Utils.arrowBytesKryoClasses` (the `Array[ChunkedByteBuffer]` that a
native broadcast ships) along with the cache classes
([`CometKryoRegistrator.scala`](https://github.com/apache/datafusion-comet/blob/c473dbb2adc37fcd77592ff1227a233c08577b78/spark/src/main/scala/org/apache/comet/CometKryoRegistrator.scala)).
The startup warning in
[`Plugins.scala#L141-L148`](https://github.com/apache/datafusion-comet/blob/c473dbb2adc37fcd77592ff1227a233c08577b78/spark/src/main/scala/org/apache/spark/Plugins.scala#L141-L148)
names both. A user with `spark.serializer=KryoSerializer` and
`spark.kryo.registrationRequired=true` who never enables the cache will hit
"Class is not registered" on the first broadcast join, and this is the only
page that mentions the registrator. Could you document it somewhere every user
reads, such as "Additional Conf
iguration" in `installation.md`, and have this section link to it? The
condition is also narrower than written here: it applies only when
`spark.serializer` is `org.apache.spark.serializer.KryoSerializer`.
--
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]
