[jira] [Commented] (CALCITE-7557) Linq4j.ListEnumerable.take(int) / skip(int) diverge from EnumerableDefaults on negative counts

["microbluey (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CALCITE-7557?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18099350#comment-18099350 ] 

microbluey commented on CALCITE-7557:
-------------------------------------

[~rickydong] are you still planning to submit a PR for this? If you've moved on I'd be happy to pick it up, but I don't want to duplicate your work.
One data point that arrived after this discussion: CALCITE-7624 (merged 9 Jul) added take(BigDecimal)/skip(BigDecimal) to ListEnumerable, and both clamp via count.max(BigDecimal.ZERO). So the int overloads are now the only ones in that class that don't clamp, which makes the LINQ-consistent direction agreed above look even more clearly right.

> Linq4j.ListEnumerable.take(int) / skip(int) diverge from EnumerableDefaults on negative counts
> ----------------------------------------------------------------------------------------------
>
>                 Key: CALCITE-7557
>                 URL: https://issues.apache.org/jira/browse/CALCITE-7557
>             Project: Calcite
>          Issue Type: Bug
>          Components: linq4j
>    Affects Versions: 1.41.0
>            Reporter: Ruiqi Dong
>            Priority: Major
>
> *Summary*
> Linq4j.asEnumerable(list) exposes list-specialized take(int) and skip(int) implementations that are not behaviorally equivalent to the generic EnumerableDefaults versions for negative counts. For the same input enumerable and the same negative count:
>  * EnumerableDefaults.take(source, -1) returns an empty enumerable, but ListEnumerable.take(-1) throws
>  * EnumerableDefaults.skip(source, -1) returns the original sequence, but ListEnumerable.skip(-1) throws
> This is a clean semantic divergence between the generic and optimized paths of the same API.
>  
> *Affected code*
> File: linq4j/src/main/java/org/apache/calcite/linq4j/EnumerableDefaults.java
> {code:java}
> public static <TSource> Enumerable<TSource> skip(Enumerable<TSource> source,
>     final int count) {
>   return skipWhile(source, (v1, v2) -> {
>     return v2 < count;
>   });
> }
> public static <TSource> Enumerable<TSource> take(Enumerable<TSource> source,
>     final int count) {
>   return takeWhile(
>       source, (v1, v2) -> {
>         return v2 < count;
>       });
> } {code}
> File: linq4j/src/main/java/org/apache/calcite/linq4j/Linq4j.java
> {code:java}
> @Override public Enumerable<T> skip(int count) {
>   final List<T> list = toList();
>   if (count >= list.size()) {
>     return Linq4j.emptyEnumerable();
>   }
>   return new ListEnumerable<>(list.subList(count, list.size()));
> }
> @Override public Enumerable<T> take(int count) {
>   final List<T> list = toList();
>   if (count >= list.size()) {
>     return this;
>   }
>   return new ListEnumerable<>(list.subList(0, count));
> }{code}
>  
> *Reproducer*
> Add the following test to linq4j/src/test/java/org/apache/calcite/linq4j/test/Linq4jTest.java
> {code:java}
>  @Test void testTakeListEnumerableNegativeSize() {
>   final List<Integer> values = Arrays.asList(1, 2, 3);
>   assertThat(EnumerableDefaults.take(Linq4j.asEnumerable(values), -1).toList(),
>       is(empty()));
>   assertThat(Linq4j.asEnumerable(values).take(-1).toList(), is(empty()));
> }
> @Test void testSkipListEnumerableNegativeSize() {
>   final List<Integer> values = Arrays.asList(1, 2, 3);
>   assertThat(EnumerableDefaults.skip(Linq4j.asEnumerable(values), -1).toList(),
>       is(values));
>   assertThat(Linq4j.asEnumerable(values).skip(-1).toList(), is(values));
> }{code}
> Run:
> {code:java}
> ./gradlew :linq4j:test \
>   --tests org.apache.calcite.linq4j.test.Linq4jTest.testTakeListEnumerableNegativeSize \
>   --tests org.apache.calcite.linq4j.test.Linq4jTest.testSkipListEnumerableNegativeSize {code}
> Observed behavior:
> The optimized list path throws in both methods instead of matching the generic enumerable semantics
> {code:java}
> java.lang.IllegalArgumentException: fromIndex(0) > toIndex(-1)
>     at java.base/java.util.AbstractList.subListRangeCheck(AbstractList.java:509)
>     at java.base/java.util.AbstractList.subList(AbstractList.java:497)
>     at org.apache.calcite.linq4j.Linq4j$ListEnumerable.take(Linq4j.java:599)
> java.lang.IndexOutOfBoundsException: fromIndex = -1
>     at java.base/java.util.AbstractList.subListRangeCheck(AbstractList.java:505)
>     at java.base/java.util.AbstractList.subList(AbstractList.java:497)
>     at org.apache.calcite.linq4j.Linq4j$ListEnumerable.skip(Linq4j.java:591) {code}
> Expected behavior:
> Negative counts should behave the same regardless of whether the source enumerable happens to be backed by a List.
>  * Negative `take` should return an empty enumerable, matching EnumerableDefaults.take
>  * Negative `skip` should return the original sequence, matching EnumerableDefaults.skip
>  
> This is an optimization-path semantic regression. The same logical operation has two implementations in Calcite, but the list-specialized path is not behaviorally equivalent to the generic enumerable contract for negative counts.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)