[jira] [Commented] (CALCITE-5730) First nulls can be dropped by EnumerableLimitSort with offset

["Ruben Q L (Jira)" <ji...@apache.org>]

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

Ruben Q L commented on CALCITE-5730:
------------------------------------

[~gf2121], what [~julianhyde] means is that, normally, the procedure to contribute with a bug fix is as follows:
 - 1) Provide a unit test reproducing the problem, i.e. a test that systematically: either throws an exception, or (like in this case) returns an incorrect result.
 - 2) Provide the fix.
 - 3) The test from the first point that used to fail, now passes.

In this case, I think you can add in your PR this test class with the following test, which reproduces the problem (returns empty on the current Calcite code, returns one null value with the fix):
{code:java}
/** Tests for
 * {@link org.apache.calcite.adapter.enumerable.EnumerableLimitSort}. */
public class EnumerableLimitSortTest {

  /** Test case for
   * <a href="https://issues.apache.org/jira/browse/CALCITE-5730">[CALCITE-5730]
   * First nulls can be dropped by EnumerableLimitSort with offset</a>. */
  @Test void nullsFirstWithLimitAndOffset() {
    tester("select commission from emps order by commission nulls first limit 1 offset 1 ")
        .explainContains("EnumerableCalc(expr#0..4=[{inputs}], commission=[$t4])\n"
            + "  EnumerableLimitSort(sort0=[$4], dir0=[ASC-nulls-first], offset=[1], fetch=[1])\n"
            + "    EnumerableTableScan(table=[[s, emps]])")
        .returnsOrdered("commission=null");
  }

  private CalciteAssert.AssertQuery tester(String sqlQuery) {
    return CalciteAssert.that()
        .with(CalciteConnectionProperty.LEX, Lex.JAVA)
        .with(CalciteConnectionProperty.FORCE_DECORRELATE, false)
        .withSchema("s", new ReflectiveSchema(new HrSchemaBig()))
        .query(sqlQuery)
        .withHook(Hook.PLANNER, (Consumer<RelOptPlanner>) planner -> {
          planner.removeRule(EnumerableRules.ENUMERABLE_SORT_RULE);
          planner.addRule(EnumerableRules.ENUMERABLE_LIMIT_SORT_RULE);
        });
  }
}
{code}

> First nulls can be dropped by EnumerableLimitSort with offset
> -------------------------------------------------------------
>
>                 Key: CALCITE-5730
>                 URL: https://issues.apache.org/jira/browse/CALCITE-5730
>             Project: Calcite
>          Issue Type: Bug
>          Components: linq4j
>            Reporter: Feng Guo
>            Priority: Major
>              Labels: pull-request-available
>             Fix For: 1.35.0
>
>
> *Description*
> The EnumerableSortLimit Node deals with offset with following logic:
> {code:java}
> // skip the first 'offset' rows by deleting them from the map
> if (offset > 0) {
>   // search the key up to (but excluding) which we have to remove entries from the map
>   int skipped = 0;
>   TKey until = null;
>   for (Map.Entry<TKey, List<TSource>> e : map.entrySet()) {
>     skipped += e.getValue().size();
>     if (skipped > offset) {
>       // we might need to remove entries from the list
>       List<TSource> l = e.getValue();
>       int toKeep = skipped - offset;
>       if (toKeep < l.size()) {
>         l.subList(0, l.size() - toKeep).clear();
>       }
>       until = e.getKey();
>       break;
>     }
>   }
>   if (until == null) {
>     // the offset is bigger than the number of rows in the map
>     return Linq4j.emptyEnumerator();
>   }
>   map.headMap(until, false).clear();
> }
> {code}
>  In a NULLS FIRST sort, if we set offset=1, limit = 1 when first 10 rows have null compare key, the until will be null. But that does not mean offset bigger than number of rows, it should have results instead of emptyEnumerator;



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