Sort options for DataType::Dictionary is incorrect #4649

Activity

1. mingmwang

   added

   enhancementNew feature or requestNew feature or request

   on Dec 15, 2022
2. tustvold commented on Dec 16, 2022

   

   tustvold

   on Dec 16, 2022

   Contributor

   More actions

   The logic you link to is fairly well tested - https://github.com/apache/arrow-rs/blob/a0a5880665b1836890f6843b6b8772d81c463351/arrow-ord/src/sort.rs#L2998

   There is, however, a known issue with lexsort's handling of nested nulls in dictionaries - apache/arrow-rs#2687 although again it should be noted that arrow-rs will not generate such arrays naturally.
3. matthewmturner commented on Oct 27, 2026

   

   matthewmturner

   on Oct 27, 2026

   Last edited by matthewmturner

   Contributor

   More actions

   I tried to reproduce this like so but was not able to. i dont think any tests were ever added for this - do we think adding some SLT tests for example here with these cases would be helpful to ensure no regressions and proper integration with arrow kernel? Or is it okay to just close this?

   DataFusion CLI v55.1.0
   > CREATE TABLE t AS
     SELECT arrow_cast(v, 'Dictionary(Int32, Utf8)') AS d
     FROM (VALUES ('b'), (NULL), ('a'), ('c'), (NULL)) AS x(v);
   0 row(s) fetched.
   Elapsed 0.011 seconds.
   
   > SELECT arrow_typeof(d) FROM t LIMIT 1;
   +-------------------------+
   | arrow_typeof(t.d)       |
   +-------------------------+
   | Dictionary(Int32, Utf8) |
   +-------------------------+
   1 row(s) fetched.
   Elapsed 0.033 seconds.
   
   > SELECT d FROM t ORDER BY d ASC NULLS FIRST;
   +------+
   | d    |
   +------+
   | NULL |
   | NULL |
   | a    |
   | b    |
   | c    |
   +------+
   5 row(s) fetched.
   Elapsed 0.016 seconds.
   
   > SELECT d FROM t ORDER BY d ASC NULLS LAST;
   +------+
   | d    |
   +------+
   | a    |
   | b    |
   | c    |
   | NULL |
   | NULL |
   +------+
   5 row(s) fetched.
   Elapsed 0.001 seconds.
   
   > SELECT d FROM t ORDER BY d DESC NULLS FIRST;
   +------+
   | d    |
   +------+
   | NULL |
   | NULL |
   | c    |
   | b    |
   | a    |
   +------+
   5 row(s) fetched.
   Elapsed 0.001 seconds.
   
   > SELECT d FROM t ORDER BY d DESC NULLS LAST;
   +------+
   | d    |
   +------+
   | c    |
   | b    |
   | a    |
   | NULL |
   | NULL |
   +------+
   5 row(s) fetched.
   Elapsed 0.001 seconds.
   
4. matthewmturner commented on Oct 27, 2026

   

   matthewmturner

   on Oct 27, 2026

   Contributor

   More actions

   @xudong963 @zhuqi-lucas what do you think?
5. zhuqi-lucas commented on Oct 27, 2026

   

   zhuqi-lucas

   on Oct 27, 2026

   Contributor

   More actions

   @matthewmturner @alamb It may have been fixed by the arrow-rs dictionary sort rewrite (apache/arrow-rs#4605). arrow-rs now tests null dictionary values with all asc/desc and nulls first/last combinations (test_sort_int8_dicts), so I think it's fine to close.
6. alamb commented on Oct 27, 2026

   

   alamb

   on Oct 27, 2026

   Contributor

   More actions

   I tried to reproduce this like so but was not able to. i dont think any tests were ever added for this - do we think adding some SLT tests for example here with these cases would be helpful to ensure no regressions and proper integration with arrow kernel? Or is it okay to just close this?

   It wouldn't hurn to add SLT to be sure, but I also think closing is fine
7. zhuqi-lucas commented on Oct 27, 2026

   

   zhuqi-lucas

   on Oct 27, 2026

   Contributor

   More actions

   It wouldn't hurn to add SLT to be sure, but I also think closing is fine

   +1 it's fine to adding the SLT cases, they're cheap and also cover the DataFusion integration.
8. matthewmturner commented on Oct 27, 2026

   

   matthewmturner

   on Oct 27, 2026

   Contributor

   More actions

   Okay will work on that
9. matthewmturner commented on Oct 27, 2026

   

   matthewmturner

   on Oct 27, 2026

   Contributor

   More actions

   Here we go
10. alamb

    closed this as completedin #26834on Oct 27, 2026
11. harsh-ande

    added a commit that references this issue on Oct 27, 2026

    Ordering tests on dictionary types (apache#26834)

    ...

    985bbbf