malinjawi commented on code in PR #12647:
URL: https://github.com/apache/gluten/pull/12647#discussion_r4513086384
##########
backends-velox/src-delta33/main/scala/org/apache/spark/sql/delta/GlutenOptimisticTransaction.scala:
##########
@@ -91,53 +91,64 @@ class GlutenOptimisticTransaction(delegate:
OptimisticTransaction)
val empty2NullPlan =
convertEmptyToNullIfNeeded(queryExecution.executedPlan,
partitioningColumns, constraints)
- val maybeCheckInvariants = if (constraints.isEmpty) {
+ val rowInvariantPlan = if (constraints.isEmpty) {
// Compared to vanilla Delta, we simply avoid adding the invariant
checker
// when the constraint list is empty, to prevent the unnecessary
transitions
// from being added around the invariant checker.
empty2NullPlan
} else {
DeltaInvariantCheckerExec(empty2NullPlan, constraints)
}
Review Comment:
My bad @zhztheplayer, I only just saw your comment on #12030 from before it
was closed as stale. I missed it, and I wish I'd seen it sooner, because you
were right to push back on this.
The reason we put the check in the writer was to keep Delta's own
`DeltaInvariantViolationException`. An error raised inside Velox only reaches
Spark as a `GlutenException`, so the check had to throw on the JVM. In the
writer's task the batches are already on the JVM and sorted, so nothing native
runs above it. We never tried a columnar `DeltaInvariantCheckerExec`.
I've prototyped your approach since, and there's no real blocker. I wrote a
`ColumnarToColumnarExec` at Delta's position that calls the same
`firstNullColumnIndex`, and tested it on Spark 3.5.
On unpartitioned writes it works as is. On partitioned writes, and on
optimized writes with more than one input partition, Velox pulls the node's
batches through the writer's native sort or the shuffle's hash projection, so
the exception comes back as a `GlutenException`. Delta's row checker has the
same problem there today.
I think @felipepessoto #13560/#13561 fix that: they keep Java exceptions
intact when they pass through native code. With a small local stand-in for his
change, every path kept Delta's exception, and Delta's CHECK errors on
partitioned writes came out right too. At Delta's position the node also runs
before the writer turns empty partition strings into nulls, so the guards our
writer approach needed go away. The only cost I saw is that the node splits the
native sort into its own stage.
If we don't want to wait for #13561, the writer can lift the node above its
native sort. I tested that too: Delta's exception came through on every path,
and the sort stayed in the same stage. It still needs the
empty-partition-string guard, and the lift can be removed once #13561 lands.
Which would you prefer: the lifted version now, or the node at Delta's
position on top of #13561? And should CHECK, generated-column and nested NOT
NULL constraints go into this PR or a follow-up?
--
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]
