viirya commented on PR #5567:
URL:
https://github.com/apache/datafusion-comet/pull/5567#issuecomment-5505942567
@sunchao thanks — confirmed, and it is a query failure rather than a
fallback. I executed it.
Unit level, slicing two two-entry maps to drop the first:
```
sliced len=1 offsets=OffsetBuffer(ScalarBuffer([2, 4]))
Err = Arrow error: Invalid argument error: Max offset of 4 exceeds length of
entries 2
```
End to end, with the nested-hash config on:
```sql
SELECT * FROM (SELECT * FROM tbl ORDER BY _1 LIMIT 10 OFFSET 5) DISTRIBUTE
BY _2
```
```
CometExchange hashpartitioning(mapsort(_2#3383), 10), REPARTITION_BY_COL,
CometNativeShuffle
+- CometTakeOrderedAndProjectExec(limit=15, offset=5, orderBy=[_1#3382 ASC
NULLS FIRST])
+- CometNativeScan parquet [_1#3382,_2#3383]
CometNativeException: Error inserting batch: Arrow error:
Invalid argument error: Max offset of 30 exceeds length of entries 20
```
One correction to the framing, though: this is not only exposed by the newly
admitted
path. `mapsort` is not user-callable, but Spark 4.0+ has two rules that
insert it —
`InsertMapSortInRepartitionExpressions` and
`InsertMapSortInGroupingExpressions`. The
second one needs no repartition at all, so with the nested-hash config
**off** (its
default) and no map shuffle key anywhere:
```sql
SELECT _2, count(*) FROM (SELECT * FROM tbl ORDER BY _1 LIMIT 15 OFFSET 5)
GROUP BY _2
```
```
HashAggregate(keys=[_groupingmapsort#3400], functions=[count(1)])
+- HashAggregate(keys=[_groupingmapsort#3400], functions=[partial_count(1)])
+- CometTakeOrderedAndProjectExec(limit=20, offset=5, orderBy=[_1 ASC
NULLS FIRST])
+- CometNativeScan parquet [_1,_2]
CometNativeException: Invalid argument error: Max offset of 40 exceeds
length of entries 30
```
So it reproduces on current `main` and is a pre-existing bug rather than a
regression
from this PR. Since it stands on its own I have split the fix out into #5630
(issue
#5629) rather than carrying it here — it rebases the output offsets as you
suggested,
with a Rust unit test and an end-to-end test on the group-by path, both
verified to
fail without the fix.
I have taken the repartition-on-map coverage back out of this PR for now,
because on
`main` that path is still gated off and the test would be dead code. Once
#5630 merges
I will sync this branch and add it there, asserting both operators run
natively as you
asked.
--
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]
