comphead commented on code in PR #7477:
URL: https://github.com/apache/datafusion-comet/pull/7477#discussion_r4532818974
##########
docs/source/contributor-guide/adding_a_new_expression.md:
##########
@@ -466,9 +466,9 @@ test("bit_length dispatches on binary and stays native on
string") {
Names are the expression's `prettyName` lowercased. Naming an expression
asserts both that it ran
through the expected mechanism and that it did not run through the other one,
so a serde that
-later gains a native path (or loses its dispatcher route) fails here instead
of passing unnoticed.
-Prefer the SQL file equivalents, `expect_native(...)` and
`expect_dispatch(...)`, when the test
-fits in a fixture.
+later gains a native implementation (or loses its dispatcher route) fails here
instead of passing
Review Comment:
The `bit_length` example a few lines above has the same problem that this PR
fixes in `sql-file-tests.md`. `CometBitLength` returns `Unsupported` for
`BinaryType` and does not mix in `CodegenDispatchFallback`, so `SELECT
bit_length(b)` falls back to Spark (`bit_length.sql` pins it with
`expect_fallback(bit_length on BinaryType is not supported)`). The `dispatched
= Seq("bit_length")` assertion would fail as written. The test name "stays
native on string" also still uses a phrase that N1 rules out. Could this
example use an expression that really splits by mechanism, for example `lower`
with and without `spark.comet.caseConversion.enabled`, as the SQL file page now
does?
##########
docs/source/user-guide/latest/tuning/shuffle.md:
##########
@@ -115,7 +115,7 @@ even when both of those aggregates run on Spark.
The native shuffle writer buffers a task's output in memory and spills it to
disk. By default
(`spark.comet.shuffle.native.maxBufferBytes=0`) it spills only when the memory
pool denies an allocation, so it holds
as much of the task's share of the off-heap pool as it can get. That can leave
a sort or an aggregation in the same
-task without memory, and the large buffers it frees at the end of the task can
stay resident in the native allocator,
+task without memory, and the large buffers it frees at the end of the task can
stay resident in the memory allocator,
Review Comment:
Dropping "native" leaves "the memory allocator", which a reader could take
to mean the JVM's. `tuning/memory.md` explains that this is the system
allocator used by Comet's Rust code, and the `spark.comet.memory.logInterval`
doc says "the Rust allocator". Would "stay resident in the Rust allocator" keep
the meaning and match the other pages?
--
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]
